Page MenuHomePhabricator
Create Task
Maniphest T12533

Reducing "Accept" action to not render a checkbox when only one package or project would be accepted is perhaps more confusing than not
Closed, ResolvedPublic
Actions

Description

Version information:

phabricator 087fb93036d98cdf89057578f2e18efe31edd803 (Thu, Apr 6)
arcanist 65125d5ac6286a88b3bdfa83693fd2344df2e190 (Thu, Apr 6)
phutil db82199467d95c6a079624d2725aa67a656d5e23 (Tue, Apr 4)

Reproduction steps:

  1. As a member (lvital) of ci-job-configs:/devprod/submitqueue/, request changes in a revision that includes this package
    Screen Shot 2017-04-10 at 3.59.32 PM.png (101×439 px, 17 KB)
  2. As another member (jmeador) of ci-job-configs:/devprod/submitqueue/, add action "accept changes"

Expected: Have option to accept for [ ] jmeador and [ ] ci-job-configs:/devprod/submitqueue/.

Actual:

  1. Accept for everything
  2. Resulting accept results in all owner packages owned by jmeador
    Screen Shot 2017-04-10 at 3.59.21 PM.png (103×448 px, 17 KB)

Revisions and Commits

rP Phabricator
D17652rP26d6096e0a5b When reviewing, always show "Accept" checkboxes for packages/projects, even if…

Event Timeline

lvital created this task.Apr 10 2017, 11:06 PM
Herald added a subscriber: eadler. · View Herald TranscriptApr 10 2017, 11:06 PM
epriestley added a subscriber: epriestley.Apr 10 2017, 11:21 PM

It looks like before step (2), jmeador can only accept for one reviewer (the package), since he's already accepted for himself. The current rule is:

  • Only show checkboxes for accepts which would actually do something (so "[X] jmeador" won't be shown: he's already accepted).
  • If we would only show one checked checkbox, don't show anything.

We could change the second rule to maybe be "if we would only show one checked checkbox, AND that checkbox is the user themselves, don't show anything". I want this workflow to reduce down to "no checkboxes" in the simple (no packages / projects) case but we could reduce it a little less aggressively.

Or maybe this make sense with that explanation?

lvital added a comment.Apr 10 2017, 11:39 PM

Whoops, I think I may have totally fudged the screenshots. Let me try to reproduce with proper screenshots.

lvital added a comment.Apr 11 2017, 12:02 AM

Scratch that, you're right @epriestley . We just got confused during the workflow and didn't realize jmeador previously accepted.

We could change the second rule to maybe be "if we would only show one checked checkbox, AND that checkbox is the user themselves, don't show anything".

I could see this reducing confusion like this. If it's trivial and you agree it's a good idea, I'd say go for it. Regardless, I'll go ahead and just close this since it's not actually a bug :).

noidea

lvital added a comment.Apr 11 2017, 12:03 AM

Or I guess, you* can close this since I can't. Whoops.

epriestley added a comment.Apr 11 2017, 12:05 AM

You should have sweeping janitorial powers as a member of Community now.

I think the checkbox thing is trivial and reasonable, let me see if it's more involved than I'm remembering.

lvital closed this task as Invalid.Apr 11 2017, 12:07 AM

Thanks, closing this one!

epriestley renamed this task from Reviewers are unable to accept for a subset of packages if they have all been rejected to Reducing "Accept" action to not render a checkbox when only one package or project would be accepted is perhaps more confusing than not.Apr 11 2017, 12:08 AM
epriestley added a revision: D17652: When reviewing, always show "Accept" checkboxes for packages/projects, even if there's only one checkbox.Apr 11 2017, 12:10 AM
epriestley changed the task status from Invalid to Resolved by committing rP26d6096e0a5b: When reviewing, always show "Accept" checkboxes for packages/projects, even if….Apr 11 2017, 12:28 AM
epriestley added a commit: rP26d6096e0a5b: When reviewing, always show "Accept" checkboxes for packages/projects, even if….
Phabricator · Built by Phacility