Summary: Ref T8726. Convert these to be modular.
Test Plan:
- Created rules using these actions.
- Upgraded them.
- Verified they still work.
{F659266}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13705
Summary: Ref T8726. No surprises.
Test Plan:
Created rules using both action variants, applied upgrade, saw rules still work correctly.
{F658842}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13701
Summary: Ref T8726. Converts these actions to be modular. No real surprises in this change.
Test Plan:
{F658709}
- Wrote some rules.
- Migrated them forward.
- Used a bunch of these rules.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13699
Summary:
Ref T8726. This modularizes "Mark with flag", plus rebuilds transcripts in a more modern/flexible way. The big transcript stuff is:
- Transcripts are now translatable.
- Transcripts can now show multiple outputs from a single action. For example, an action like "add A, B, C to subscribers" can now say "added A; B is invalid; C was already subscribed".
Test Plan: {F637784}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, eadler, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13649
Summary:
Ref T8726. Herald actions are technically sort-of modular already, but make them more aggressively modular similar to `HeraldField`.
I plan to obsolete and replace `HeraldCustomAction`.
Test Plan: Saw actions in nice groups; created and ran a "Do Nothing" action. Transcripts are a bit rough for now.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13646
Summary:
Ref T8726. I want to modularize actions like fields, but the base class should be "HeraldAction".
Eventually, "HeraldCondition" should probably be "HeraldFieldRecord", and then both Action and Condition should just be rolled into Rule, probably, but that can wait and doesn't block anything.
Test Plan: Ran migration, poked around UI, used `git grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13644
Summary:
Ref T8726. Some adapters now have a large number of fields, and we lost the sort-of-human-readable implicit ordering when fields were modularized.
Instead, group and sort fields.
Test Plan: {F603066}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13619
Summary: Ref T8726. This gets rid of all the `VALUE_*` constants and lets Fields provide arbitrary typeaheads without upstream/JS changes.
Test Plan: Used all tokenizers. Used "Another Herald Rule". Grepped for all removed constants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13616
Summary: Ref T8726. The gruntwork part of this is finally over.
Test Plan:
- Made a huge rule with every field.
- Applied migration.
- Verified the rule was still the same.
- Pushed a bunch of commits and verified transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13601
Summary: Ref T8726. Just (pre-commit content) one more left.
Test Plan:
- Created a big rule with every field.
- Migrated it.
- Verified the rule was still the same.
- Pushed a bunch of changes and reviewed the transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13600
Summary: Ref T8726. Make all the DifferentialRevision stuff modular.
Test Plan:
- Created a rule with all fields.
- Ran upgrade.
- Saw all fields preserved with new modular versions.
- Used test console to run rule with all fields, verified field values as broadly sensible.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13598
Summary: Ref T8726. This deals with all the Differential diff fields, same deal as previous changes.
Test Plan:
- Wrote a rule with every field.
- Migrated it.
- Saw the same rule working.
- Rigged the hell out of transcripts (diffs normally do not generate transcripts, because the only action is "block" and they don't exist yet when Herald runs).
- Verified that all fields looked sensible in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13590
Summary: Ref T8726.
Test Plan:
Created a giant rule with every commit field:
{F594686}
Ran the upgrade, got the same rule with new fields:
{F594688}
Used "Test Console" to run transcripts, saw all the fields populate correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13567
Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary: Ref T8726. Use modular fields for the Pholio adapter.
Test Plan:
- Created rule using all the old fields.
- Migrated, saw upgrade apply correctly.
- Created mock, reviewed transcript.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13513
Summary:
Ref T8726. The only notable bit here is that the "body" / "title" fields (which are currently shared across a bunch of types) are getting split into application variants.
Among other things, this will let us label the field "Commit message" for commits, for example.
Test Plan:
- Created a rule using all four fields.
- Applied patch, saw rule break ("unknown field").
- Ran storage upgrade, saw rule fix itself in the migration.
- Edited tasks, triggered rule, viewed transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13501
Summary: Ref T8726. These are a bit involved because they have custom rendering and editor values.
Test Plan: Created new rule using these fields, edited tasks to trigger them, viewed transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13500
Summary:
Ref T8726. There's no interface we can check for this, so the adapter needs to opt in.
Also fix a spelling mistake.
Test Plan: Created rules with "Application Email" fields.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13499
Summary: Ref T8726. No clue what this was.
Test Plan: Grepped for usage, found none.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13498
Summary: Ref T8726. The existing implementation is largely made redundant by the newer modular implementation.
Test Plan: Created a custom field rule, set custom field to various values until it matched, saw effect and proper transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13497
Summary: Ref T8726. Continue making Herald fields more modular than they currently are.
Test Plan:
- Created a rule using all the affected fields.
- Ran the rule.
- Saw reasonable object field values.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13495
Summary:
Ref T8726. This lays groundwork for modularizing Herald fields and modularizes the "Content source" field.
This doesn't touch conditions or values yet.
Test Plan: Created a rule with a "content source" field, created a new task, saw rule apply.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13488
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: Ref T8498. Allow Herald rules to act on the Space which contains an object.
Test Plan:
- Wrote a "Space is any of..." rule, created tasks that matched and failed the rule.
- Also created a Pholio rule with the "Space..." condition.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8498
Differential Revision: https://secure.phabricator.com/D13242
Summary:
Ref T8455. Use standard effects for revisions, instead of a custom effect.
This fixes the major issue (conduit error) in T8455 because the standard effect now performs PHID type filtering.
This retains other behaviors (in particular: not re-CC'ing explicitly removed CCs).
Test Plan:
- With a Herald rule that adds a mailing list as a CC, created a revision before the change and hit the error in T8455. After the change, saw correct behavior.
- Wrote a normal Herald rule to add CCs and created a revision, saw it fire properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13183
Summary: Ref T8455. Use the standard effect in task rules, instead of a custom effect.
Test Plan: Wrote a Maniphest CC rule, updated a task, saw rule activate properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13182
Summary: Ref T8455. Use the standard actions in commit rules, instead of a custom action.
Test Plan: Wrote an "add cc" Herald rule for commits, pushed a commit, saw the rule fire correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13181
Summary:
Ref T8455. Begins consolidating the code for applying these effects:
- Makes Add/Remove subscribers a standard effect, and uses it in Pholio.
- This includes the "don't re-subscribe users who have explicitly unsubscribed" logic from Differential in the standard effect. I think this rule is always desirable.
- This adds new filtering of invalid PHID types to resolve the `arc diff` issue in T8455 once Differential uses this standard effect.
- Added "Remove Subscribers" to MockAdapter in order to test that it works.
- Relabeled "CC" in Pholio to "Subscribers" for consistency.
Test Plan:
- Created several rules which add subscribers to (and remove subscribers from) mocks.
- Updated mocks, changing properties and adding and removing subscribers.
- Observed transactions applying and aggregating properly.
- Observed add/remove rules each working correctly.
- Observed the "don't re-add unsubscribed users" condition acting on subscribers who had previously been added but explicitly removed/unsubscribed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13179
Summary:
Ref T8455. The Herald code in general isn't nearly as modular as it should be, and the subscriber code particularly has some legacy cruft. This is making it fragile and causing the issue described in T8455.
Currently, each Herald adapter has essentially identical code which it uses to determine which users are subscribed to an object. Instead, share code between object types.
I removed "explicitCCs":
- The value was always identical to doing the query in the common/standard way.
- They were only used to print a diagnostic message on transcripts, which I think is no longer relevant.
- I believe it predates transactions, so when it was added you couldn't figure out the old object state by looking at the transaction history. Now, CC changes are recorded there, so there's no need to restate the CC state on the transcript.
- Even if we do want to restore this (or something similar), we can do it directly from Herald now.
Test Plan:
- Created rules that use the "CCs" field in Herald, Pholio, Maniphest and Differential.
- Updated objects in each application.
- Observed valid field reads in the tranascript.
- Grepped for `FIELD_CC`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13177
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:
# Ran this rule on a differential revision and saw the rule fail in the transcript.
# Was able to submit a differential without receiving an `ERR-CONDUIT-CORE`.
# Edited the Herald rule using the UI and was able to save the rule succesfully.
# Ran this rule on a differential revision and saw one success and one failure in the transcript.
# Was able to submit a differential without receiving an `ERR-CONDUIT-CORE`.
# Edited the Herald rule using the UI. Clicking save caused a `HeraldInvalidActionException` to be thrown, but maybe this is okay.
Differential Revision: http://phabricator.local/D41
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
- Now that we have "browse", this is a much more reasonable control for random sets of things.
- The new explicit search scope selector reduces the need to fiddle with this field manually, too.
Test Plan:
{F379292}
{F379293}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12510
Summary: Fixes T7849. This adds "Remove Projects" to all objects which implement `PhabricatorProjectInterface`.
Test Plan:
- Wrote a rule to remove a project, edited task, saw the project removed.
- Admired entirely reasonable implementation complexity.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7849
Differential Revision: https://secure.phabricator.com/D12505
Summary: Ref T7849. If the adapted object implements `PhabricatorProjectInterface`, support the ADD_PROJECTS action.
Test Plan:
- Wrote a Differential rule with "Add Projects".
- Updated a revision.
- Got projects added.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7849
Differential Revision: https://secure.phabricator.com/D12504
Summary: Ref T7849. Lift more action handling out of adapters. In theory, adapters will some day do no action handling. That day is not today, but it is now a step closer.
Test Plan:
- Wrote a rule using the email and flag actions.
- Ran that rule.
- Got an email and flag.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7849
Differential Revision: https://secure.phabricator.com/D12502
Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.
In particular, this is stronger than these downgrades:
- Downgrades due to "self actions";
- downgrades due to "mail tags".
Test Plan:
- Wrote various Herald rules with "Send me an email" rules.
- Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
- Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12300
Summary:
Ref T7731. For no particular reason, we currently put `ruleID` and `rulePHID` on `HeraldEffect` objects.
Pretty much all callers need the `HeraldRule` objects instead, and some go to great lengths to get them.
Just attach the `Rule` objects.
Test Plan: Will test thoroughly after next-ish changeset.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12269
Summary:
Ref T7731. Every adapter subclass currently implements this effect in an essentially identical way.
Some day far from now the effects will be modular and this mess will vanish completely, but reduce its sprawl for now.
Test Plan: I'll test this thoroughly at the end of the change sequence since writing rules is a pain.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12268
