Page MenuHomePhabricator

Support custom actions in Herald
ClosedPublic

Authored by hach-que on Apr 16 2014, 8:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 22, 8:23 PM
Unknown Object (File)
Sun, Dec 22, 6:47 PM
Unknown Object (File)
Fri, Dec 20, 12:09 PM
Unknown Object (File)
Fri, Dec 20, 11:25 AM
Unknown Object (File)
Fri, Dec 20, 11:10 AM
Unknown Object (File)
Fri, Dec 20, 6:11 AM
Unknown Object (File)
Fri, Dec 13, 7:21 PM
Unknown Object (File)
Wed, Dec 11, 1:45 PM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T4884: At some point, review hach-que's diffs
Commits
Restricted Diffusion Commit
rP88aba65d54c2: Support custom actions in Herald
Summary

This was significantly easier than expected. Here's an example of what an extension class might look like:

<?php

final class AddRiskReviewHeraldCustomAction extends HeraldCustomAction {

  public function appliesToAdapter(HeraldAdapter $adapter) {
    return $adapter instanceof HeraldDifferentialRevisionAdapter;
  }

  public function appliesToRuleType($rule_type) {
    return $rule_type == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL ||
      $rule_type == HeraldRuleTypeConfig::RULE_TYPE_OBJECT;
  }

  public function getActionKey() {
    return 'custom:add-risk';
  }

  public function getActionName() {
    return 'Add risk rating (JSON)';
  }

  public function getActionType() {
    return HeraldAdapter::VALUE_TEXT;
  }

  public function applyEffect(
    HeraldAdapter $adapter,
    $object,
    HeraldEffect $effect) {

    $key = "phragile:risk-rating";

    // Read existing value.
    $field_list = PhabricatorCustomField::getObjectFields(
      $object,
      PhabricatorCustomField::ROLE_VIEW);
    $field_list->readFieldsFromStorage($object);
    $field_list = mpull($field_list->getFields(), null, 'getFieldKey');
    $field = $field_list[$key];
    $field->setObject($object);
    $field->setViewer(PhabricatorUser::getOmnipotentUser());

    $risk = $field->getValue();
    $old_risk = $risk; // PHP copies arrays by default!

    // Add new value to array.
    $herald_args = phutil_json_decode($effect->getTarget());
    $risk[$herald_args['key']] = array(
      'value' => $herald_args['value'],
      'reason' => $herald_args['reason']);
    $risk_key = $herald_args['key'];

    // Set new value.
    $adapter->queueTransaction(
      id(new DifferentialTransaction())
        ->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD)
        ->setMetadataValue('customfield:key', $key)
        ->setOldValue($old_risk)
        ->setNewValue($risk));

    return new HeraldApplyTranscript(
      $effect,
      true,
      pht(
        'Modifying automatic risk ratings (key: %s)!',
        $risk_key));
  }

}
Test Plan

Created a custom action for differential revisions, set up a Herald rule to match and trigger the custom action, did 'arc diff' and saw the action trigger in the transcripts.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

hach-que retitled this revision from to Support custom actions in Herald.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.

Currently only the Differential Herald adapter has been updated (so custom actions will only appear under that). Will fix soon.

hach-que edited edge metadata.

Re-run with lint and unit tests

Hook up other adapters, remove redundant getObjectForCustomActions method

Fix crash when editing rules

Add mechanism for queuing transactions in herald actions

This is actually way more reasonable than I imagined, I think we can definitely upstream it.

  • Is there any specific reason that HeraldCustomAction implements multiple actions, instead of just one? It looks like doing one-to-one would simplify a few things.
  • What does getActions() return? Looks like a dictionary of magic -- can this be a few separate methods instead (getActionName(), etc)? After one-to-one, this probably gets easier.
  • Can we implement the Flag action a custom action? What are the barriers to implementing more/other/all actions in this way? (Maybe this shouldn't be called "Custom" action, since I think we can put everything in it and that's highly desirable).
  • Can the massive levels of hacks in the rendering code call into the action implementation instead? (renderActionForTranscript() or similar)? This is just existing badness in Herald.

