On Fri, Jul 27, 2012 at 6:02 PM, Nicolas Cornu <nicolac76@yahoo.fr> wrote:

Responding to FS #30109 --- web/html/pkgsubmit.php | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)

diff --git a/web/html/pkgsubmit.php b/web/html/pkgsubmit.php index 84688b4..2e6bf28 100644 --- a/web/html/pkgsubmit.php +++ b/web/html/pkgsubmit.php @@ -397,6 +397,38 @@ if ($uid): pkg_notify(account_from_sid($_COOKIE["AURSID"], $dbh), array($packageID), true, $dbh); }

+ # If it was a previously created package notify users of this update. + if($pdata) { + $uid = uid_from_sid($uid, $dbh); + + # Send email notifications + $q = 'SELECT u.Email FROM Users u '; + $q.= 'INNER JOIN CommentNotify cn ON u.ID=cn.UserID '; + $q.= 'AND cn.UserID != ' . $uid . ' '; + $q.= 'AND cn.PkgID = ' . intval($packageID); + $result = db_query($q, $dbh); + $bcc = array(); + + if (mysql_num_rows($result)) { + while ($row = mysql_fetch_assoc($result)) { + array_push($bcc, $row['Email']); + } + + # 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 = + 'from ' . $AUR_LOCATION . '/' . get_pkg_uri($pkg_name) . "\n" + . username_from_sid($_COOKIE['AURSID'], $dbh) . " update the package to the version: " + . $pkg_version + . "\n\n---\nIf you no longer wish to receive notifications about this package, please go the the above package page and click the UnNotify button."; + $body = wordwrap($body, 70); + $bcc = implode(', ', $bcc); + $headers = "Bcc: $bcc\nReply-to: nobody@archlinux.org\nFrom: aur-notify@archlinux.org\nX-Mailer: AUR\n"; + @mail('undisclosed-recipients: ;', "AUR Update for " . $pkg_name, $body, $headers); + } + } + # Entire package creation process is atomic end_atomic_commit($dbh);

-- 1.7.11.3

I'm not a fan of this patch for a couple of reasons. First, it introduces SQL code in web/html. Multiple patches have attempted to limit SQL code to the web/lib location. This patch also assumes everyone who wants comment notifications also wants notifications for package updates. I don't think that is necessarily the case. Those individuals will have many more e-mails with this patch. I think the best way to solve this is with what I proposed with https://bugs.archlinux.org/task/30109#comment95855 previously.

The shortened table names are my fault ;) From my point of view this makes the join syntax more readable. But this is only a matter of taste. Sent from my iPhone On 28.07.2012, at 01:11, "canyonknight@gmail.com" <canyonknight@gmail.com> wrote:

On Fri, Jul 27, 2012 at 6:02 PM, Nicolas Cornu <nicolac76@yahoo.fr> wrote:

...

--- web/lib/pkgfuncs.inc.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index 0610617..969da11 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -215,11 +215,10 @@ function add_package_comment($pkgid, $uid, $comment, $dbh=NULL) { db_query($q, $dbh);

# Send email notifications - $q = 'SELECT CommentNotify.*, Users.Email '; - $q.= 'FROM CommentNotify, Users '; - $q.= 'WHERE Users.ID = CommentNotify.UserID '; - $q.= 'AND CommentNotify.UserID != ' . $uid . ' '; - $q.= 'AND CommentNotify.PkgID = ' . intval($pkgid); + $q = 'SELECT u.Email FROM Users u ';

Is there any reason for this to shorten Users to u? Personally I find it easier to parse something like 'SELECT Users.Email FROM Users' compared to yours. This also isn't really done anywhere else in the codebase.

...

+ $q.= 'INNER JOIN CommentNotify cn ON u.ID=cn.UserID'; + $q.= 'AND cn.UserID != ' . $uid . ' '; + $q.= 'AND cn.PkgID = ' . intval($pkgid); $result = db_query($q, $dbh); $bcc = array();

@@ -228,7 +227,7 @@ function add_package_comment($pkgid, $uid, $comment, $dbh=NULL) { array_push($bcc, $row['Email']); }

- $q = 'SELECT Packages.* '; + $q = 'SELECT Packages.Name '; $q.= 'FROM Packages '; $q.= 'WHERE Packages.ID = ' . intval($pkgid); $result = db_query($q, $dbh); -- 1.7.11.3

I do like you replacing SELECT * with what specific columns are being selected. I also like the explicit join syntax.