Re: [aur-dev][PATCH] Fix notifications emails going to the right people, part #2
On 10/26/18 10:00 AM, Eli Schwartz wrote:
> Notifications are still going to the wrong people. We tried to fix this
> in commit b702e5c0e7f13103fc764b7e5613f78f3e7acd30, but only fixed it
> for the python callers. There's another caller in the php code, which
> needs to use the right order of arguments as well.
>
> Fixes FS#60602
Obviously the version of the patch I actually merged to pu, references
the correct FS#60601 :)
Hotpatched in the live instance.
> Signed-off-by: Eli Schwartz
> ---
>
> The division between the python and php code is, well, sort of awkward,
> who knew? And I guess the php callers would be the more common cause
> here, so...
>
> Anyway, hopefully this actually fixes things for the common case.
>
> web/lib/pkgbasefuncs.inc.php | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/web/lib/pkgbasefuncs.inc.php b/web/lib/pkgbasefuncs.inc.php
> index b39bca5..1df21a2 100644
> --- a/web/lib/pkgbasefuncs.inc.php
> +++ b/web/lib/pkgbasefuncs.inc.php
> @@ -733,7 +733,7 @@ function pkgbase_adopt ($base_ids, $action=true, $via) {
> }
>
> foreach ($base_ids as $base_id) {
> - notify(array($action ? 'adopt' : 'disown', $base_id, $uid));
> + notify(array($action ? 'adopt' : 'disown', $uid, $base_id));
> }
>
> if ($action) {
> @@ -1204,7 +1204,7 @@ function pkgbase_set_comaintainers($base_id, $users,
> $override=false) {
> foreach ($uids_new as $uid) {
> if (in_array($uid, $uids_add)) {
> $q = sprintf("INSERT INTO PackageComaintainers
> (PackageBaseID, UsersID, Priority) VALUES (%d, %d, %d)", $base_id, $uid, $i);
> - notify(array('comaintainer-add', $base_id, $uid));
> + notify(array('comaintainer-add', $uid, $base_id));
> } else {
> $q = sprintf("UPDATE PackageComaintainers SET Priority
> = %d WHERE PackageBaseID = %d AND UsersID = %d", $i, $base_id, $uid);
> }
> @@ -1216,7 +1216,7 @@ function pkgbase_set_comaintainers($base_id, $users,
> $override=false) {
> foreach ($uids_rem as $uid) {
> $q = sprintf("DELETE FROM PackageComaintainers WHERE
> PackageBaseID = %d AND UsersID = %d", $base_id, $uid);
> $dbh->exec($q);
> - notify(array('comaintainer-remove', $base_id, $uid));
> + notify(array('comaintainer-remove', $uid, $base_id));
> }
>
> return array(true, __("The package base co-maintainers have been
> updated."));
>
--
Eli Schwartz
Bug Wrangler and Trusted User
signature.asc
Description: OpenPGP digital signature
Re: [aur-dev][PATCH] Fix notifications emails going to the right people
On 8/11/18 4:25 AM, Lukas Fleischer wrote: > Wow, good catch! > > The patch is totally correct but the suspected cause is not. I think > thus is a regression introduced by f3b4c5c (Refactor the notification > script, 2018-05-17) where some of the parameters where accidentally > pushed around. We're still getting a small handful of reports about this, and I'm not sure why. When was this deployed to aur.archlinux.org? -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [aur-dev][PATCH] Fix notifications emails going to the right people
On 8/11/18 4:25 AM, Lukas Fleischer wrote: > On Fri, 10 Aug 2018 at 23:26:28, Eli Schwartz wrote: >> The notify script expects to see the userid followed by additional >> arguments like the pkgbase id, however, these were getting sent swapped >> around (presumably due to the similarity with the db connection which >> expects them in the other order when processing SQL statements). >> >> As a result, some random userid with the same id as the pkgbase, got sent a >> notification regarding some package with the same id as the real user's id. >> >> Signed-off-by: Eli Schwartz >> --- >> aurweb/git/serve.py | 10 +- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> [...] > > Wow, good catch! > > The patch is totally correct but the suspected cause is not. I think > thus is a regression introduced by f3b4c5c (Refactor the notification > script, 2018-05-17) where some of the parameters where accidentally > pushed around. > > It is a shame that our test suite did not catch this but looking at the > tests... > > "$NOTIFY" adopt 1 1 && > [...] > "$NOTIFY" comaintainer-add 1 1 && > [...] > "$NOTIFY" comaintainer-remove 1 1 && > > ... it is not much of a surprise either. Maybe we should try to make > each parameter unique? I also realized that we don't seem to test disown > notifications at all! > > I will add a reference to f3b4c5c (Refactor the notification script, > 2018-05-17) to the commit message, merge to pu and patch our live setup > in a minute. Looks good to me. Minor nit: s/where/were/ in the commit message. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [aur-dev][PATCH] Fix notifications emails going to the right people
On Fri, 10 Aug 2018 at 23:26:28, Eli Schwartz wrote: > The notify script expects to see the userid followed by additional > arguments like the pkgbase id, however, these were getting sent swapped > around (presumably due to the similarity with the db connection which > expects them in the other order when processing SQL statements). > > As a result, some random userid with the same id as the pkgbase, got sent a > notification regarding some package with the same id as the real user's id. > > Signed-off-by: Eli Schwartz > --- > aurweb/git/serve.py | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > [...] Wow, good catch! The patch is totally correct but the suspected cause is not. I think thus is a regression introduced by f3b4c5c (Refactor the notification script, 2018-05-17) where some of the parameters where accidentally pushed around. It is a shame that our test suite did not catch this but looking at the tests... "$NOTIFY" adopt 1 1 && [...] "$NOTIFY" comaintainer-add 1 1 && [...] "$NOTIFY" comaintainer-remove 1 1 && ... it is not much of a surprise either. Maybe we should try to make each parameter unique? I also realized that we don't seem to test disown notifications at all! I will add a reference to f3b4c5c (Refactor the notification script, 2018-05-17) to the commit message, merge to pu and patch our live setup in a minute. Thanks! Best regards, Lukas