It was mainly modelled off how the adapters currently do things with a giant switch statement. Bundling multiple actions together in the same class sometimes makes sense (in our case we have "add risk review" and "remove risk review" actions, both which go in the same CustomAction class). But we can totally make this one-to-one and I can't foresee any issues with that (and that also solves the getActions magic).

We should be able to implement other, if not all, actions with this model. General actions (such as add a flag) are probably easier to change over than things that depend heavily on the adapter state (which I'm guessing is the case for some of the actions in the adapters). Currently a check is done inside getActions to see if something applies, in a one-to-one model we could have a similar method that is used to indicate whether an action can be applied for the current rule type and adapter.

src/applications/herald/controller/HeraldTranscriptController.php
345–369

Ideally replace this blob with a call to custom action rendering. That will require migrating the existing actions to the new structure.

The adapter and rule type are both available here, so we can pull in a list of compatible actions and call a renderActionForTranscript method on the right one.

Restructure this so that custom actions are one-to-one. While I'm at it, remove the "Custom" part of the name so that other actions can be migrated in the future.

hach-que edited edge metadata.

Rearchitecture to orientate custom action class for a single action

epriestley edited edge metadata.

One minor thing -- sorry for the long feedback cycles, I'll try to do the next one promptly. This looks good to me. Some bits of it could probably be a little cleaner by refactoring how some of the adapter stuff works, but it's probably good to separate those out of this anyway.

src/applications/herald/adapter/HeraldAdapter.php
128–138

Instead of making this a static side effect of construction and then filtering the actions over and over again, let's do this:

  • Make $customActions nonstatic.
  • Add getCustomActions(), which builds custom actions if they're unbuilt.
  • Have it use PhutilSymbolLoader->loadObjects(), which slightly simplifies this pattern. It also handles abstract subclasses correctly, which this does not.
  • Have it filter actions which don't apply to the current adapter.

This will simplify the other code a bit since we'll be able to assume that $this->getCustomActions() doesn't need an adapter compatibility check.

This revision now requires changes to proceed.May 17 2014, 3:50 AM
hach-que edited edge metadata.

Changes requested in code review

epriestley edited edge metadata.

Some really nitpicky style stuff inline -- maybe I'll send you a counterdiff at some point. I think we'd be slightly better off keeping $customActions keyed by getActionKey() and explicitly rejecting duplicates. The rest of it is pretty whatever. Looks great functionally. Thanks!

src/applications/herald/adapter/HeraldAdapter.php
109–118

Hypothetically, it's slightly cleaner to use a temporary variable here and then assign $this->customActions = $actions; at the end, so the object doesn't end up in a bad state if appliesToAdapter() throws.

113–115

Would it make sense to key this by getActionKey() and throw on duplicates? It seems like that would simplify some logic later on...

166–183

abstract public is consistent in this codebase.

170

This would be simplified by a getActionKey() mapping.

176–178

Oh.. are you intentionally allowing multiple custom actions to have the same key? Is there a need for this? It seems very messy.

691–700

Maybe cleaner to implement getCustomActionsForRuleType($rule_type), and then do:

$custom_actions = $this->getCustomActionsForRuleType($rule_type);
return mpull($custom_actions, 'getActionKey');

On its own, this isn't much cleaner, but the method could be reused below.

740–745

Then this could be:

$custom_actions = $this->getCustomActionsForRuleType($rule_type);
$standard += mpull($custom_actions, 'getActionName', 'getActionKey');

Not sure this is really all that much cleaner.

926

ActionKey keying would simplify this, too.

This revision is now accepted and ready to land.Jul 2 2014, 2:16 AM
hach-que updated this revision to Diff 23509.

Closed by commit rP88aba65d54c2 (authored by @hach-que).

Oops, I didn't see those inlines before landing...

Will send you a diff for these fixes.

src/applications/herald/adapter/HeraldAdapter.php
113–115

Can we use mpull after this is done to set the keys to getActionKey or does that method not care about duplicates?

166–183

Yeah not really sure how that got in there.

176–178

No it's not intentional, just a side effect of the way the code is written. Probably need to check for duplicates as per above.

(I think this is a side effect of when a single class could handle multiple actions).