Index: src/applications/policy/controller/PhabricatorPolicyEditController.php =================================================================== --- src/applications/policy/controller/PhabricatorPolicyEditController.php +++ src/applications/policy/controller/PhabricatorPolicyEditController.php @@ -49,6 +49,7 @@ $default_action = $policy->getDefaultAction(); $rule_data = $policy->getRules(); + $errors = array(); if ($request->isFormPost()) { $data = $request->getStr('rules'); $data = @json_decode($data, true); @@ -83,26 +84,42 @@ ); } + // Filter out nonsense rules, like a "users" rule without any users + // actually specified. + $valid_rules = array(); + foreach ($rule_data as $rule) { + $rule_class = $rule['rule']; + if ($rules[$rule_class]->ruleHasEffect($rule['value'])) { + $valid_rules[] = $rule; + } + } + + if (!$valid_rules) { + $errors[] = pht('None of these policy rules have any effect.'); + } + // NOTE: Policies are immutable once created, and we always create a new // policy here. If we didn't, we would need to lock this endpoint down, // as users could otherwise just go edit the policies of objects with // custom policies. - $new_policy = new PhabricatorPolicy(); - $new_policy->setRules($rule_data); - $new_policy->setDefaultAction($request->getStr('default')); - $new_policy->save(); - - $data = array( - 'phid' => $new_policy->getPHID(), - 'info' => array( - 'name' => $new_policy->getName(), - 'full' => $new_policy->getName(), - 'icon' => $new_policy->getIcon(), - ), - ); - - return id(new AphrontAjaxResponse())->setContent($data); + if (!$errors) { + $new_policy = new PhabricatorPolicy(); + $new_policy->setRules($valid_rules); + $new_policy->setDefaultAction($request->getStr('default')); + $new_policy->save(); + + $data = array( + 'phid' => $new_policy->getPHID(), + 'info' => array( + 'name' => $new_policy->getName(), + 'full' => $new_policy->getName(), + 'icon' => $new_policy->getIcon(), + ), + ); + + return id(new AphrontAjaxResponse())->setContent($data); + } } // Convert rule values to display format (for example, expanding PHIDs @@ -120,7 +137,13 @@ 'name' => 'default', )); + if ($errors) { + $errors = id(new AphrontErrorView()) + ->setErrors($errors); + } + $form = id(new PHUIFormLayoutView()) + ->appendChild($errors) ->appendChild( javelin_tag( 'input', Index: src/applications/policy/rule/PhabricatorPolicyRule.php =================================================================== --- src/applications/policy/rule/PhabricatorPolicyRule.php +++ src/applications/policy/rule/PhabricatorPolicyRule.php @@ -34,4 +34,15 @@ return $value; } + /** + * Return true if the given value creates a rule with a meaningful effect. + * An example of a rule with no meaningful effect is a "users" rule with no + * users specified. + * + * @return bool True if the value creates a meaningful rule. + */ + public function ruleHasEffect($value) { + return true; + } + } Index: src/applications/policy/rule/PhabricatorPolicyRuleProjects.php =================================================================== --- src/applications/policy/rule/PhabricatorPolicyRuleProjects.php +++ src/applications/policy/rule/PhabricatorPolicyRuleProjects.php @@ -64,4 +64,8 @@ return mpull($handles, 'getFullName', 'getPHID'); } + public function ruleHasEffect($value) { + return (bool)$value; + } + } Index: src/applications/policy/rule/PhabricatorPolicyRuleUsers.php =================================================================== --- src/applications/policy/rule/PhabricatorPolicyRuleUsers.php +++ src/applications/policy/rule/PhabricatorPolicyRuleUsers.php @@ -50,4 +50,8 @@ return mpull($handles, 'getFullName', 'getPHID'); } + public function ruleHasEffect($value) { + return (bool)$value; + } + }