1022895 - Rejected themes are sometimes showing up in the Updates queue

Status: RESOLVED INVALID

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P2)

Description

[Amy Tsay [:amyt]]

Sometimes, rejected themes show up in the Updates queue. Here is an example: https://addons.mozilla.org/editors/themes/queue/single/verde-hay-azul. Not sure what is causing this. If you go to the main Updates queue: https://addons.mozilla.org/editors/themes/queue/updates, you don't see verde-hay-azul in the list.

cc'ing candelora, who reported the problem and may have details to add.

Comment 1

[Amy Tsay [:amyt]]

Attachment: Screenshot 2014-06-09 13.45.08.png

Comment 2

[Amy Tsay [:amyt]]

Attachment: Another example - 6/23/14

Comment 3

[David Larlet [:davidbgk]]

I need some more info on your process to fix that bug.

First, which URL do you hit that contains the rejected theme? From your second screenshot it looks to be /editors/themes/queue/updates but your first comment states that you don't see the addon at that URL so I'm confused.

Second, to better understand your workflow which URLs should contain rejected themes and which ones shoudln't?

Comment 4

[Amy Tsay [:amyt]]

You could see rejected themes in the log: https://addons.mozilla.org/editors/themes/logs. You should be able to filter by "reject", but that doesn't seem to be working. For this bug, we could tell https://addons.mozilla.org/editors/themes/queue/single/verde-hay-azul was rejected by the Review History on the bottom of the page.

There are 3 queues: Pending, Flagged, and Updates. None of those queues should include rejected themes.

Does this help?

Comment 5

[David Larlet [:davidbgk]]

Amy, thanks for your answer. I still don't get if the display of the Reject line in the theme log at the bottom of your first attachment is valid though https://bug1022895.bugzilla.mozilla.org/attachment.cgi?id=8437199

Should we keep it or remove it from that page too?

The case with Pending/Flagged/Updates tabs of the second attachment should be fixed in this pull-request: https://github.com/mozilla/olympia/pull/169

Comment 6

[Amy Tsay [:amyt]]

Hi David, the "reject" message in the log should remain, because it was rejected. The theme just shouldn't be listed in the Update queue. Does this help?

Comment 7

[David Larlet [:davidbgk]]

Thanks Amy, merged in https://github.com/mozilla/olympia/commit/cdd535fa695409d99886c2e7ecbc5ff052269b5c

You should soon be able to test it on -dev.

Comment 8

[Madalin Cotetiu]

Verified as fixed.
The rejected themes are no longer displayed in the update queue.

Comment 9

[Amy Tsay [:amyt]]

This is happening again: http://i.imgur.com/960ebcs.png

Comment 10

[David Larlet [:davidbgk]]

Amy: do you confirm that this theme is no longer in that state in production? Do you have another example in order to check his rereview/locks status?

I have hard time reproducing it locally and I don't want to fix half of the issue again.

Comment 11

[Amy Tsay [:amyt]]

I still see them in the update queue, yes: http://i.imgur.com/lw4zL0k.png. Do you need the links to them? Here are some:

https://addons.mozilla.org/editors/themes/queue/single/tann3
https://addons.mozilla.org/editors/themes/queue/single/verde-hay-azul
https://addons.mozilla.org/editors/themes/queue/single/zedbaziii

Does this help?

Comment 12

[David Larlet [:davidbgk]]

Amy, can you give me the URL of your screenshot? I guess that's the rereview one but I prefer to be sure.

Comment 13

[Amy Tsay [:amyt]]

Do you mean the Updates queue? https://addons.mozilla.org/editors/themes/updates

Comment 14

[David Larlet [:davidbgk]]

https://github.com/mozilla/olympia/commit/d94f2c93cadbab4f6bc92f5dac32eae5435f3437

Comment 15

[Amy Tsay [:amyt]]

Was this fix pushed? If so, does it apply to future instances? I ask bc there are still rejected themes in the Updates queue.

Comment 16

[David Larlet [:davidbgk]]

Yes, that fix was pushed. It's hard to fix because I wasn't able to reproduce it, I hope my successor will :)

Comment 17

[Mathieu Agopian [:magopian]]

Ok, I believe that I managed to reproduce the issue:
1/ upload a new theme
2/ reject it
3/ modify the theme by clicking on the "upload new design" link above the header image
4/ the theme will now appear in the "updates" queue

Amy: can you confirm that this is the issue you're seeing? I'll attach a screenshot.

If that is the issue you're seeing, then I'm pretty sure this is by design, and expected: the theme is updated (with a new design), so it should be re-reviewed, right? And thus, should be displayed in the "updates queue"?

Comment 18

[Mathieu Agopian [:magopian]]

Attachment: Rejected theme displayed in the updates queue once a new design is uploaded

Comment 19

[Amy Tsay [:amyt]]

Ah, I see. Yes that would make sense. The Rejected label is misleading, but it tells us that the theme was previously rejected and has now been updated?

Comment 20

[Mathieu Agopian [:magopian]]

I believe that's the point yes.

On the other hand, I believe :mat (Mathieu Pillard) told me that on zamboni, if a theme is rejected, then it shouldn't go through the "updates" queue if it's modified, but rather go through the "pending" queue again, as if it was new.
The rationale is that the updates queue should be reserved to themes that have already been approved: it's therefore easier to approve it again, on the grounds that it should be trustworthy. On the contrary, if it's been rejected already, then maybe it's not trustworthy, and should go through the full scrutiny again.

I don't know what's the correct way, nor if the current behavior is expected.

If you feel it should be otherwise, please open a new bug so we can dig into it.

Comment 21

[Amy Tsay [:amyt]]

Thanks for looking into it! The way it is currently isn't ideal, but will suffice.