Summary:
Add a very basic edit history table to herald rules. This table is updated
whenever saving a herald rule. The contents of the save are not examined, and
the edit history contains no information about the rule itself *yet*. Edit
history can be viewed by anyone through /herald/history/<rule id>/.
Task ID: #
Blame Rev:
Test Plan:
Made a test rule, saved some stuff.
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: zizzy, aran, xela, epriestley
Differential Revision: https://secure.phabricator.com/D1387
Summary:
- Only write the <ruleID, phid> row if the rule is a one-time rule.
- Delete all the rows for rules which aren't one-time.
NOTE: This is probably like several million rows for Facebook and could take a
while.
Test Plan:
Added some one-time and every-time rules, ran them against objects, verified
only relevant rows were inserted.
Ran upgrade script against a database with one-time and every-time "ruleapplied"
rows, got the irrelevant rows removed.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1484
Summary:
Herald rules may be marked as "one-time". We track this by writing a row with
<ruleID, phid> when we apply a rule.
However, the current test for rule application involves loading every <ruleID,
*> pair. We also always write this row even for rules which are not one-time, so
if there are 100 rules, we'll load 1,000,000 rows after processing 10,000
objects.
Instead, load only the <phid, *> pairs, which are guaranteed to be bounded to at
most the number of rules.
I'll follow up with a diff that causes us to write rows only for one-time rules,
and deletes all historic rows which are not associated with one-time rules.
Test Plan:
Grepped for callsites to loadAllByContentTypeWithFullData(). Ran
rules in test console.
Reviewers: nh, btrahan, jungejason
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1483
Summary:
A personal rule only has actions targeting the owner. Likewise, only they can
edit the rule. OTOH, a global may affect any target and is editable by anyone.
There are no new action types. Instead, type of the rule modifies the available
targets and the messaging in the ui. This is beneficial because herald rule
adapters don't need to be aware of the difference between emailing the owner of
a personal rule and emailing an arbitrary user.
This diff sets up the logic and ui for creating personal/global rules. All
existing rules have been defaulted to global.
TODO: Filter all existing rules into personal/global
TODO: Create a UI for surfacing (relevant?) global rules.
Test Plan:
1. Created a personal rule to email myself. Created a dumby revision satisfying
the conditions of that rule. Verified that I recieved a herald email.
2. Removed my adminship, change the owner of a personal rule. verified that I
couldn't edit the rule.
3.Changed rule type to global. verified that I could edit the rule.
4. Verified that admins can edit both global and personal rules.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, zizzy
Differential Revision: https://secure.phabricator.com/D1449
Summary:
See T354. List every rule which has ever been applied in X-Herald-Rules, not
just the ones which most recently triggered.
Also some random fixes while I was debugging this:
- When conduit methods throw non-conduit exceptions, make sure they get
logged.
- Trigger the Facebook "tasks" backcompat block only if we were going to fail
(this should reduce the shakniess of the transition).
- Fix some log spew from the new field stuff.
Test Plan:
- Created a rule (ID #3) "No Zebras" which triggers for revisions without
"zebra" in the title.
- Created a revision without "zebra" in the title, got X-Herald-Rules: <2>,
<3>
- Updated revision to have "zebra" in the title, verified rule did not trigger
in Herald transcript.
- Verified X-Herald-Rules is still: <2>, <3>
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 817
Summary:
This is pretty subtle and tricky, but some apply transcripts don't have a rule
ID because they're purely informational. We currently get an exception, which
prevnets diff updates.
jason/tuomas: don't update phabricator.fb.com until this lands :P
Test Plan:
Applied this patch live to secure.phabricator.com and was able to update D420.
Reviewed By: gc3
Reviewers: gc3, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, gc3
Differential Revision: 421
Summary:
- added a new config class for representing the kind of repetition a rule has
(once, every time, first time only)
- added an email action to herald rules for differential to allow someone to get
an email but only the first one
- changed the herald rule ui to allow a user to pick the amount of repetition
Test Plan:
created a test rule and ran it over and over
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley, gc3
Revert Plan:
Tags:
- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -
Differential Revision: 357
Summary:
This isn't terribly elegant but it solves the problem without loss of
generality. We can pursue a more finessed solution later if it seems prudent.
Test Plan:
Created a revision matched by a blanket herald rule, and then commented on it.
Comment email had X-Herald-Rules header in it.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 218
