Closed Bug 717495 Opened 13 years ago Closed 1 month ago

Uploading a new version of an add-on pending review should not reset its queue position

Categories

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

Product:
addons.mozilla.org Graveyard
Component:
Admin/Editor Tools
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE
Milestone:
2015-01

People

(Reporter: jorgev, Unassigned, NeedInfo)

References

Details

(Whiteboard: [ReviewTeam:P1][See comment 15]) 

This is a very common complaint, and bug 656122 has a previous discussion about this. Greg suggested I file a new bug to start fresh with what our current needs are.

Here's what we need:

1) Set an upload timestamp when a version is added to the queue (any queue). I believe every version row has a nomination date column, so that could be used. Otherwise, use a new one.
2) If a new (non-beta) version is uploaded and the latest version is pending review, the new version should inherit that upload timestamp so that its position in the queue is preserved.
3) If a new file is uploaded to a version that already has approved files, the upload timestamp should be reset. This is meant to preserve what we wanted fixed in bug 654096.

The queues should be sorted by this upload timestamp, clearly.
Blocks: 689635
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam:P1]
Target Milestone: Q1 2012 → Q2 2012
Hello guys,

There's no way to stress the importance of this. It could substantially lower the queue load. It's almost always the case that when an approval happens, so many bugs have been fixed during the approval period to warrant another release. And devs won't risk wasting 2 weeks of wait and submit a new version during the waiting period. 

This pattern would repeat on every version submitted creating a semi-endless cycle of submissions of the same extension. Wasted effort for editors who would be reviewing something that will be replaced in a day or two after the review is done (so why not review the last submission instead?)

Haven't done this on a calculator, but a decrease of 25-50% of queue numbers is quite possible if this is implemented.
Any progress yet? this bug is lingering around for exactly 1 year and 9 days now. I just noticed how frustrating it is to be bounced back to the top of the queue when uploading a new version for a 2nd preliminary review. Even if I change one character I have to wait weeks. If you need some help, let me know.
Bumping this. It's been on the issue queue forever now.
Correct me if I'm wrong, but I think the solution is quite simple no matter how complex the system is. Once one uploads a new version it currently gets a new timestamp and another status that indicates it is either in preliminary review or full review. So I guess the solution could be to _NOT_ update the timestamp _IF_ it is in preliminary review or full review. If it does not have a preliminary review or full review status, then update the timestamp to a new one, and thereby shift it back to the queue. This way developers have the option to make last changes before a couple of days it will be reviewed. (give or take 5 days, depending how long the review takes which implicates a lock)
How I have had the problem explained is the really high level of abuse this adds. In order to beat the queue time they will always have a version submitted.

Intention: So you submit your addon, add some changes and resub, it keeps position and this is great.

Gaming the system: So you submit your addon it gets accepted, you don't want to have to wait the full queue once your are done with your next set of changes, so you submit a 'new' version with something minimal changed and then submit all the changes when you are done so you can beat the wait.

Implementation would be less simple but I'd propose that each new version gets a spot in the queue, if they are all legit versions then reviewed all at once, if they are not, the addon/author gets the grouping flag removed and they can only have one in the queue per addon at a time.

Also another problem is the tools are geared around comparing the new version with the old version, instead of seeing a simple old and new, they would see old, new, newer, newest, or it would have to try to collapse the changes into one change. Anyone who has done a few rebases in git will know how hard this is. Or finally it would fully replace the intermediary versions that were uploaded. I'm not sure which is the most right.

Also predicted change is queue size is likely an illusion. An active developer is an active developer. If they are changing things enough to need to submit multiple times in a given review cycle they are probably active enough to be submitting fairly often. If anything I see this raising queue size because it would lower frustration from the addon developers side (as a person who has written an addon and maintains it I know it would make a difference to me)

These are just my thoughts, if the following are thought about by someone who understands things a bit better than me, then I can take this on as a side project.
Hey Wraithan,

We can try to foretell the effect of this system for a decade and won't know the real results unless we try. How hard is it to revert this change if it proves bad?

