search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

[Bug]: Version with developer replies has Needs Human Review flag set, but no due date #14888

Closed abhn closed 1 month ago

abhn commented 1 month ago

What happened?

For add-on 2774123

Additional (potentially irrelevant details)

Developer submitted 0.4.2.0 unlisted channel version after their reply on 0.4.2 and they deleted that version before it got reviewed/signed. As per https://github.com/mozilla/addons-server/pull/20546 specifically When a developer disables a version that isn't signed yet and has needs_human_review, the due_date is removed., could the due date have gotten moved to the latest version 0.4.2.0 (now in the unlisted channel) and then dropped altogether instead of moving it back to the listed channel's latest version 0.4.2?

/cc @wagnerand

What did you expect to happen?

If a developer replies to a version, it should get marked for human review, and get a due date so that a reviewer can get to it via the manual review queue.

Is there an existing issue for this?

┆Issue is synchronized with this Jira Task

wagnerand commented 1 month ago

Like I said in chat, the unlisted version is entirely unrelated to this issue. It is likely caused by https://github.com/mozilla/addons-server/blob/2f365811dea159b7c4d1328e2865fbdeb83e2e25/src/olympia/versions/models.py#L220

KevinMind commented 1 month ago

If I understand correctly, we are talking about what should happen:

I'm trying to understand in what scenario we would NOT want a due date added to the version. If the developer is actively reaching out about it, they clearly have an interest and without a due date there would likely never be a response? @wagnerand can you describe such a scenario where that would be a good thing?

eviljeff commented 1 month ago

paraphrasing from a Slack DM: from reading https://github.com/mozilla/addons/issues/9131 that the PR resolves, and the associated story, somewhere along the way there was an idea that we should not have a due_date for a developer disabled version. That "developer disabled" requirement in the issue was translated into the file being disabled. Which is the case for developer disabled versions, but also the case for rejections. (For post-reviewed versions they'd be signed so it gets a due_date because of that condition, so this is only a problem for pre-reviewed versions)

Possible fixes:

wagnerand commented 1 month ago

@eviljeff covered it accurately, as far as I can see.

Fwiw, the original "spec" is in https://docs.google.com/document/d/1G3Bts1P7N7KA5vmLhwjZNCg50hVRn2CSfbsFaYPcD6I/edit

KevinMind commented 1 month ago

Looking into this further, maybe you can help straighten some stuff out @eviljeff.

We call the method reset_due_date EVERYWHERE.

This method seems to have logic that either sets an appropriate due date or clears any existing due date based on the results of should_have_due_date

So @wagnerand is right, I think this is where we need to modify the query to include versions which:

