[Bug]: comment from 2nd level approval should show up in version history

[ioanarusiczki]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

from #15652 (comment)

STR:

1. A promoted addon is rejected/disabled by a reviewer.
2. From 2nd level approval queue it's canceled - a comment is added.
3. Checking the reviewer page the comment is available from version history.
4. Version is this time disabled or rejected again and there's a new decision in 2nd level approval queue.
5. Decision is again canceled + a comment added.

What did you expect to happen?

The comment is not displayed in version's history.

Other examples #15652 (comment)

With an abuse report attached the same scenario works as expected.

┆Issue is synchronized with this Jira Task

[eviljeff]

So, this is a weird scenario, from a chain of consequences:

• first off, the 2nd level approval comments are attached to the NeedsHumanReview activity log - no log means nowhere for the comments;
• but in this case there wasn't any activity log for the NHR, because there wasn't a new NHR created;
• this was because there was already a NHR - from the first time the 2nd level approval was cancelled;
• this should have been cleared when the add-on was disabled again
• but wasn't, because we have that type of NHR classified as ABUSE_OR_APPEAL_RELATED, meaning it's not automatically cleared (and can't be manually cleared either! confusingly)
• the original reasoning behind this restriction was that it was a NHR related to a Job, so it would be cleared when the job was closed.
• but since [Bug]: Comment from second-level approver is not shown in the reviewer tools when canceling and re-enqueuing #15652 this isn't the case for proactive decisions

Two possible fixes here I can think of:

1. relax the restriction on that type of NHR, so it would be cleared by a disable/reject (downside: if there was a job that wasn't resolved, it would stay open without any NHR to indicate the add-on still needed review)
2. split the NHR types, so the type for a proactive decision 2nd level approval cancellation is different from a non-proactive (i.e. abuse related) decision 2nd level approval cancellation (downside: more complexity - and more future bugs - as we're going further and further down the rabbit hole of special cases)

@wagnerand any thoughts? I'd lean towards 1.

[wagnerand-moz]

(1) makes sense in terms of pragmatism, but I am a bit concerned about everlasting invisible open jobs. We should avoid that. And worse, if we accept that risk, and come across that case, iiuc, we'd have no way to actually close the job via the reviewer tools.

As an alternative, rather than splitting the NHR type, could we split the logic handling it? I.e. only auto-clear the NHR if there are no open jobs (or if at least one of the open ones are not resolved by the reviewer in the same action)?

[eviljeff]

I am a bit concerned about everlasting invisible open jobs. We should avoid that. And worse, if we accept that risk, and come across that case, iiuc, we'd have no way to actually close the job via the reviewer tools.

The job(s) would still be open (visible) and closable via the normal reviewer tools actions - it's just the NHR that is cleared

[ioanarusiczki]

So looking in the original description what I noticed was that the comment made from decisions was not visible, I don't think I've noticed the NHR part.

Here's my results:

with reject action from reviewer's own initiative I rejected + re-enqueued the same version of a promoted addon 3 times and wfm as expected ✅
https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-listed/640283

But the same scenario with force-disable -> at the second force-disable action NHR is not cleared, and then if I try to make a comment to re-enqueue this won't be visible on the review page (the initial problem filed) ❌

https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-listed/640285

Then I tried all the str from pr (with force disable action) and indeed I see the NHR not being cleared (with clear hr action or force enable) if the appeal job is not checked ✅ -> not sure here what’s the following str for a reviewer to resolve it …

Repeating str with an abuse report attached and force disable - wfm as expected, I've force-disabled and re-enqueued 2 times , resolved developer appeal. https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-listed/640287 ✅

My conclusion is , following the pr testing str , what is not happening is nr. 5 Check that the NHR was cleared (automatically) by the force disable - because there is no open job with force-disable action (reject works as expected)

[eviljeff]

But the same scenario with force-disable -> at the second force-disable action NHR is not cleared, and then if I try to make a comment to re-enqueue this won't be visible on the review page (the initial problem filed) ❌

https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-listed/640285

I guess it's what @diox highlighted in mozilla/addons-server#24316 (comment) - force disable action in the reviewer tools doesn't actually clear NHR from the versions. I assume in normal operations someone would clear the NHRs manually to get the add-on out of the queue... but should they be cleared with a force disable automatically? mmm, any thoughts on this @wagnerand-moz - it's easy enough to change, but was it designed that way on purpose?