700338 - Deleting a reviewed version of an add-on can lead to listing becoming unavailable due to status conflict

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect, P2)

Description

[Ioan C.]

Attachment: after each of this pages i get "we're sorry" page - tried after 3 hours and same result

User is logged in - has one add-on submitted.
1. Submit an add-on which has max compatibility < 7.0.1
2. View in My Addons.
3. Click on Compatibility link and submit another (higher) version which has max_ver = 9.*
4. Check in My Addons (red alert is now gone).
5. From Manage Versions, remove the newest version.
6. Go to Addons Details Page.

Actual Result: Page (https://addons-dev.allizom.org/en-US/firefox/addon/test-mohawk-_-05/) doesn't load / "We're sorry, but we can't find what you're looking for." Tried 3 hours later, got same result.

Expected: Page should load and the old version should be displayed.

Comment 1

[Ioan C.]

Update - today, the page its still unreachable. Note: previous version should still be available.

Comment 2

[Rob Hudson [:robhudson]]

The addons-dev database was refreshed between now and when this was filed. Could you reconfirm the bug and upload a new test case to look at?

Thanks.

Comment 3

[krupa raj[:krupa]]

Cristian, can you try to reproduce?

Comment 4

[Cristian Boldan]

I was able to reproduce this issue on AMO-dev.
Please see the attached screencast http://screencast.com/t/dAR7QJV2AX

Comment 5

[Rob Hudson [:robhudson]]

Adding URL:
https://addons-dev.allizom.org/en-US/developers/addon/hihihohomymy/versions

I believe what's happening is that when the 2nd version is added and the prior version hasn't been approved it marks it as disabled. Then when the 2nd version is deleted the add-on no longer has any valid versions and thus "current_version" is set to NULL. When the "View Listing" page is loaded there are no valid versions so it 404s.

The code is here:
https://github.com/mozilla/zamboni/blob/master/apps/addons/views.py#L91

I don't know what the correct action would be in this case. A 404 for no valid versions seems reasonable to me, but AMO flagging a prior version as disabled when a new version was added, then that new version being deleted leaving it as disabled seems like the issue.

Comment 6

[Andrew Williamson [:eviljeff]]

As you suggested on irc yesterday, we could do the same as bug 800086 and correct the status.  

The difference would be we need to handle prelim and full, statuses.  So if the highest status of the remaining versions is preliminary then make the add-on preliminary reviewed; if there are no approved versions remaining then make the add-on incomplete. 

The only edge case I can think of is when the add-on is under nomination with one "awaiting preliminary review" version, and one "preliminary approved version", and the preliminary approved version is deleted. So its "Preliminary Reviewed and Awaiting Full Review" which needs to change to "Awaiting Full Review".

Comment 8

[Jorge Villalobos [:jorgev] (he/him)]

Rewording this bug to make it easier to find. Also, setting some priorities since we run into this very often and it's likely it's causing developers to think they are awaiting review when then aren't, and then silently leaving.

Comment 9

[Yohan Boniface [:ybon]]

So here is a little update on this.

First, what I've understood.
1. When a new version of an addon waiting for review is uploaded, the "old" version's files are set to DISABLED.
Here is the code chain:
devhub.views.version_add
    => Version.from_upload
        => v.disable_old_files
            => f.update(status=amo.STATUS_DISABLED)

2. When a version is deleted, if no "valid" version remain, the addon._current_version property will be cleared (note that this occurs also when a version is manually disabled).
Here is the code chain:
devhub.views.version_delete
    => version.delete()
        => update_status (signal on version post_save)
            => addon.update_version()
                => current = addon.get_version() => None
                => self._current_version = current (None)

3. If an addon has no "current_version" (among other tests), the addon details page will be in 404.

So, as far as I understand the previous comments, what we want to do is, roughly, when the user delete a version and no enable version remain, to "promote" an "old" version that has been automatically disabled.

Here are some remarks about that:

- the issue is occurring also when the current version is disabled by the user; so this means that we need to keep in mind to handle the case when this version would be reenabled

- when a user disable by hand a version, the linked filed will be set as STATUS_DISABLED, just like when a version is automatically disabled; which will make hard to distinguish both scenarios; and we can imagine automatically promote a version that has been previously automatically disabled, but I don't think we want to automatically reenable a version that has been disabled by hand. But maybe I'm missing another property to distinguish both situations

- I don't master all the possible scenarios: for example, what if the addon changes review process in between the moment where a new version is uploaded and the moment this new version is deleted?

In brief, I'm not sure I'm able to guess the correct status to set to a version that we would reenabled automatically, and I'm not even sure I'm able to sort out cases were we can "undo" the automatic disabling.

So, at this point in time, I wonder if this is the way to go. Clearly, in theory, reenabling a version previously automatically disabled seems the clean way. But I'm not sure it's doable in a safe manner.

So, in a parallel thread, I'm also looking for other workarounds, like not going in 404 when there is no current_version but there are some old ones, and maybe putting up a message that make clear the current situation, so that the developer can manually fix it the best way. That's just an idea on the fly.

Thanks in advance for your comments and suggestions!

Comment 10

[Wil Clouser [:clouserw]]

I would be happy with a clear message about what is going on and options if the add-on owner is visiting the detail page.  404s for non-auth'd and non-owners is fine with me still.

I think any other logic changes is likely to be chasing our tails, but will see if kris or jorge have other ideas.

Comment 11

[Kris Maglione [:kmag]]

I think the easiest thing to do is to offer to re-enable the newest disabled version without any review/user disable log entries, but not to re-enable it automatically, when the user disables the only publicly visible version.

It would also be nice not to show a 404 for such add-ons, yes.

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

Users can't disable versions, can they? They can only delete them.

Comment 13

[Kris Maglione [:kmag]]

When they try to delete a version, they're given the option to disable it instead.

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

My suggestion is that we don't bother automatically re-enabling versions. What matters most to me is that the add-on status matches that of the files that are left enabled after the disabling or deletion. Add-ons with no reviewed files should have an Incomplete status, the rest should have Full Review if there is a fully-reviewed file, and Preliminarily Review otherwise.

When the file being deleted or disabled is up for review, we can just show a warning explaining that a new version needs to be submitted. I think that's less error-prone than trying to automatically enable previous versions.

Comment 15

[Kris Maglione [:kmag]]

I think that sounds reasonable.

Comment 16

[Yohan Boniface [:ybon]]

Thanks Jorge and Kris for your feedback.
I will work on this, that is: when the last version of an addon is deleted, retrieve the more recent of its validated files, and, if any, update the addon status accordingly, otherwise set it to incomplete.

Comment 17

[Yohan Boniface [:ybon]]

So after hours of discussions and code inspection, many versions coded and recoded, I've ended with this PR: https://github.com/mozilla/olympia/pull/71
Basically, I'm on the suggestion of Jorge:
- ensure that the addon version is set to NULL if no version with valid file exists
- add a message to warn user when deleting a version which is the addon current version.

Thanks for the review and feedback, (and please suggest a better message for the user: https://github.com/mozilla/olympia/pull/71/files#diff-a47361d62e6de680908d758a4e4ffaddR187 :) )

Comment 18

[Yohan Boniface [:ybon]]

Kris or Jorge, can you suggest a better error message here: https://github.com/mozilla/olympia/pull/71/files#diff-a47361d62e6de680908d758a4e4ffaddR187 ?
Then I can merge it and close here.

Thanks in advance! :)

Comment 19

[Jorge Villalobos [:jorgev] (he/him)]

I like Kris's version.

Comment 20

[Yohan Boniface [:ybon]]

Thanks!
Merged: https://github.com/yohanboniface/olympia/commit/a910407fdb5d1cf877a7b0b13e405d12422100d4

Comment 21

[Iulian Timis]

Verified as fixed in https://addons-dev.allizom.org/en-US/developers/ on FF29 (Win 7).
Postfix screencast http://screencast.com/t/utBuuyRi4
Closing bug.