Page MenuHomePhabricator

Make the new Build Plan "Runnable" behavior work
ClosedPublic

Authored by epriestley on Thu, Feb 28, 7:41 PM.

Details

Summary

Ref T13258. Fixes T11415. This makes "Runnable" actually do something:

  • With "Runnable" set to "If Editable" (default): to manually run, pause, resume, abort, or restart a build, you must normally be able to edit the associated build plan.
  • If you toggle "Runnable" to "If Viewable", anyone who can view the build plan may take these actions.

This is pretty straightforward since T9614 already got us pretty close to this ruleset a while ago.

Test Plan
  • Created a Build Plan, set "Can Edit" to just me, toggled "Runnable" to "If Viewable"/"If Editable", tried to take actions as another user.
  • With "If Editable", unable to run, pause, resume, abort, or restart as another user.
  • With "If Viewable", those actions work.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Thu, Feb 28, 7:41 PM
Owners added a subscriber: Restricted Owners Package.Thu, Feb 28, 7:41 PM
epriestley requested review of this revision.Thu, Feb 28, 7:43 PM
amckinley accepted this revision.Tue, Mar 5, 7:38 PM
amckinley added inline comments.
src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php
21–22

"You must have edit permission on this build plan to pause, abort, resume, restart, or run it manually."

This revision is now accepted and ready to land.Tue, Mar 5, 7:38 PM
epriestley added inline comments.Wed, Mar 6, 1:55 PM
src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php
21–22

My phrasing is weird, but I think this replacement phrasing allows a reading as "...to (x, y, or z) it manually". That is, it could suggest we're talking about the ability to pause the build "manually", versus pause the build in general.

Although this is technically true today (pauses are manual), there's no other type of pause, while there are other types of "run". Notably, you can still write a Herald global rule to run a plan you can't run manually. So the "manual" part is an important qualifier for "run", but a possibly misleading qualifier for "pause/restart/etc".

I'm just going to break this into two rules/sentences, since that's probably clearer and we might refine these rules eventually anyway.

epriestley updated this revision to Diff 48335.Wed, Mar 6, 1:57 PM
  • Describe "run" and "pause/abort/resume/restart" in two different rules to get rid of weird compound clauses.
This revision was automatically updated to reflect the committed changes.