[aur-dev] [PATCH] send emails when delteing packages

[Dan McGee]

On Thu, Sep 8, 2011 at 6:54 AM, Lukas Fleischer
<archlinux at cryptocrack.de> wrote:
> On Thu, Sep 08, 2011 at 10:45:33AM +0200, Florian Pritz wrote:
>> Signed-off-by: Florian Pritz <bluewind at xinu.at>
>> ---
>>  web/lib/pkgfuncs.inc.php |   43 +++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 43 insertions(+), 0 deletions(-)
>>
>
> As I already stated in another thread, I'm not really sure if we want
> this and if this is the Arch way of informing users.
>
> How will we handle mass-deletes in future if users expect mail
> notifications on package removal (we currently run a simple DELETE query
> on the database)? Keep in mind that this might be confusing if a package
> is moved to the binary repositories, also. Trusted Users and/or
> developers should at least drop one more comment before moving a package
> in this case.
>
> +/-0 from me. Any other opinions (I'll review the patch anyway)?
>
>> diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php
>> index 3e89fa3..b6d4d08 100644
>> --- a/web/lib/pkgfuncs.inc.php
>> +++ b/web/lib/pkgfuncs.inc.php
>> @@ -710,6 +710,49 @@ function pkg_delete ($atype, $ids, $mergepkgid, $dbh=NULL) {
>>       }
>>
>>       if ($mergepkgid) {
>> +             $mergepkgname = pkgname_from_id($mergepkgid, $dbh);
>> +     }
>> +
>> +     # Send email notifications
>> +     foreach ($ids as $pkgid) {
>> +             $q = 'SELECT CommentNotify.*, Users.Email ';
>> +             $q.= 'FROM CommentNotify, Users ';
>> +             $q.= 'WHERE Users.ID = CommentNotify.UserID ';
>> +             $q.= 'AND CommentNotify.UserID != ' . uid_from_sid($_COOKIE['AURSID']) . ' ';
>> +             $q.= 'AND CommentNotify.PkgID = ' . intval($pkgid);
>
> No real need to convert to an int here. We already sanitized the "$ids"
> array before.
>
>> +             $result = db_query($q, $dbh);
>> +             $bcc = array();
>> +
>> +             if (mysql_num_rows($result)) {
>
> Uh, we should really get rid of all mysql_*() invocations and write
> wrappers for fetching results as soon as possible...
Not to mention num_rows is a bad idea anyway here. Loop your results
and then check that array for size > 0 rather than having to ask the
database for a number you don't actually care about (the only thing
cared about is the existence of at least one row).
>
>> +                     while ($row = mysql_fetch_assoc($result)) {
>> +                             array_push($bcc, $row['Email']);
>> +                     }
>> +
>> +                     $q = 'SELECT Packages.* ';
>> +                     $q.= 'FROM Packages ';
>> +                     $q.= 'WHERE Packages.ID = ' . intval($pkgid);
>> +                     $result = db_query($q, $dbh);
>> +                     $row = mysql_fetch_assoc($result);
>
> Any reason not to use pkgname_from_id() here? :)
>
>> +
>> +                     # TODO: native language emails for users, based on their prefs
>> +                     # Simply making these strings translatable won't work, users would be
>> +                     # getting emails in the language that the user who posted the comment was in
>> +                     $body = "";
>> +                     if ($mergepkgid) {
>> +                             $body .= username_from_sid($_COOKIE['AURSID']) . " merged \"".$row["Name"]."\" into \"$mergepkgname\".\n\n";
>> +                             $body .= "\n\n---\nYou will no longer receive notifications about this package, please go to https://aur.archlinux.org/packages.php?ID=".$mergepkgid." and click the Notify button if you wish to recieve them again.";
>> +                     } else {
>> +                             $body .= username_from_sid($_COOKIE['AURSID']) . " deleted \"".$row["Name"]."\".\n\n";
>> +                             $body .= "\n\n---\nYou will no longer receive notifications about this package.";
>> +                     }
>> +                     $body = wordwrap($body, 70);
>> +                     $bcc = implode(', ', $bcc);
>> +                     $headers = "Bcc: $bcc\nReply-to: nobody at archlinux.org\nFrom: aur-notify at archlinux.org\nX-Mailer: AUR\n";
>> +                     @mail(' ', "AUR Package deleted: " . $row['Name'], $body, $headers);
>
> So this means we will send a whole bunch of notifications if, say,
> forty-two packages are deleted at once? I do not really insist on
> bundling mails since we will not mass-delete packages very often.
>
> We should also ensure that none of the fields included in the headers
> might contain a newline in any case. I don't want sigurd to become a
> spam catapult. Having a look at valid_email(), which is used to validate
> mail addresses during the register process, it doesn't seem like there's
> any possibility to inject a line break here, though.
>
>> +             }
>> +     }
>> +
>> +     if ($mergepkgid) {
>>               /* Merge comments */
>>               $q = "UPDATE PackageComments ";
>>               $q.= "SET PackageID = " . intval($mergepkgid) . " ";
>> --
>> 1.7.6.1
>
> Thanks!
>