[Koha-patches] [PATCH] user privacy managing and dealing with AnonymousPatron new syspref

[Joe Atzberger]

The SQL forming here is rather fragile.


    my $query          = "
>         UPDATE old_issues
> -        SET    borrowernumber = NULL
> +        SET    borrowernumber =
> ".C4::Context->preference('AnonymousPatron')."
>         WHERE  returndate < '".$date."'
>           AND borrowernumber IS NOT NULL
> +          AND (SELECT privacy FROM borrowers WHERE
> borrowers.borrowernumber=old_issues.borrowernumber)<>0
>     ";
>     $query .= " AND borrowernumber = '".$borrowernumber."'" if defined
> $borrowernumber;


This would be safer with placeholders, especially since the users can go
delete the AnonymousPatron syspref, and the API is expecting a certain
format of date *string*, not a C4::Dates object.  Might make sense to do a
parameter check, and default to "today", if not passed a date.

Also, I haven't tested it, but the subquery in the WHERE clause probably
forces bad performance compared to a JOIN.  Since this could run on the
entire old_issues table (in some libraries, the largest table!) if not
passed a borrowernumber, subquerying may be particularly costly.


> +    my $sth=$dbh->prepare("SELECT value FROM systempreferences WHERE
> variable='AnonSuggestions'");
> +    $sth->execute;
> +    my ($value) = $sth->fetchrow();
> +    $dbh->do("UPDATE systempreferences SET value=$value WHERE
> variable='AnonymousPatron'");


This will crash updatedatabase if AnonSuggestions was deleted.  Recommend
having a fallback value if not defined($value).


> +                <!-- TMPL_IF name="privacy0" -->
> +                    <option value="0" selected="1">Forever</option>


To be valid, those options attributes need to be:
selected="selected"

(I think it's annoying too.)

-- 
Joe Atzberger
LibLime - Open Source Library Solutions
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20090724/1c4fbfd3/attachment-0002.htm>