Apply "Can Bulk Edit Tasks" to "Move Tasks" workboard options
AcceptedPublic
Actions

Authored by aklapper on Sat, Mar 22, 08:13.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Summary

The "Can Bulk Edit Tasks" Maniphest application setting introduced in rP7fedfacbca27c165cf193f286cef9306cf9762e2 applies to the "Bulk Edit Tasks…" menu item in the project workboard column dropdown. Make it also apply to the menu items "Move Tasks to Column…" and "Move Tasks to Project…" as those are similar actions.

Test Plan

Go to /applications/view/PhabricatorManiphestApplication/ and set Can Bulk Edit Tasks to Administrators
Go to /project/board/1/ and create a second column
As a default user, open the column header dropdown of the default column on /project/board/1/
Select Bulk Edit Tasks… (no changes in behavior)
Select Move Tasks to Column…; select Move Tasks to Project…, don't succeed.
Try step 5 again as an administrator, succeed.

Diff Detail

Repository

rP Phorge

Branch

bulkPerms (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1787
Build 1787: arc lint + arc unit

Event Timeline

aklapper created this revision.Sat, Mar 22, 08:13

Owners added a reviewer: O1: Blessed Committers.Sat, Mar 22, 08:13

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptSat, Mar 22, 08:13

aklapper requested review of this revision.Sat, Mar 22, 08:13

Harbormaster completed remote builds in B1787: Diff 2752.Sat, Mar 22, 08:13

(Feel free to copy-paste the downstream task in Phorge - so the lack of task is not used as blocking reason - maybe title "Allow to mitigate spammers from workboard bulk move" or something similar)

P.S. maybe if the task number is very small (e.g. 2) maybe we can still allow that.

src/applications/project/controller/PhabricatorProjectColumnBulkMoveController.php
45	Does it work with the `::class` notation? so the IDE is happier It should be 100% the same since it returns the very same string. We already have done that somewhere.

aklapper marked an inline comment as done.Sat, Mar 22, 09:55

aklapper added inline comments.

src/applications/project/controller/PhabricatorProjectColumnBulkMoveController.php
45	`grep -r "PhabricatorApplication::getByClass" . \| wc -l` says 46. If you prefer to replace that by a different notation to make some IDE happy, feel free to file a separate task for consistency across the codebase. Thanks!

In D25912#24618, @valerio.bozzolan wrote:

P.S. maybe if the task number is very small (e.g. 2) maybe we can still allow that.

I'd prefer not to introduce non-obvious confusing behavior (sometimes it does, sometimes it doesn't?) to increase code (and testing) complexity for no good reason...

Took the opportunity to fix a typo in the summary.

In D25912#24622, @aklapper wrote:

In D25912#24618, @valerio.bozzolan wrote:

P.S. maybe if the task number is very small (e.g. 2) maybe we can still allow that.

I'd prefer not to introduce non-obvious confusing behavior (sometimes it does, sometimes it doesn't?) to increase code (and testing) complexity for no good reason...

You are right premising that I thought that is there is just 1 task this "bulk blocker" has just no sense since the user can just drag & drop that task lol, so it sounded a bit overkill to add such "You do not have permission to bulk edit 1 task" lol

valerio.bozzolan accepted this revision.Sat, Mar 22, 21:47

valerio.bozzolan added inline comments.