Page MenuHomePhabricator

Revisions show in Other Revisions when acceptance status is unclear
Closed, ResolvedPublic

Description

If a revision has been accepted by one user, but another user has previously raised concerns regarding a revision (and it has subsequently been updated), then it will be displayed under "Other Revisions" in the bucketed list of revisions.

Instead these revisions should display under Waiting on Review (for the revision author and other reviewers) and Must Review (for the person who raised the concern).

Version Information:

phabricator

c3bdcb4ca85487921909f0202aa760e8ed61404a (Sun, Feb 5) (branched from 9c62a10989e03e518f855de3fc0a610543287c71 on origin)

arcanist

67a47acebd6b5e5809745db12955ad251c66004e (Tue, Feb 14) (branched from 224986af634e1dfc40916e5baee76897db4c907f on origin)

phutil

58fb3b94464e1c00d0f348d1501bec6b8690253c (Tue, Feb 14) (branched from 82f71f62129bc19f28aeed8c660c931e512a66e7 on origin)

Reproduction Steps:

  1. Create a review
  2. As someone else, request changes to the revision
  3. As the original person, update the review diff
  4. As a third person, review the updated diff and accept it.

This should result in a review with two reviewers, where one has "Accepted" it and another has "Rejected Prior Diff".
This review should then be shown under "Other Revisions".

The only Differential settings we have changed from upstream defaults are:
a) differential.require-test-plan-field: False
b) differential.always-allow-close: True
c) differential.allow-reopen: True

Everything else is at defaults.

Event Timeline

chad added a subscriber: chad.Feb 26 2017, 9:55 PM

We need version information on all bug reports.

chad added a comment.Feb 26 2017, 9:56 PM

You also mention "raised the concern". Is this report against Audit or Differential?

bcooksley updated the task description. (Show Details)Feb 26 2017, 9:59 PM

This report is against Differential.

An example of this behaviour can be seen at https://phabricator.kde.org/differential/query/eSo.v6zZDm4i/#R

chad added a comment.Feb 26 2017, 10:04 PM
In T12323#213451, @chad wrote:

We need version information on all bug reports.

chad added a comment.Feb 26 2017, 10:06 PM

Also, we need steps to reproduce. the issue locally. There are setting that affect where revisions go after acceptance. I don't know what your install has those set to.

Sorry, you'll need to provide more detail on what's wrong than repeating your comment. I added the version information to the Task Description.

bcooksley updated the task description. (Show Details)Feb 26 2017, 10:10 PM
chad added a comment.Feb 26 2017, 10:10 PM

Well that's my mistake then! I didn't see it initially.

bcooksley updated the task description. (Show Details)Feb 26 2017, 10:16 PM

I've now added reproduction steps, including details on customised settings for the Differential application.

chad added a comment.Feb 26 2017, 10:19 PM

Is sticky-accept still set to true then? (I think that's the default).

If sticky-accept is true, then I think the revision is still accepted after the update.

Yes, sticky-accept is True.

If the revision was "Accepted" then shouldn't it show under Ready to Land?

My assumption reading through docs and tasks is that "Other" means... well, other. It's been both accepted and rejected and I think it's important to convey it's not completely in one or the other state. If you set sticky-accept to false, then it will always go back for review. Maybe @epriestley has other thoughts. I would swear this came up recently though, but I can't find an absolute duplicate task.

chad added a comment.Feb 26 2017, 10:28 PM

T9372 is the main "Differential Buckets" task.

Note that these steps depend on who is viewing the page (or, specifically, who "Responsible Users" are). More concise steps are likely:

  • Create a revision as Alice.
  • Reject it as Betty.
  • Accept it as Claire.
  • As Claire, view Differential's "Action Required" view.

In this state, the revision is not accepted (Betty has rejected it) and not ready to land. The viewer (Clarie) can not act on it. The revision currently falls through the cracks a little bit and ends up in "Other Revisions".

I think there's a reasonable argument that that's a good place for it, but putting it in "Waiting on Authors" might be a little clearer. The expected next step is for the author (Alice) to address the rejecting user's (Betty) feedback.

@bcooksley, feel free to mention that you've talked with me about stuff when filing tasks in the future. Like 90% of what shows up here spontaneously is pretty questionable, so unannounced reports get fairly strict scrutiny.

chad added a comment.Feb 26 2017, 11:01 PM

Sorry, yes I am bad cop sometimes. :(

The actual reproduction instructions are slightly more involved:

  • Alice creates a revision.
  • Betty rejects it.
  • Claire accepts it.
  • Alice updates it.
  • Look at the revision list as Claire.

The revision now needs review by Betty, who has an active reject. Only Betty can move it forward.

Prior to the update, the revision already buckets into "Waiting on Authors", which I think is reasonable.

After the update, it should not go into "Waiting on Authors". I can add a "Waiting on Other Reviewers" bucket, I suppose. I dislike creating more buckets and also dislike conditional buckets, but this state is reasonable and doesn't really fit into the other buckets.

(They could also go into the existing "Waiting on Review" bucket, but I dislike mixing authored and non-authored revisions in that bucket.)