recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix a Herald repetition policy selection error for rule types which support only one policy

Summary:
Ref T13048. See <https://discourse.phabricator-community.org/t/configuring-commit-hook-commit-content-rules-fail-with-exception/1077/3>.

When a rule supports only one repetition policy (always "every time") like "Commit Hook" rules, we don't render a control for `repetition_policy` and fail to update it when saving.

Before the changes to support the new "if the rule did not match the last time" policy, this workflow just defaulted to "every time" if the input was invalid, but this was changed by accident in D18926 when I removed some of the toInt/toString juggling code.

(This patch also prevents users from fiddling with the form to create a rule which evaluates with an invalid policy; this wasn't validated before.)

Test Plan:
- Created new "Commit Hook" (only one policy available) rule.
- Saved existing "Commit Hook" rule.
- Created new "Task" (multiple policies) rule.
- Saved existing Task rule.
- Set task rule to each repetition policy, saved, verified the save worked.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18992

epriestley ef121b3e b3880975

+18 -9
+18 -9
src/applications/herald/controller/HeraldRuleController.php
··· 265 265 $new_name = $request->getStr('name'); 266 266 $match_all = ($request->getStr('must_match') == 'all'); 267 267 268 - $repetition_policy_param = $request->getStr('repetition_policy'); 268 + $repetition_policy = $request->getStr('repetition_policy'); 269 + 270 + // If the user selected an invalid policy, or there's only one possible 271 + // value so we didn't render a control, adjust the value to the first 272 + // valid policy value. 273 + $repetition_options = $this->getRepetitionOptionMap($adapter); 274 + if (!isset($repetition_options[$repetition_policy])) { 275 + $repetition_policy = head_key($repetition_options); 276 + } 269 277 270 278 $e_name = true; 271 279 $errors = array(); ··· 348 356 $match_all, 349 357 $conditions, 350 358 $actions, 351 - $repetition_policy_param); 359 + $repetition_policy); 352 360 353 361 $xactions = array(); 354 362 $xactions[] = id(new HeraldRuleTransaction()) ··· 373 381 // mutate current rule, so it would be sent to the client in the right state 374 382 $rule->setMustMatchAll((int)$match_all); 375 383 $rule->setName($new_name); 376 - $rule->setRepetitionPolicyStringConstant($repetition_policy_param); 384 + $rule->setRepetitionPolicyStringConstant($repetition_policy); 377 385 $rule->attachConditions($conditions); 378 386 $rule->attachActions($actions); 379 387 ··· 594 602 */ 595 603 private function renderRepetitionSelector($rule, HeraldAdapter $adapter) { 596 604 $repetition_policy = $rule->getRepetitionPolicyStringConstant(); 597 - 598 - $repetition_options = $adapter->getRepetitionOptions(); 599 - $repetition_names = HeraldRule::getRepetitionPolicySelectOptionMap(); 600 - $repetition_map = array_select_keys($repetition_names, $repetition_options); 601 - 605 + $repetition_map = $this->getRepetitionOptionMap($adapter); 602 606 if (count($repetition_map) < 2) { 603 - return head($repetition_names); 607 + return head($repetition_map); 604 608 } else { 605 609 return AphrontFormSelectControl::renderSelectTag( 606 610 $repetition_policy, ··· 611 615 } 612 616 } 613 617 618 + private function getRepetitionOptionMap(HeraldAdapter $adapter) { 619 + $repetition_options = $adapter->getRepetitionOptions(); 620 + $repetition_names = HeraldRule::getRepetitionPolicySelectOptionMap(); 621 + return array_select_keys($repetition_names, $repetition_options); 622 + } 614 623 615 624 protected function buildTokenizerTemplates() { 616 625 $template = new AphrontTokenizerTemplateView();