On Thu, Sep 08, 2011 at 10:45:33AM +0200, Florian Pritz wrote:
> Signed-off-by: Florian Pritz <[email protected]>
> ---
> 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...
> + 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:
> [email protected]\nFrom: [email protected]\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!
