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

Branch

herald-custom-action

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

hach-que updated this revision to Diff 20843.Apr 16 2014, 8:07 AM

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.

Herald added a reviewer: Blessed Reviewers.Apr 16 2014, 8:07 AM

Herald added subscribers: Korvin, epriestley.

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

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.

hach-que added inline comments.Apr 23 2014, 10:46 PM

src/applications/herald/controller/HeraldTranscriptController.php
301–325	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.

Rearchitecture to orientate custom action class for a single action

Harbormaster completed remote builds in B282: Diff 21375.May 7 2014, 5:55 AM

Fix issues

Harbormaster completed remote builds in B283: Diff 21376.May 7 2014, 6:14 AM

hach-que updated this object.May 7 2014, 6:14 AM

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
108–118	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

ite-klass added a subscriber: ite-klass.May 27 2014, 1:22 PM

• edutibau added a subscriber: • edutibau.Jun 12 2014, 6:10 PM

locutus added a subscriber: locutus.Jun 30 2014, 5:35 PM

Changes requested in code review

Harbormaster completed remote builds in B1478: Diff 23504.Jul 2 2014, 1:00 AM

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
107–116	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.
111–113	Would it make sense to key this by `getActionKey()` and throw on duplicates? It seems like that would simplify some logic later on...
162–176	`abstract public` is consistent in this codebase.
166	This would be simplified by a `getActionKey()` mapping.
172–174	Oh.. are you intentionally allowing multiple custom actions to have the same key? Is there a need for this? It seems very messy.
676–688	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.
727–732	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.
913	ActionKey keying would simplify this, too.

This revision is now accepted and ready to land.Jul 2 2014, 2:16 AM

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
111–113	Can we use `mpull` after this is done to set the keys to `getActionKey` or does that method not care about duplicates?
162–176	Yeah not really sure how that got in there.
172–174	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).