Summary: Ref T2769. The `HeraldRule` class has some query logic; move it into `HeraldRuleQuery`. Also some minor cleanup.
Test Plan: Ran test console, created a new revision, used `reparse.php --herald`. Verified rules triggered correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6689
Summary: Ref T2769. I'm planning to keep this pretty simple, but we have this ad-hoc edit log for rules already and some other mess that we can clean up.
Test Plan: No effect yet; see future changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2769
Differential Revision: https://secure.phabricator.com/D6654
Summary:
Ref T603. Ref T2769. Herald currently interacts with policies in a bad way; specifically, I can create a rule which emails me for everything, and thus learn about objects I can't otherwise see.
This shouldn't be possible, so I'm going to reduce personal rules to have only the viewer's scope.
For global rules, I think I'm always going to let any user edit them, but make who the rule acts as part of the configuration. There will be an option to make a rule omnipotent, but only admins (or some other special subset of users) will be able to select it.
Transactions/subscriptions will provide a check against users editing global rules in ways that are bad.
Test Plan: Next diffs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2769
Differential Revision: https://secure.phabricator.com/D6649
Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.
There are a few notable cases here:
- I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
- I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
- I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.
Test Plan:
- Grepped for all PhabricatorObjectHandleData references.
- Gave them viewers.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5151
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary:
**Who can delete global rules?**: I discussed this with @jungejason. The current behavior is that the rule author or any administrator can delete a global rule, but this
isn't consistent with who can edit a rule (anyone) and doesn't really make much sense (it's an artifact of the global/personal split). I proposed that anyone can delete a
rule but we don't actually delete them, and log the deletion. However, when it came time to actually write the code for this I backed off a bit and continued actually
deleting the rules -- I think this does a reasonable job of balancing accountability with complexity. So the new impelmentation is:
- Personal rules can be deleted only by their owners.
- Global rules can be deleted by any user.
- All deletes are logged.
- Logs are more detailed.
- All logged actions can be viewed in aggregate.
**Minor Cleanup**
- Merged `HomeController` and `AllController`.
- Moved most queries to Query classes.
- Use AphrontFormSelectControl::renderSelectTag() where appropriate (this is a fairly recent addition).
- Use an AphrontErrorView to render the dry run notice (this didn't exist when I ported).
- Reenable some transaction code (this works again now).
- Removed the ability for admins to change rule authors (this was a little buggy, messy, and doesn't make tons of sense after the personal/global rule split).
- Rules which depend on other rules now display the right options (all global rules, all your personal rules for personal rules).
- Fix a bug in AphrontTableView where the "no data" cell would be rendered too wide if some columns are not visible.
- Allow selectFilter() in AphrontNavFilterView to be called without a 'default' argument.
Test Plan:
- Browsed, created, edited, deleted personal and gules.
- Verified generated logs.
- Did some dry runs.
- Verified transcript list and transcript details.
- Created/edited all/any rules; created/edited once/every time rules.
- Filtered admin views by users.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2040
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
Allows you to write a commit rule that triggers an audit by a user (personal
rules) or a project (global rules).
Mostly this is trying to make auditing more lightweight and accessible in
environments where setting up Owners packages doesn't make sense.
For instance, Disqus wants a rule like "trigger an audit for everything that
didn't have a Differential revision". While not necessarily scalable, this is a
perfectly reasonable rule for a small company, but a lot of work to implement
with Owners (and you'll get a lot of collateral damage if you don't make every
committer a project owner).
Instead, they can create a project called 'Unreviewed Commits' and write a rule
like:
- When: Differential revision does not exist
- Action: Trigger an Audit for project: "Unreviewed Commits"
Then whoever cares can join that project and they'll see those audits in their
queue, and when they approve/raise on commits their actions will affect the
project audit.
Similarly, if I want to look at all commits that match some other rule (say,
XSS) but only want to do it like once a month, I can just set up an audit rule
and go through the queue when I feel like it.
NOTE: This abuses the 'packagePHID' field to also store user and project PHIDs.
Through the magic of handles, this (apparently) works fine for now; I'll do a
big schema patch soon but have several other edits I want to make at the same
time.
Also:
- Adds an "active" fiew for /audit/, eventually this will be like the
Differential "active" view (stuff that is relevant to you right now).
- On commits, highlight triggered audits you are responsible for.
Test Plan: Added personal and global audit triggers to Herald, reparsed some
commits with --herald, got audits. Browsed all audit interfaces to make sure
nothing exploded. Viewed a commit where I was responsible for only some audits.
Performed audits and made sure the triggers I am supposed to be responsible for
updated properly.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1690
Summary: Since mailing list rules are now "global", don't run "personal" rules
for disabled/invalid users.
Test Plan: Added a personal rule that matches every revision for a test user.
Created a revision, checked transcript, rule matched. Disabled user, updated
revision, checked transcript, rule got auto-disabled.
Reviewers: btrahan, jungejason, nh, xela
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1517
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
