Page MenuHomePhabricator

Config - add an option to lock policy settings
ClosedPublic

Authored by btrahan on Jan 12 2015, 11:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jan 31, 8:16 PM
Unknown Object (File)
Fri, Jan 24, 11:36 AM
Unknown Object (File)
Thu, Jan 23, 9:33 PM
Unknown Object (File)
Thu, Jan 23, 9:33 PM
Unknown Object (File)
Thu, Jan 23, 9:33 PM
Unknown Object (File)
Thu, Jan 23, 9:33 PM
Unknown Object (File)
Thu, Jan 23, 9:33 PM
Unknown Object (File)
Thu, Jan 23, 9:32 PM
Subscribers

Details

Summary

Fixes T6947

Test Plan

locked people.create.user and noted the UI only showed a link to the existing policy with no way to edit it.

tried to set the config to all the various bad things and saw helpful error messages telling me what I did wrong.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Config - add an option to lock policy settings.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.

It might be slightly safer to have a map<policy-name, forced-policy> but I'm not sure how easy that is to implement. But then if we decide to lock something later, we don't have to migrate existing installs: we can just add the lock and force it to the desired policy.

This revision is now accepted and ready to land.Jan 12 2015, 11:52 PM
btrahan edited edge metadata.

make the storage a dictionary and validate the heck out of everything

clean up some implemenation notes left in as comments

This revision was automatically updated to reflect the committed changes.
src/applications/meta/controller/PhabricatorApplicationEditController.php
118

I think $locked_map will end up with array('admin' => true) here? We can just remove this line and rename one of the variables, I think?

src/applications/policy/config/PhabricatorPolicyConfigOptions.php
49

This option should probably be setLocked(true) itself, so you can't just unlock it and then unlock the policies. We can force it locked in config, but in the general case locking it seems like the right default.

src/applications/policy/config/PolicyLockOptionType.php
7

ees niceee

on the feeedback, new diff coming soonish

Not sure why the robots didn't close this one. Daemon task queue looks clear from the web console at the moment.

There was a close transaction earlier (D11358#106423), maybe there's some rare race condition.