544486 - Keep editor reviews for deleted/replaced add-on files and versions

Status: VERIFIED FIXED

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

Description

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

Editors and administrators have a very hard time trying to find historical information about certain add-ons, like previous reviews and files, because authors can completely remove them at will.

This means an author can easily remove all versions that have had a rejection from an editor, making his review history cleaner. It's also a problem when looking for versions that are known to be malicious or cause crashes, because they are usually deleted by the author.

We think that author removal should be equivalent to an admin disable. This way the files/versions won't appear in the author pages and they will remain accessible to admins.

Comment 1

[Justin Scott [:fligtar]]

I don't think I agree with this. The add-on and its data belongs to the author and if they want it deleted, we should honor their request. We can save historical metadata, but the files themselves should be deleted.

Comment 2

[mcdavis941 (sporadically reading bugmail)]

Is there any value in being able to go back and rerun current virus scans on older and deleted add-ons?  It might be nice to be able to do this if someone were to come forward in the future reporting an infected add-on ... to be able to say "no it's not infected and in fact we know that it never was".

Comment 3

[Justin Scott [:fligtar]]

As I understand it, our virus scanning scans all files as definitions are updated, so there isn't a need to explicitly re-run virus scanning.

Comment 4

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

(In reply to comment #3)
> As I understand it, our virus scanning scans all files as definitions are
> updated, so there isn't a need to explicitly re-run virus scanning.

There is if a user claims to be infected and the version happens to no longer exist on the site. We can't just assume that we always have perfect scanning, since that has been proven wrong before.

I would prefer to keep the files so we can do version comparisons and validations on any version, as well as preventing authors from uploading different files with the same version number over and over. If that can't be done, I guess keeping the revision history for deleted versions would be an improvement over not keeping anything.

Bug 559102 is a similar problem, but about user reviews.

Comment 5

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

I'll try to implement this pre-zamboni, since it's a common editor request.

Comment 6

[Andrew Williamson [:eviljeff]]

I hate to add to bugspam with nothing new to add but this bug is the bane of my life! (well, AMO work at least)

When the user deletes the version I've reviewed I have to guess what I previously rejected it for.  If I've not kept a copy of the previous xpi to compare to then the entire review has to start from scratch even though it may only be 1 line of code difference.

Of course, if another editor picks up the addon then they don't even know I rejected it previously (and visa versa), making giving a consistent message on what's required difficult.  This could be exploitable - hide some malicious code that's missed by the validator in your addon and keep deleting the rejected versions until it gets approved.  You only need the 10th editor to miss it to get it public (we're only human).

On a motivation point (an issue we discussed at the Summit) there is nothing more unmotivating than spending your time reviewing an addon and then the next day all traces of your work being wiped away!  

This bug is connected/duplicate: 359368

Comment 7

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

Updating title to reflect the current intended purpose. I still think that we should retain files, but let's focus on the less controversial objective.

Comment 9

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

Moving to 5.12.3

Comment 10

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

Moving to Q1 2011. This should be considered during the Editor Tools migration.

Comment 11

[Wil Clouser [:clouserw]]

How should these be revealed in the UI?

Comment 12

[Andrew Williamson [:eviljeff]]

(In reply to comment #11)
> How should these be revealed in the UI?

I don't think it needs any special UI so show them as a version without files would be displayed now? i.e. as normal but with an empty Files section.

Comment 13

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

What matters here is that the Item History in review pages remains unchanged after file or version deletions. It isn't critical for the UI to expose the fact that something was deleted. That's something we can discuss when doing the larger UI overhaul of the editor tools.

Comment 14

[Wil Clouser [:clouserw]]

(In reply to comment #13)
> What matters here is that the Item History in review pages remains unchanged
> after file or version deletions. It isn't critical for the UI to expose the
> fact that something was deleted. That's something we can discuss when doing the
> larger UI overhaul of the editor tools.

Sorry, it's still not clear to me what needs to happen in this bug.  The big "Item History" rewrite is still in planning - should this bug wait until then, or is there something that we need to do sooner?

Comment 15

[Kris Maglione [:kmag]]

This is by far the bug most complained about and most troublesome to editors. I think that if it's in any way feasible to simply keep a log of reviews even after the add-on version in question is deleted before the rewrite it should be done. Even if that meant temporarily not allowing authors to delete versions (but allowing them to delete the files therein), I think this should be addressed sooner rather than later.

Comment 16

[Wil Clouser [:clouserw]]

If I understand correctly, you want something like this on an add-on review request page (newest at the top):

2010-01-04 clouserw denied nomination saying "You're still doing $x and I can verify I already told you about it!"
2010-01-03 jorgev nominated some add-on v1.3 to become public
2010-01-03 jorgev uploaded some add-on v1.3
2010-01-03 jorgev deleted add-on v1.2
2010-01-02 clouserw denied nomination saying "You can't do $x"
2010-01-01 jorgev nominated some add-on v1.2 to become public
2010-01-01 jorgev uploaded some add-on v1.2

If so, I feel like we're talking about the big "Item History" table that needs to be planned out still (jorge is the mastermind behind that).

Comment 17

[Kris Maglione [:kmag]]

Something like the above might be the final goal, but it's not what I'm talking about.

Given, for instance, this review page: https://addons.mozilla.org/en-US/editors/review/404194?num=85

If the author deletes version 1.1 of his add-on, the delete seems to cascade and delete all reviews that reference it. That means that we can no longer see that version 1.1 ever existed, nor any review comments for it. Aside from being somewhat irritating, it means that since many authors delete versions after we've rejected them, we often have no idea what we should be looking for in an update (especially when a different editor is reviewing a file the second time).

It would be nice if, until the major item history rewrite, we could either stop reviews from being deleted along with the versions they reference, or otherwise prevent authors from deleting those versions (while nevertheless letting them delete the files therein).

Comment 18

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

(In reply to comment #14)
> (In reply to comment #13)
> > What matters here is that the Item History in review pages remains unchanged
> > after file or version deletions. It isn't critical for the UI to expose the
> > fact that something was deleted. That's something we can discuss when doing the
> > larger UI overhaul of the editor tools.
> 
> Sorry, it's still not clear to me what needs to happen in this bug.  The big
> "Item History" rewrite is still in planning - should this bug wait until then,
> or is there something that we need to do sooner?

This is *only* about preserving the review history entries that are currently visible in review pages, even if the versions or files are deleted.

Like Kris explained, it's probably a matter of avoiding the cascade deletion.

Comment 19

[Wil Clouser [:clouserw]]

Thanks, the example in comment 17 helped a lot.  Alright, so this bug is about the entries in the "Item History" section on an add-on review page.

We want those items to be preserved despite versions/files being deleted.  Kris/Jorge suspect cascading deletes.

Comment 20

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

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

Comment 21

[krupa raj[:krupa]]

verified @ https://addons.allizom.org/z/en-US/editors/review/400102?num=1

Comment 22

[krupa raj[:krupa]]

Attachment: post-fix screenshot