741679 - Send email to reviewers when manifest is updated

Status: RESOLVED WONTFIX

(Marketplace Graveyard :: Reviewer Tools, defect, P4)

Description

[Christopher Van Wiemeersch [:cvan]]

When we create a new version of an app -- that is, if we detect changes to the manifest during our cronjob run (bug 719234) -- we should send an email to reviewers who have checked "[x] Notify me the next time the manifest is updated."

Also, see bug 692170 for an ancient history lesson.

Comment 1

[Rob Hudson [:robhudson]]

Fwiw, the manifest can change quite a bit and we ping them each night. This could send the reviewers a lot of emails. We also have the re-review queue for when something important enough changes or the manifest URL can't be reached or there are validation errors.

Do we still want this?

Would we rather email the reviewers who clicked this checkbox if the app got thrown into the re-review queue (assuming other changes to the manifest are ok)?

Comment 2

[Andrew Williamson [:eviljeff]]

Personally, I'm unconvinced we need this feature, for the reasons you mention.

Comment 3

[Rob Hudson [:robhudson]]

(In reply to Andrew Williamson [:eviljeff] from comment #2)
> Personally, I'm unconvinced we need this feature, for the reasons you
> mention.

Do we want this feature for packaged apps? e.g. "Notify me when a new version is uploaded"? Does AMO have something like this for add-ons that is used?

If not, I'll wontfix this and file a bug to remove the "Notify me" checkbox from the form, then.

Thanks.

Comment 4

[Andrew Williamson [:eviljeff]]

(In reply to Rob Hudson [:robhudson] from comment #3)
> (In reply to Andrew Williamson [:eviljeff] from comment #2)
> > Personally, I'm unconvinced we need this feature, for the reasons you
> > mention.
> 
> Do we want this feature for packaged apps? e.g. "Notify me when a new
> version is uploaded"? Does AMO have something like this for add-ons that is
> used?

AMO has it, but I can't say how widely its used (it defaults to off).  I suppose it is useful occasionally so it would be good to have it implemented for packaged apps.  Its not a priority feature (for me anyway)

Comment 5

[Andrew Williamson [:eviljeff]]

Lisa, do we want this?
I'm not sure what it would add and why individual reviewers would want/need to follow updates to a particular app that closely.
I'd vote WONTFIX

Comment 6

[Lisa Brewster [:adora]]

1.  I use the "Notify me the next time the manifest is updated" feature, so plz2not take it away.  I wouldn't call it a priority, though.

2.  Whether it triggers on ANY manifest change or just enough to put it in the re-review queue:  what kind of change doesn't trigger re-review?  Re-review / update is probably what we want.

3.  My #1 use case for this feature is to get a notification when a rejected priority app gets resubmitted, so I can recheck right away.

Comment 7

[Andrew Williamson [:eviljeff]]

Can anyone clarify whats implemented now?  I thought the checkbox didn't do anything currently...

Comment 8

[Andrew Williamson [:eviljeff]]

Actually, it seems to me that what we want is to know when something we've rejected is resubmitted (which may or may not correspond to a manifest update).  Or maybe that's an additional use case.

Comment 9

[Rob Hudson [:robhudson]]

(In reply to Andrew Williamson [:eviljeff] from comment #7)
> Can anyone clarify whats implemented now?  I thought the checkbox didn't do
> anything currently...

I thought it didn't do anything either, but I just checked the code and it looks like it does what AMO does and sends an email when a new version is uploaded. I'm not seeing anything that sends emails based on manifest changes for hosted apps, however.

Comment 10

[Andrew Williamson [:eviljeff]]

(In reply to Rob Hudson [:robhudson] from comment #9)
> (In reply to Andrew Williamson [:eviljeff] from comment #7)
> > Can anyone clarify whats implemented now?  I thought the checkbox didn't do
> > anything currently...
> 
> I thought it didn't do anything either, but I just checked the code and it
> looks like it does what AMO does and sends an email when a new version is
> uploaded. I'm not seeing anything that sends emails based on manifest
> changes for hosted apps, however.

So the checkbox would work for packaged apps (which have 'versions') but not hosted apps?  

The 3 scenarios I think we have are:
a) packaged app new version uploaded
b) hosted app resubmitted
c) hosted app manifest change (a change worthy enough to flag for re-review?)

I guess we'd want the update to fire on a) and b) - not sure about c) though?

Thoughts Lisa?

Comment 11

[Lisa Brewster [:adora]]

Getting emails for all 3 scenarios in comment 10 would be swell.

Comment 12

[Rob Hudson [:robhudson]]

It looks like this never worked and was disabled 6 mos ago b/c it broke when someone clicked the "Notify me ..." checkbox:

https://github.com/mozilla/zamboni/commit/e8086bd628614b76e3e4dce2e64fea2a1f67b845

Just FYI if you're expecting an email.

Should we hide the checkbox until we fix this?

Comment 13

[Mathieu Pillard [:mat]]

Yeah, if we are not going to make it work, I vote we just hide the checkbox and keep the bug to implement it opened :)

Comment 14

[Wil Clouser [:clouserw]]

In the triage happening right now we concluded that this was such a rarely used feature that we'd rather not fix and maintain it and we should remove the checkbox instead.  Lisa: if you're distraught after reading this comment, let me know. :)

Comment 15

[Andrew Williamson [:eviljeff]]

The checkbox is hidden right now, but the underlying code is still there.  IMO we should strip it out and if subscriptions to updates are still needed feature then implement via commbadge notes instead where all the infrastructure to handle notifications, unsubscription, etc is already there.

Comment 16

[Ram Dayal Vaishnav [:ram]]

Hi Andrew,

Just to clarify, is this bug to remove the NotifyMe checkbox (https://github.com/mozilla/zamboni/blob/master/mkt/reviewers/forms.py#L125) and related code from codebase? 

Also setting the lang to py, correct me if I am wrong.

Thanks,
Ram

Comment 17

[Andrew Williamson [:eviljeff]]

remove the checkbox from the template and form, and any unused functions to do with editor subscriptions.  I forget how deep the code goes, but its legacy AMO code so insert usual 'here-be-dragons' warning.

Comment 18

[Andreas Wagner [:TheOne] [use NI]]

Ram, are you working on this?

Comment 19

[Ram Dayal Vaishnav [:ram]]

(In reply to Andreas Wagner [:TheOne] from comment #18)
> Ram, are you working on this?

No

Comment 20

[Vidhan Jain]

I would like to work on this bug but I need more information related to it.

Comment 21

[Andrew Williamson [:eviljeff]]

(In reply to Vidhan Jain from comment #20)
> I would like to work on this bug but I need more information related to it.

which part do you require more information on?  There is quite a lot of detail in the comments already.

Comment 22

[Ram Dayal Vaishnav [:ram]]

Hi Andrew, is this a good first bug? If no, then I wont recommend Vidhan to work on this bug.

Comment 23

[Andreas Wagner [:TheOne] [use NI]]

In the bug triage meeting we decided this is not a good first bug.

Comment 24

[Andrew Williamson [:eviljeff]]

https://github.com/mozilla/zamboni/pull/3229

Comment 25

[David Durst [:ddurst]]

The remaining piece of this was to move this functionality to commbadge.