658036 - Duplicate emails in bulk compatibility check notification

Status: VERIFIED FIXED

Description

[krupa raj[:krupa]]

Created attachment 533387 [details]
screenshot

steps to reproduce:
1. Load https://addons.allizom.org/z/en-US/admin/validation
2. Click on 'Download Email log'
3. In the downloaded csv file. search for 'Wappalyzer'


observed behavior:
The same owner of the add-ons 'Wappalyzer' is being emailed 5 times. This add-on is fully reviewed and has no versions which are awaiting review (https://addons.allizom.org/en-US/developers/addon/wappalyzer/versions/)

Comment 1

[Kumar McMillan [:kumar] (needinfo all the things)]

There are two errors at play:

1. Two emails are getting sent for the same validation result.  This seems to be specific to the wappalyzer addon and perhaps is either related to the SQL query that starts validation or the query that selects authors to email.

2. The same author is getting emailed for each version that does not pass the compatibility test.  This is to be expected with the current behavior of validating not only the public version but any version that may be pending review.  Is it ok to send dupe emails like that?

Comment 2

[Justin Scott [:fligtar]]

(In reply to comment #1)
> 2. The same author is getting emailed for each version that does not pass
> the compatibility test.  This is to be expected with the current behavior of
> validating not only the public version but any version that may be pending
> review.  Is it ok to send dupe emails like that?

This isn't a blocker but it would be nice if we could somehow put the version number in the email so they realize which version we bumped.

Comment 3

[Kumar McMillan [:kumar] (needinfo all the things)]

The first email log seems to be an anomaly.  The latest email logs still have duplicate emails but they are unique per version.  This will be fixed when we only validate the latest version (bug 658173)

Comment 4

[Kumar McMillan [:kumar] (needinfo all the things)]

(In reply to comment #2)
> This isn't a blocker but it would be nice if we could somehow put the
> version number in the email so they realize which version we bumped.

I added the addon version number to the email subject in https://github.com/jbalogh/zamboni/commit/f055302

Comment 5

[krupa raj[:krupa]]

Owners of platform-specific files will get emailed multiple times. The content of the email doesn't specify that the tests are run for individual files in a version. Is this something that we care about?

Comment 6

[Kumar McMillan [:kumar] (needinfo all the things)]

(In reply to comment #5)
> Owners of platform-specific files will get emailed multiple times.

This only happens for failure emails.  They get one email per file that failed validation.  Success emails are only sent one per version.

Comment 7

[Andy McKay]

https://github.com/jbalogh/zamboni/commit/b31b05ff1f7102b28117c6b6423239f98c636cab

Comment 8

[Kumar McMillan [:kumar] (needinfo all the things)]

bug 658288 (fix to compatibility type) also fixes this bug.

Comment 9

[krupa raj[:krupa]]

Created attachment 537832 [details]
screenshot

I still see duplicate email notification in run #18

LastPass Password Manager's owner is being emailed twice.

Comment 10

[krupa raj[:krupa]]

Also, the add-on "LastPass Password manager" doesn't have  a beta or newer version awaiting review. See https://addons-next.allizom.org/en-US/developers/addon/lastpass-password-manager/versions/

Comment 11

[Wil Clouser [:clouserw]]

(In reply to comment #9)
> Created attachment 537832 [details]
> screenshot
> 
> I still see duplicate email notification in run #18
> 
> LastPass Password Manager's owner is being emailed twice.

Looks like they are getting an email for each platform.  In theory there could be different results.  We should specify platform in the email if it's not ALL.

Comment 12

[Kumar McMillan [:kumar] (needinfo all the things)]

Fixed in https://github.com/jbalogh/zamboni/commit/14815e9b75c1274d24daeb969042cc0f85f36b72

The failure emails are indeed sent one per file (which might span platforms) so with this change the platform name is appended to the addon name.

Comment 13

[krupa raj[:krupa]]

verified. See post-fix screenshot

Comment 14

[krupa raj[:krupa]]

Created attachment 541408 [details]
post-fix screenshot