Make Herald rules more resilient
ClosedPublic
Actions

Authored by joshuaspence on May 20 2015, 9:37 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Summary

Make Herald conditions and actions more resilient (see discussion in D12896). This protects against invalid rules, which may have been valid in the past but are no longer valid. Specifically:

If a rule has an invalid field, the conditions fail and the actions do not execute.
The transcript shows that the rule failed because of an invalid field, and points at the issue.
If a rule has an invalid action, that action fails but other actions execute.
The transcript shows that the action failed.
Everything else (particularly, other rules) continues normally in both cases.
The edit interface is somewhat working when editing an invalid rule, but it could use some further improvements.

Test Plan

Created a Herald rule with an invalid condition.
1. Ran this rule on a differential revision and saw the rule fail in the transcript.
2. Was able to submit a differential without receiving an ERR-CONDUIT-CORE.
3. Edited the Herald rule using the UI and was able to save the rule succesfully.
Created a Herald rule with one valid action and one invalid action.
1. Ran this rule on a differential revision and saw one success and one failure in the transcript.
2. Was able to submit a differential without receiving an ERR-CONDUIT-CORE.
3. Edited the Herald rule using the UI. Clicking save caused a HeraldInvalidActionException to be thrown, but maybe this is okay.

Diff Detail

Repository

rP Phabricator

Branch

master

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6175
Build 6196: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 31209.May 20 2015, 9:37 PM

joshuaspence retitled this revision from to Make Herald rules more resilient.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 20 2015, 9:37 PM

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

Further progress

Seems working now

joshuaspence updated this object.May 21 2015, 11:10 AM

joshuaspence edited the test plan for this revision. (Show Details)

Ah actually, saving the rule wiped out the value for the condition.

joshuaspence added a child revision: D12896: Remove arcanist projects from Herald.May 21 2015, 11:13 AM

In D12957#134582, @joshuaspence wrote:

Ah actually, saving the rule wiped out the value for the condition.

I think this is because of the typeahead... when I nuked the "flag" action for testing purposes, the value persisted.

johnny-bit mentioned this in D12896: Remove arcanist projects from Herald.May 21 2015, 11:41 AM

I think it's fine to require rules to be valid in order to be saved. Couple of minor inlines to smooth the UX out a little.

In HeraldTranscriptController.php near line 383, we do this:

$rule = idx($action_names, $apply_xscript->getAction(), pht('Unknown'));

Let's make the default for that look more like <Unknown Action "flag"> too? That cleans up the transcripts a bit.

src/applications/herald/controller/HeraldRuleController.php
323	Let's make this `$ex->getMessage();`? That makes the error a bit more user-friendly.
453	Maybe translate this as `<Unknown Action "%s">` to make debugging easier? And same for `<Unknown Field>` above?