Summary: ...and surface it in all adapters except commit adapters. Values are true or false. Ref T4294
Test Plan: made a herald rule to be cc'd on new tasks. was cc'd on new tasks and not cc'd on updates to existing tasks.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8142
Summary:
Fixes T1353. Also some minor unrelated cleanup:
- `openTransaction()` / `saveTransaction()` exist now, fix TODOs.
- Fix some instructions.
- Make `diffusion.branchquery` return empty for SVN rather than fataling.
Test Plan:
- Added a branches rule.
- Ran a dry run against commits in different VCSes.
{F105574}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, Nopik
Maniphest Tasks: T1353
Differential Revision: https://secure.phabricator.com/D8086
Summary: See IRC. This is a likely fix for @DctrWatson's error: we're returning `null` but should return `array()` to indicate no values.
Test Plan: Will make @DctrWatson do it.
Reviewers: btrahan
Reviewed By: btrahan
CC: dctrwatson, aran
Differential Revision: https://secure.phabricator.com/D7950
Summary: This also cleans up some code a little bit. Most of the gymnastics are to make sure we call `needProjectPHIDs()` appropriately.
Test Plan: Created new commit and revision rules with this field. Ran commits and revisions through the test console. Field behavior seemed correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7923
Summary: use the loadReviewedBy function, which seems to do what we want -- returns a reviewer IFF the last thing was an accept
Test Plan: i believe in the power of loadReviewedBy
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, aarwine
Differential Revision: https://secure.phabricator.com/D7903
Summary: "Run" is clearer than "Apply". This has already been changed in Harbormaster itself.
Test Plan: used eyeballs
Reviewers: btrahan, zeeg
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7889
Summary:
Fixes T4276. This adds "Change is enormous" to pre-commit content rules so we can, e.g., just reject these and not worry about them elsewhere.
Also, use the same numeric limits across the mechanisms so there's a consistent definition of an "enormous" changeset.
Test Plan:
- Set enormous limit to 15 bytes, pushed some changes, got blocked by a rule.
- Set it back, pushed OK.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7887
Summary:
Ref T4276. When a change is larger than 2GB, PHP can not read the entire change into a string, so Herald can not process it.
Additionally, we already have a time limit for practical reasons, but it's huge (probably incorrectly). To deal with these things:
- Add an optional byte limit to `diffusion.rawdiffquery`.
- Make the query with a 1GB limit.
- Reduce the diff timeout from 15 hours to 15 minutes.
- Add a "Changeset is enormous" field. This field is true for changes which are too large to process.
This generally makes behaviors more sane:
- We'll always make progress in Herald in a reasonable amount of time.
- Installs can write global rules to handle (or reject) these types of changes.
Test Plan: Set limit to 25 bytes instead of 1GB and ran test console on various changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7885
Summary:
Fixes T4264. Adds:
- New "Repository's projects" field to Herald pre-commit rules, so you can write global rules which act based on projects.
- Allows pre-ref/pre-content rules to bind to projects, and fire for all repositories in that project, so users with limited power can write rules which apply to many repositories.
- The pre-ref and pre-content classes were starting to share a fair amount of code, so I made them both extend an abstract base class.
Test Plan: Wrote new pre-ref and pre-content rules bound to projects, then pushed commits into repositories in those projects and not in those projects. The "repository projects" field populated, and the rules fired for repositories in the relevant projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7883
Summary:
Ref T4264. Allows you to create "Object" rules, in addition to Global and Personal rules. If you choose to create an Object rule, you'll be prompted to select an object on a new screen. You must be able to edit and object in order to create rules for it.
Ref T3506. This makes "All" the default filter for the transcript view, which should reduce confusion on smaller installs.
Test Plan:
- Created non-object rules.
- Created object rules.
- Triggered object rules against matching and unmatching objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3506, T4264
Differential Revision: https://secure.phabricator.com/D7853
Summary: Ref T4264. Lays the groundwork for new "Object" rule types. Prevents personal "Hook" rules, which don't make any sense.
Test Plan: Created new Maniphest (global/personal available) and Ref Hook (global only) rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7852
Summary: Ref T4264. Instead of a dropdown, make this step more informative.
Test Plan: {F93928}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7846
Summary:
Ref T4195. A legitimate rule which needs this field is "do not allow commits as root". Interestingly, we have exactly one commit as root in each Phabricator, Arcanist and libphutil.
Since the committer and author don't need to be Phabricator accounts (just the Pusher), the existing "Committer" and "Author" fields can't express this rule (they'll be empty).
Test Plan: {F93406}
Reviewers: btrahan
Reviewed By: btrahan
CC: SEJeff, aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7841
Summary:
Fixes T4195. Allows you to write a rule against a commit's branches.
This completes outstanding work on T4195.
Test Plan: Pushed to Git and Mercurial repositories and verified branches were selected correctly by examining transcripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7820
Summary: add support for "assignee" conditions
Test Plan: Create a Herald rule where condition is assignee, and create a task assign to someone.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7813
Summary:
Ref T4195. This allows you to write rules which disallow merge commits.
Also make the reject message a little more useful.
Test Plan:
remote: This push was rejected by Herald push rule H27.
remote: Change: commit/daed0d448404
remote: Rule: No Merges
remote: Reason: No merge commits allowed. If you must push a merge, include "@force-merge" in the commit message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7809
Summary: Refs T4195. Fixes T3936. You can't currently write rules like "block commits unless they're attached to an **accepted** revision"; allow that.
Test Plan: Pushed commits into a rule with this field, saw it work / not crash.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T3936, T4195
Differential Revision: https://secure.phabricator.com/D7807
Summary: Ref T4195. Allows you to write revision-based commit hooks, e.g. block all commits with no corresponding revision.
Test Plan:
Here's are the fields populating:
{F90989}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7806
Summary: Ref T4249. Currently, a global rule can only trigger project audits. Although there probably aren't a huge number of use cases for triggering users from global rules, it works fine and it's somewhat confusing not to allow it.
Test Plan: {F90902}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4249
Differential Revision: https://secure.phabricator.com/D7803
Summary: Fixes T4225. Adds the NON_EXISTS condition to Herald for "Reviewers", and adds a few more conditions which have reasonable meanings.
Test Plan: Used test console to check a revision with reviewers, and another without reviewers. Both produced the expected results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4225
Differential Revision: https://secure.phabricator.com/D7757
Summary: adds FIELD_PROJECTS and deploys it to Maniphest Task Herald Adapter. Went with "projects" because it feels like that could go well in other Adapters that want to conditionalize based on project.
Test Plan: made a new herald rule to be cc'd if project foo was on a task. it worked!
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7564
Summary:
Depends on D7500.
This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).
Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, hwinkel
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7501
Summary: The "Reviewers" condition in Differential Revision rules has the wrong typeahead and can't select projects, but should be able to.
Test Plan: {F79273}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7526
Summary:
This code is a little funky right now, and can return `array("error message")` and then try to call `getHunks()` on it. Additionally, each field loads the commit's changes separately.
Instead, load the commit's changes once and cache them, and handle exceptions appropriately.
Test Plan:
- Created a rule like "changed, added, removed content all match /.*/" to force all fields to generate.
- Ran it successfully.
- Faked an error and ran it, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: bigo, aran
Differential Revision: https://secure.phabricator.com/D7384
Summary: Ref T1279. I only tested the global case. :O
Test Plan: Created a personal "add me as blocking" rule.
Reviewers: btrahan, zeeg
Reviewed By: zeeg
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7261
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.
Test Plan: Added Herald rules and updated revisions; see screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7244
Summary:
Ref T603. Move toward stamping out all the Project / ProjectProfile query irregularities with respect to policies.
- Fixes a bug with Asana publishing when the remote task is deleted.
- Fixes an issue with Herald commit rules.
Test Plan:
- Viewed projects;
- edited projects;
- added and removed members from projects;
- republished Asana-bridged feed stories about commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7251
Summary:
Ref T1279. This allows installs to implement two different flavors of project review. They can either implement this rule:
When:
[ ... ] [ ... ]
Take Action:
[ Add blockign reviewers ] [ Security ]
...which means "every revision matching X needs to be signed off by someone else on the Security team, //even if the author is on that team//". The alternative is to implement this rule:
When:
[ Author's projects ] [ do not include ] [ Security ]
[ ... ] [ ... ]
Take Action:
[ Add blocking reviewers ] [ Security ]
...which means that people on the Security team don't need a separate signoff from someone else on the team.
I think this weaker version maps to some of what, e.g., Google does (you need to be reviewed by someone with "readability" in a language, but if you have it that's good enough), but I could imagine cases like "Security" wanting to prevent self-review from satisfying the requirement.
@zeeg, not sure which of these use cases is relevant here, but either one should work after this.
Test Plan: Created rules with this field, verified it populated properly in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7238
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.
I will probably write some lengthy treatise on how you shouldn't use this rule later.
Implementation is straightforward because Differential previously supported this rule.
This rule can also be used to add project reviewers.
Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7235
Summary:
Ref T603. This closes the other major policy loophole in Herald, which was that you could write a rule like:
When [Always], [Add me to CC]
...and end up getting email about everything. These rules are now enforced:
- For a //personal// rule to trigger, you must be able to see the object, and you must be able to use the application the object exists in.
- In contrast, //global// rules will //always// trigger.
Also fixes some small bugs:
- Policy control access to thumbnails was overly restrictive.
- The Pholio and Maniphest Herald rules applied only the //last// "Add CC" or "Add Project" rules, since each rule overwrote previous rules.
Test Plan:
- Created "always cc me" herald and maniphest rules with a normal user.
- Created task with "user" visibility, saw CC.
- Created task with "no one" visibility, saw no CC and error message in transcript ("user can't see the object").
- Restricted Maniphest to administrators and created a task with "user" visibility. Same deal.
- Created "user" and "no one" mocks and saw CC and no CC, respectively.
- Thumbnail in Pholio worked properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7224
Summary:
Used `DifferentialRevisionQuery` with the relevant `need*()` calls in the test controller.
And started assuming the revision has reviewers and CC phids in `HeraldDifferentialRevisionAdapter`.
Test Plan:
Added herald rules that use revisions (one for revisions another for commit) and reviewers.
Created, accepted and landed a revision that matched the rules and checked all rules were applied.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6468
Conflicts:
src/applications/herald/adapter/HeraldCommitAdapter.php
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
src/applications/herald/controller/HeraldTestConsoleController.php
Summary:
Ref T603. Herald transcripts potentially leak a bunch of content (task text, revision/commit content). Don't let users see them if they can't see the actual objects.
This is a little messy but ends up mostly reasonable-ish.
Test Plan:
- Verified that transcripts for objects I couldn't see no longer appear in the list, and reject access.
- Verified that transcripts for objects in applications I can't see reject access, albeit less gracefully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7221
Summary:
Ref T603. Herald is a bit of a policy minefield right now, although I think pretty much everything has straightforward solutions. This change:
- Introduces "create" and "create global" permisions for Herald.
- Maybe "create" is sort of redundant since there's no reason to have access to the application if not creating rules, but I think this won't be the case for most applications, so having an explicit "create" permission is more consistent.
- Add some application policy helper functions.
- Improve rendering a bit -- I think we probably need to build some `PolicyType` class, similar to `PHIDType`, to really get this right.
- Don't let users who can't use application X create Herald rules for application X.
- Remove Maniphest/Pholio rules when those applications are not installed.
Test Plan:
- Restricted access to Maniphest and uninstalled Pholio.
- Verified Pholio rules no longer appear for anyone.
- Verified Maniphest ruls no longer appear for restricted users.
- Verified users without CREATE_GLOBAL can not create global ruls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7219
Summary:
Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'.
- There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant.
- The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally.
Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3829
Differential Revision: https://secure.phabricator.com/D7214
Summary:
Fixes T1461.
Adds
- FIELD_ALWAYS - now you could add this to a content type to always get notified
- FIELD_REPOSITORY_AUTOCLOSE_BRANCH - solves T1461
- CONDITION_UNCONDITIONALLY - used by these two fields to not show any value for the user to select
Test Plan: made a herald rule where diffs on autoclose branches would get flagged blue. made a diff on an autoclose branch and committed it. commit was flagged!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1461
Differential Revision: https://secure.phabricator.com/D7210
Summary: we were bad at displaying phid-based values nicely. Now we are good at it.
Test Plan: made a herald rule where if the author was a or b, the task should be assigned to c and have projects x, y, z added to it. this displayed nicely.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7158
Summary:
Ref T603. When a diff is attached to a revision, try to guess the repository if possible. In cases where we succeed, this automatically gives us intuitive policy behavior (i.e., you can see a revision if you can see the repository the change is against).
I pulled this into a funky little "Lookup" class for two reasons:
- It's used in two places;
- I anticipate that we might need to add some sort of `explainWhy()` method if users find the heuristics confusing.
Test Plan: Created and updated revisions, saw them pick up the correct repository association. Ran Herald dry run against associable and nonassociable revisions, saw correct values populate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7147
Summary: This ends up living in HeraldAdapter even though its "task only" stuff. Reason being There are 4 or 5 functions that have little hooks; see diff. Ref T1638.
Test Plan: made a rule to assign tasks to me if made on web - great success. made a rule to assign tasks to other guy and add a project if title contained "foobar" - great success, including some confusion as ther two herald rules fought each other for task ownership.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1638
Differential Revision: https://secure.phabricator.com/D7146
Summary: ...and deploy on Maniphest. Ref T1638.
Test Plan: created a herald rule to be cc'd for tasks created via web. made a task via web and another via email and was cc'd appropriately. edited the herald to be cc'd for tasks created via not web. made 2 tasks again and got cc'd appropriately
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1638
Differential Revision: https://secure.phabricator.com/D7145
Summary: Ref T603. Makes the majority of reads policy aware (and pretty much all the important ones).
Test Plan:
- Created a comment with `differential.createcomment`.
- Created a new revision with `arc diff` in order to exercise `differential.creatediff`.
- Created an inline comment with `differential.createinline`.
- Added a comment to a revision.
- Edited an inline comment.
- Edited a revision.
- Wrote "Depends on ..." in a summary, saved, verified link was created.
- Browsed a file in Diffusion.
- Got past the code I changed in the Releeph request thing.
- Edited a Releeph request.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7136
Summary: Ref T603. This swaps almost all queries against the repository table over to be policy aware.
Test Plan:
- Made an audit comment on a commit.
- Ran `save_lint.php`.
- Looked up a commit with `diffusion.getcommits`.
- Looked up lint messages with `diffusion.getlintmessages`.
- Clicked an external/submodule in Diffusion.
- Viewed main lint and repository lint in Diffusion.
- Completed and validated Owners paths in Owners.
- Executed dry runs via Herald.
- Queried for package owners with `owners.query`.
- Viewed Owners package.
- Edited Owners package.
- Viewed Owners package list.
- Executed `repository.query`.
- Viewed "Repository" tool repository list.
- Edited Arcanist project.
- Hit "Delete" on repository (this just tells you to use the CLI).
- Created a repository.
- Edited a repository.
- Ran `bin/repository list`.
- Ran `bin/search index rGTESTff45d13dffcfb3ea85b03aac8cc36251cacdf01c`
- Pushed and parsed a commit.
- Skipped all the Drydock stuff, as it it's hard to test and isn't normally reachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7132
Summary:
standing on the shoulders of the badass work to move Maniphest to ApplicationTransactions, this diff implements a few methods and adds an adapter class.
For now, we can add cc and flag tasks. I figure see what people ask for? Ref T1368.
Test Plan: created herald rules for title and description text hits. made tasks and verified CC and flags worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1368, T1638
Differential Revision: https://secure.phabricator.com/D7122
Summary: Fixes T1485.
Test Plan: made a herald rule for "not exists". committed to master with no diff. audit was triggered
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1485
Differential Revision: https://secure.phabricator.com/D6964
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))
Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, FacebookPOC
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6941
Summary:
D6660 accidentally allowed you to build Herald rules for commits that take action "Add to CC", but provided no implementation.
Someone at Facebook then wrote such a rule.
Fix forward since there's no real reason not to allow this.
Test Plan: Used `./scripts/repository/reparse.php --herald rXnnnn` to trigger rules. Observed rule trigger and subsequent subscription.
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6845
Summary: Allows building Herald rules against committer, similar to author. Useful for monitoring cherry-picked commits.
Test Plan: Applied patch, restarted php-fpm and phd daemons to ensure code changes took effect. Added a new herald rule to trigger audit when committer was me. Cherry-picked someone else's commit (author=them, committer=me) and pushed to origin. Audit was triggered.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6838
Summary: Ref T2766. Does the integration via ApplicationTransactionsEditor. Only did addCC and Flag for proof of concept.
Test Plan: Made a rule to cc, made a rule to flag. They worked! (will attach screens to diff)
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2766
Differential Revision: https://secure.phabricator.com/D6766