As for your scenario for gaming the system, if a developer is quite sure enough he can beat the review time, so what if he goes for the scenario you propose? And if he fails to finish his new release in time, he will loose his spot anyway. Not to mention the review would be a breeze for reviewers since a diff will show trivial changes. If he gets his release in time, great for him. In either case, it won't be much of gaming the system. I could easily call it using the system to enforce a hard deadline which may in itself be a good thing.
I'm not opposed to it, I just want it thought about by people who are more familiar with the system than I am. Reverts of features tend to go over poorly with community, more poorly than having not released it in the first place. This is my own anecdotal experience, I have no numbers to back this up.

My speculation regarding queue size was mostly to address those who claim this will lower queue size. Raising/lowering queues size is the least important reason to implement this because it is pure speculation. The reason this should go in, in some form, is enhanced user experience. In this case the user is the add-on developer.

I am new to hacking on the system and new(-ish) to writing add-ons. I think questioning things a bit before implementing them is a good thing. I'm going to wait on someone who knows more to weigh in on this.
I can't see how one can game the system if a lock is added when a reviewers starts to review it. See my previous post where I propose a 5 day lock -or better yet a queue lock- meaning, that once the queue hits let's say the 10th position, a lock is added so no more changes can be made. This way there is no way anyone can game the review process nor can collide with it. Sitting on the 10th position is more comfortable than be bounced back in the queue.
(In reply to Sasha (retired) from comment #10)
> I can't see how one can game the system if a lock is added when a reviewers
> starts to review it. See my previous post where I propose a 5 day lock -or
> better yet a queue lock- meaning, that once the queue hits let's say the
> 10th position, a lock is added so no more changes can be made. This way
> there is no way anyone can game the review process nor can collide with it.
> Sitting on the 10th position is more comfortable than be bounced back in the
> queue.

A queue position 'lock' would be very arbitrary - sometimes the top 10 addons in the queue may be awaiting outstanding requests, or be flagged for super review.  If the 10th position is set too high (25? 50?) then its the same issue situation as now.  The complaint, as I understand it, is that addons that have been waiting for a while/are near the top of queue can't be updated as they will loose their position.  A time or position 'lock' would affect exactly these addons.
I prefer the KISS principle and simply allowing updates anytime before an add-on is approved. Chances of an upload while the reviewer is reviewing it are pretty low I'd say. Implementation wise, this would be the simplest I presume. 

Another possibility is a lock when reviewer starts reviewing it regardless of position to avoid that edge case altogether (status becomes being-reviewed?). It's reasonable to say no review would start before position 20 and this may be the thing needed to further discourage users from gaming the system (not that I think any gaming-the-system scenarios are valid anyway).
Target Milestone: Q2 2012 → ---
(In reply to ominds from comment #12)
> I prefer the KISS principle and simply allowing updates anytime before an
> add-on is approved. 
excluding during review. We do not want unreviewed code sneaked in last minute, need to avoid that at any price (that's on of the reasons why there is a queue reset at the moment).

> Another possibility is a lock when reviewer starts reviewing it regardless
> of position to avoid that edge case altogether (status becomes
> being-reviewed?).

+1

> It's reasonable to say no review would start before
> position 20 and this may be the thing needed to further discourage users
> from gaming the system 
Even if we review something further down in the queue, the assumption is always that anything that is submitted for review is a complete patch, so I don't think this would be a problem. Typical use case for a patch-while-submitted would be a last minute bugfix and for this it would be very valuable, both to AMO-Editors and Addon authors. Also, I would imagine it would be possible & desirable to sneak in a patch that reacts to a "question to author" - if I ask a question then the review-lock should be reset (don't know whether this is actually the case right now).
(In reply to Axel Grude from comment #13)
> (In reply to ominds from comment #12)
> > I prefer the KISS principle and simply allowing updates anytime before an
> > add-on is approved. 
> excluding during review. We do not want unreviewed code sneaked in last
> minute, need to avoid that at any price (that's on of the reasons why there
> is a queue reset at the moment).

This can already happen in edge cases as once the page has loaded you wouldn't be informed it has changed position or the latest version had changed.

> > Another possibility is a lock when reviewer starts reviewing it regardless
> > of position to avoid that edge case altogether (status becomes
> > being-reviewed?).
> 
> +1

I'd be for that - it should be a separate bug though if someone wants to log it (as it can already happen, and isn't just limited to the update queue).
Assignee: nobody → jesper
Been looking into this at code and have found the culpit:
https://github.com/mozilla/zamboni/blob/master/apps/editors/models.py#L248

It would seem there might be two possible solutions to this:
1. Add a new column that dates nomination time for non-fullreview
2. Reuse the versions.nomination (table.column) a few lines above as the full review queue uses.

#1 requires an extra amount of work to do
#2 It seems that the versions.nomination field is not present in other places than (excluding tests): 
* apps/addons/models.py - sets the nomination date when user asks for review.
* apps/editors/models.py - query function to get full review nominations ordered by versions.nomination.
* apps/versions/models.py - function that ensures the nomination date is inherited from the last nominated version.

Does #2 seem like a good solution?
Flags: needinfo?
If #2(In reply to Jesper Hansen from comment #15)
> Been looking into this at code and have found the culpit:
> https://github.com/mozilla/zamboni/blob/master/apps/editors/models.py#L248
> 
> It would seem there might be two possible solutions to this:
> 1. Add a new column that dates nomination time for non-fullreview
> 2. Reuse the versions.nomination (table.column) a few lines above as the
> full review queue uses.

As long as #2 doesn't cause any other problems it seems like a clean solution.
Flags: needinfo?
Probably worth noting that the full review wait times have been crazy and broken lately, so we probably don't want to use them as an example. We reject a version, a new one gets submitted, and it winds up with the wait time based on when the rejected version was submitted, and other such strange behavior.
(In reply to Kris Maglione [:kmag] from comment #17)
Would probably keep that in a different bug report. Not sure if there is one. If it is a problem then create a new bug and CC me onto it
Jesper: I'm removing you as the assignee for this bug since it's so old.  Let me know if you're still actively working on it.
Assignee: jesper → nobody
Whiteboard: [ReviewTeam:P1] → [ReviewTeam:P1][See comment 15]
Target Milestone: --- → 2014-04
Assignee: nobody → yboniface
Quick update on this issue.
We pair programmed on this all day with David [:davidbgk]. Both of us being noobs of olympia and the review workflow of AMO, plus the code being quite complex and full of history, this has been a nice sensitive archaeological day. :)
The grand part of what I'm going to expose here is well known by most of you, but I also write it to allow you to correct us if we are wrong on our understanding or to add some warning on point we missed. And also for future me and future David when our memories will fail. ;)

Here is the *work in progress* pull request: https://github.com/mozilla/olympia/pull/38

We have dug into the #2 option expressed in Comment 15, which consists in converging the use of `addon.version.nomination` as ordering key for all the queues (not only the "full review" one).

The `addon.version.nomination` value is originally set on the `watch_status` signal, executed on Addon post_save. The code was only executed when the status of the addon was one of STATUS_NOMINATED and STATUS_LITE_AND_NOMINATED, in other words only when a addon joined the full review process. We need now to execute this each time a addon enter a validation process.

Then, another post_save signal, `inherit_nomination`, is called. The goal of this signal is that, when creating a new version of an addon while being in a review process, the nomination value from the previous version is reused, in order not to lose the queue position.

So what we are trying to achieve (with as less changes as possible) is to create a new nomination value at first entry in the validation process queue, and then keep this value all the review process long, even when adding new versions OR changing review process (for example from a preliminary review asking for a full review while still waiting for the initial review).

The most delicate point at this time is that most of the decisions are taken according to the addon status, but the addon can have *in the same time* a published version AND a new version waiting for validation. So certainly in a better world we should have made the status an attribute of the version. But that's not the debate.

Next steps are to first look all the broken unit tests, understand why they are broken, fix them or fix our patch depending on what's the problem. Then create a new bunch of tests for all the scenarios we are taking into account (ongoing list at https://amo.etherpad.mozilla.org/1). We also have in mind that we will need to be backward compatible with production data (certainly having a COALESCE or something like this in the queue select to keep file.created as fallback)

David, feel free to amend if I'm missing something :)
(In reply to ybon from comment #22)
> The most delicate point at this time is that most of the decisions are taken
> according to the addon status, but the addon can have *in the same time* a
> published version AND a new version waiting for validation. So certainly in
> a better world we should have made the status an attribute of the version.

This is incorrect, or maybe I'm confused by the wording. Add-ons have a review status, but also the individual files have a review status (add-ons have versions, versions have files). This is especially important for updates, since the add-on status will be fully reviewed or preliminarily reviewed, while the new file has a status of pending review.

Having a date value that remains the same during the review process, until the version is approved, preliminarily approved or rejected, sounds good to me.
Thanks Jorge for your useful answer.
My point about `addon.status` was a little bit of a shortcut, and quite presumptuous given my poor knowledge of the code, but what I meant is that at this time the code is using the `addon.status` to make decisions about `addon.version.nomination` value. So for example, when you have an addon already published in LITE mode, when you upload a new version, the `addon.status` is unchanged, so *at this time* the pieces of code that are updating the nomination value doesn't know that a new validation process has been started. AFAIK.
But you pointed me to an interesting information: files do have a status too.
Let's work on this direction :)
Just a quick update: the PR is ready for review, and being reviewed, so anyone that is interested by this issue, please have look and give feedback :)

https://github.com/mozilla/olympia/pull/38
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Question, is this bug the reason why I no longer see the queue position of my add-ons waiting for review in the dev pages? Is that expected? I can theorize that since the queues are built/read differently, then technically my add-ons "aren't" in the queue, because the last versions awaiting review were uploaded before this patch was integrated, so they were never actually entered into these "new" queues.

And if so, I hope this won't impact negatively on the review process! For instance, if I can't see their position in the queue, I can also theorize that the reviewer can't see them in the actual queue either. I hope they are still visible to reviewers, so they will still be reviewed(?)

(Or is this completely unrelated, and the queues position indicators were removed on purpose somewhere else?)
Flags: needinfo?(yboniface)
(In reply to Luís Miguel [:Quicksaver] from comment #26)
> Question, is this bug the reason why I no longer see the queue position of
> my add-ons waiting for review in the dev pages?

No, I just filed bug 1007972.

> I can also theorize
> that the reviewer can't see them in the actual queue either. I hope they are
> still visible to reviewers, so they will still be reviewed(?)

The queues are working fine, so it isn't a problem on the review side.
(In reply to Jorge Villalobos [:jorgev] from comment #27)
> (In reply to Luís Miguel [:Quicksaver] from comment #26)
> > Question, is this bug the reason why I no longer see the queue position of
> > my add-ons waiting for review in the dev pages?
> 
> No, I just filed bug 1007972.

Actually, I don't know if this bug is related to the regression. But it certainly wasn't intended for us.
(In reply to Jorge Villalobos [:jorgev] from comment #28)
> (In reply to Jorge Villalobos [:jorgev] from comment #27)
> > (In reply to Luís Miguel [:Quicksaver] from comment #26)
> > > Question, is this bug the reason why I no longer see the queue position of
> > > my add-ons waiting for review in the dev pages?
> > 
> > No, I just filed bug 1007972.
> 
> Actually, I don't know if this bug is related to the regression. But it
> certainly wasn't intended for us.

Yes, that's what I thought, I know it wasn't the intention (at least of this patch) to remove the queue position indicators, but it is in the same area and it is recent enough, so it could have been an unwanted side-effect.

(In reply to Jorge Villalobos [:jorgev] from comment #27)
> > I can also theorize
> > that the reviewer can't see them in the actual queue either. I hope they are
> > still visible to reviewers, so they will still be reviewed(?)
> 
> The queues are working fine, so it isn't a problem on the review side.

Was just about to post about this. One of my add-ons was just reviewed, so I guess the queues themselves are still working, so hopefully the add-ons pending reviews are still known to reviewers.
Flags: needinfo?(yboniface)
I just confirmed that this bug isn't fixed yet. Uploading a new version still resets the queue position.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 2014-04 → 2014-11
Assignee: yboniface → nobody
Jorge, does it happen only for recent (less that 6 months old) add-ons or even older ones? Can I have an add-on that has currently the issue in production to check its statuses?
Assignee: nobody → david
Flags: needinfo?(jorge)
Here's one: https://addons.mozilla.org/addon/dontphishme/
It shows up in the queue as waiting for two days, but there's a previous unreviewed version that is 3 weeks old.
Flags: needinfo?(jorge) → needinfo?(david)
Hopefully fixed in https://github.com/mozilla/olympia/commit/c76af17e8313e787db2430901596c1a77b80c672
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(david)
Resolution: --- → FIXED
If I understand correctly, I have this bug was not corrected.
 		
Was version 0.7. After addition 0.7.0.1 place in queue has been reset (55 place to 64 place).
Screenshot https://yadi.sk/i/4-BrMgIHcijvN
I may be wrong.
in olympia / apps / addons / models.py
def watch_status

elif (new_status in amo.REVIEWED_STATUSES
and instance.latest_version.has_files):
   if not inherit_nomination(None, instance.latest_version):

The only single place call for inherit_nomination
But only if new_status is Reviewed

may need to be
elif (new_status in amo.UNDER_REVIEW_STATUSES)

Sorry if offered stupidity
Flags: needinfo?(david)
Thanks for pointing that out! I'm preparing a new pull-request to change and test that.
Status: RESOLVED → REOPENED
Flags: needinfo?(david)
Resolution: FIXED → ---
https://github.com/mozilla/olympia/commit/b2b0d88e201ffe82b6c41c4aa33cb47dfbcdf281
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: 2014-11 → 2015-01
I have uploaded a new version of the add-ons just now. Position in the queue was 3, and have now 17 of 17.

If I understand correctly, it is precisely this defect.
But then, it's not fixed or again broken.
Flags: needinfo?(jorge)
Status: RESOLVED → REOPENED
Flags: needinfo?(jorge)
Resolution: FIXED → ---
There must be some conditions necessary to reproduce this bug, as I've seen it fixed at least in some cases.

Could you please let me know the name/slug of your addon, and the version you've uploaded?

Also, could you please describe exactly what you did, in what order, to reproduce the bug?

Thanks!
Assignee: david → nobody
https://addons.mozilla.org/ru/firefox/addon/http-useragent-cleaner/
Version 0.7.0.11

1. Access to https://addons.mozilla.org/ru/developers/addon/http-useragent-cleaner/edit
2. Access https://addons.mozilla.org/ru/developers/addon/http-useragent-cleaner/versions 
3. Load new version file
4. Check messages of full report
5. Checkbox "All platforms" already checked
6. Submit button click
7. Check queue position (reseted)
8. Add beta-version of extension similarly (0.7.0.11rc)
Thanks a lot for these detailed steps. Could you please tell what was the status of the add-on before that, was it fully reviewed? Did you have beta versions already? What happened when you uploaded the beta version 0.7.0.11rc?

When you say the queue position got reset, what was the version that was in the queue before that? Was it a beta version?

Could it be that the queue is reset only if the version in the queue is a beta version, and then you upload a new (non-beta) version, and its position in the queue is reset?
Beta-version has been.
Lastest versions also suffered with this bug (disabled after new load).
Before several versions was rejected by the editor.

Beta-version is not in review queue. Never. In queue has been release version 0.7.0.7
However, in the version list, beta version could be higher than the release version.

When I upload beta version nothing terrible happened (release version stay on in queue: all well, another bug is fixed truly).
I experienced the same for my unlisted add-on. As my frequency of release is currently higher than the frequency of getting reviewed, the current algorithm means I effectively can't release new versions once signing becomes mandatory, as my extension includes 3rd-party code (jsencrypt) that triggers manual review.
This is a concern for us as well. We have unlisted submissions in the queue. At the same time, we're working to address signing severity warnings to reduce manual review times or bypass it all together. We won't upload new versions until queue position remains unchanged when that action is taken.
Update: We have successfully updated several add-ons in the unlisted queue without changing queue position. Did something change on AMO to address this issue?
Nothing since the fix in comment 37. It does work for some cases, and fail for others. Not being able to reproduce makes it hard to fix.
If you have a AMO test version online, I can try to repeat her behavior. However, the attempt to install AMO to the local computer have failed.
Thanks Mathieu. If we get a failure, I'll copy the extension id here to help review logs, etc. Let me know if there's anything else we can provide.
fsdc: we have http://addons-dev.allizom.org/ for a dev version of AMO. There's also http://addons.allizom.org/ (staging server, which has data close to what we have on production, but less recent).

If you have issues running AMO locally, please let me know on irc (#amo), I'll be happy to help. Also, have you seen the "recent" docker install http://olympia.readthedocs.org/en/latest/#quickstart ?

ChrisL: extension id, steps you made and time you had the issue would help for sure, thanks!
Any progress on this or has it been postponed/forgotten for now?
Product: addons.mozilla.org → addons.mozilla.org Graveyard
is this part of redesign considerations
Flags: needinfo?(adora)
Status: REOPENED → RESOLVED
Closed: 10 years ago → 1 month ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.