My thinking there (and this is where it get's a bit fuzzy) is that we use log_and_notify to create the NeedsHumanReview both for replies in reviewer tools and for replies from email reply.

When we create this record we get the creation date and reason NeedsHumanReview.REASONS.DEVELOPER_REPLY

So can we not expand the query in should_have_due_date to include versions which belong to a NHR that has the reason DEVELOPER_REPY?

I'm not sure about the lifecycle of that model. Can a version have multiple NeedsHumanReview instances accross the review lifecycle? Then we could have false positives.

Can we check the NHR against a recent rejection, seeing if the time stamp indicates the NHR is "more recent"? I still don't fully understand how to determine iff a version is rejected or not, and whend and how we model the rejection in the DB.

I wonder even if we could just traverse the activity log in all_activity and see if we get to a reply before anything else, if the most recent acitivty is a developer reply, then we should have a due date? 🤷 almost seems too simple, but I can't really see the issue with it. Too fuzzy.

Wdyt? Does any of this sound remotely plausible to someone more familiar with the codebase?

diox commented 1 month ago

I'm not sure about the lifecycle of that model. Can a version have multiple NeedsHumanReview instances accross the review lifecycle? Then we could have false positives.

Yes, it can. We also don't delete past NeedsHumanReview instances FYI: we set them to active=False when a reviewer has processed the version.

I wonder even if we could just traverse the activity log in all_activity and see if we get to a reply before anything else, if the most recent acitivty is a developer reply, then we should have a due date? 🤷 almost seems too simple, but I can't really see the issue with it. Too fuzzy.

That is super costly to do, and increases complexity or should_have_due_date quite a bit. At the moment it doesn't care about ordering or anything like that, that makes it a lot easier to follow.

If I understand the issue correctly, all we need to do is what @eviljeff suggested above:

  • The simple fix is to drop the disabled restriction entirely
  • We could check if the version was human reviewed and have that as an additional criteria, with Version.human_review_date
  • We could check if the version was specifically developer disabled

Either of these is trivial and cheap to do. Which one we should do would be up to @wagnerand.

wagnerand commented 1 month ago

Which of these meets the requirements in the scenarios specification?

diox commented 1 month ago

They all have subtle implications which is why I was asking for input. But here is a better proposal, with simpler implications:

We currently set a due date if:

There is an active NHR instance on a non-disabled or signed version
OR
The version has to be pre-reviewed for some reason

The scenario docs mention developer replies should get a due date without specifying anything about the version state, I'm suggesting we add a third possible condition to that logic:

There is an active NHR instance on a non-disabled or signed version
OR
The version has to be pre-reviewed for some reason
OR
There is an active NHR instance specifically for developer reply reason

This keeps current behavior intact for every other scenarios, including existing NHR when the add-on gets deleted/mozilla disabled, etc.

eviljeff commented 1 month ago

The scenario docs mention developer replies should get a due date without specifying anything about the version state, I'm suggesting we add a third possible condition to that logic:

There is an active NHR instance on a non-disabled or signed version
OR
The version has to be pre-reviewed for some reason
OR
There is an active NHR instance specifically for developer reply reason

That sounds good too, if we want to keep the fix focused on developer replies. (My suggestions were addressing the limitations of the original implementation for https://github.com/mozilla/addons/issues/9131 - but we don't have to touch those if we're going to special case developer reply NHRs instead)

diox commented 1 month ago

Yeah, looking at the scenarios, trying to tweak the original implementation feels more risky, so I feel we should special case NHRs like replies (possibly more in the future) that can be made on disabled versions and see if that's enough.

wagnerand commented 1 month ago

The challenge (and goal) is to avoid running into this or similar issues in the future where we only learn months or years after that an add-on/version was not flagged for review when it should have been.

eviljeff commented 1 month ago

The challenge (and goal) is to avoid running into this or similar issues in the future where we only learn months or years after that an add-on/version was not flagged for review when it should have been.

We can only absolutely guarantee nothing similar by dropping the restrictions on disabled versions, but that will lead to more add-ons in the queue.

The suggestion to special case reviewer replies would fix the scenario reported. If you want more cases where the version is disabled then we can drop (some of?) those restrictions too, but there would be a workload cost to Operations.

wagnerand commented 1 month ago

Can we align the implementation to the specifications? It is very hard for me (and I guess anyone) to understand the implications of a particular change in the current design.

diox commented 1 month ago

The implementation is aligned to the spec, we just had a bug in a very specific case because this is a complex topic with lots of variables no matter what we do (which caused both QA and us to miss this, because it only occurred under specific circumstances that weren't explicitly called out and therefore tested).

We can't predict bugs, we can just do our best to avoid them. My latest proposal tries to do that, minimizing the amount of changes and addressing the specific issue that was raised.

eviljeff commented 1 month ago

9131 - that I'm assuming is the closest we have to a spec - talks about developer disabled versions, but we're actually ignoring all disabled versions (that don't match another criteria, like being already signed).

But we define the spec so we can change it to whatever.

diox commented 1 month ago

The spec is actually https://docs.google.com/document/d/1G3Bts1P7N7KA5vmLhwjZNCg50hVRn2CSfbsFaYPcD6I/edit#heading=h.lqib3pi3a0yn (linked in the parent JIRA ticket)

At the time I explicitly chose to ignore NHR for all disabled versions that are not signed, because it was also handling deletion (add-on or version) and also because, per discussion with ops at the time, I understood that reviewers can't do anything about an already disabled version that hasn't been signed (if it has been signed, they could consider blocklisting). I just missed the possibility of an ongoing discussion with the developer on a rejected version.

eviljeff commented 1 month ago

The spec is actually https://docs.google.com/document/d/1G3Bts1P7N7KA5vmLhwjZNCg50hVRn2CSfbsFaYPcD6I/edit#heading=h.lqib3pi3a0yn (linked in the parent JIRA ticket)

Ah, if that doc is the spec (rather than the issue), then yeah, addressing reviewer replies is the way to alignment. I note the doc also calls out submission of source code as another action that should result in a due date, so we need that in addition to developer replies.

diox commented 1 month ago

It specifically calls out source code for a version pending rejection - so it wouldn't already be disabled. So that should already work correctly.

ioanarusiczki commented 2 weeks ago

I tested on -dev developer replies while awaiting review, rejected, delay rejected, disabled, invisible, blocked , approved and nhr + 2day due date is set, versions appeared in the Manual Review queue.

Exception: I don't see how a developer could add a reply for deleted versions at least not from dev hub. I didn't see an email sent to developers with reviewer replies either.

I hope that's all expected now.