1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-01-15 09:11:07 +01:00
Commit graph

78 commits

Author SHA1 Message Date
epriestley
adcc4ee1db Add a "branches" rule for Herald commit rules
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
2013-12-26 10:40:16 -08:00
xiaogaozi
54a0dd8139 Herald - add support for "assignee" conditions
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
2013-12-22 08:47:56 -08:00
epriestley
e27bbb9aa5 Enable the "Accepted Differential Revision" field for Herald post-commit hooks
Summary: I implemented this field, but didn't actually enable it.

Auditors: btrahan
2013-12-21 11:11:52 -08:00
epriestley
a64d127e25 Add "is merge commit" Herald field for pre-commit rules
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
2013-12-20 12:39:40 -08:00
epriestley
72c73d644b Add an "Accepted Differential revision" field to Commit and pre-commit Content Herald rules
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
2013-12-20 12:39:13 -08:00
epriestley
2436458b90 Implement "Differential Revision" fields in Herald pre-commit content adapter
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
2013-12-20 12:39:01 -08:00
epriestley
d64dd7e2e8 Allow global Commit herald rules to trigger audits by users
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
2013-12-19 10:33:15 -08:00
epriestley
3386920971 Add Herald support for blocking ref changes
Summary: Ref T4195. Allows users to write Herald rules which block ref changes. For example, you can write a rule like `alincoln can not create branches`, or `no one can push to the branch "frozen"`.

Test Plan:
This covers a lot of ground. I created and pushed a bunch of rules, then looked at transcripts, in general. Here are some bits in detail:

Here's a hook-based reject message:

  >>> orbital ~/repos/POEMS $ git push
  Counting objects: 5, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (3/3), done.
  Writing objects: 100% (3/3), 274 bytes, done.
  Total 3 (delta 2), reused 0 (delta 0)
  remote: +---------------------------------------------------------------+
  remote: |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
  remote: +---------------------------------------------------------------+
  remote:             \
  remote:              \                    ^    /^
  remote:               \                  / \  // \
  remote:                \   |\___/|      /   \//  .\
  remote:                 \  /V  V  \__  /    //  | \ \           *----*
  remote:                   /     /  \/_/    //   |  \  \          \   |
  remote:                   @___@`    \/_   //    |   \   \         \/\ \
  remote:                  0/0/|       \/_ //     |    \    \         \  \
  remote:              0/0/0/0/|        \///      |     \     \       |  |
  remote:           0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
  remote:        0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
  remote:                    ,-}        _      *-.|.-~-.           .~    ~
  remote:   \     \__/        `/\      /                 ~-. _ .-~      /
  remote:    \____(Oo)           *.   }            {                   /
  remote:    (    (--)          .----~-.\        \-`                 .~
  remote:    //__\\  \ DENIED!  ///.----..<        \             _ -~
  remote:   //    \\               ///-._ _ _ _ _ _ _{^ - - - - ~
  remote:
  remote:
  remote: This commit was rejected by Herald pre-commit rule H24.
  remote: Rule: No Branches Called Blarp
  remote: Reason: "blarp" is a bad branch name
  remote:
  To ssh://dweller@localhost/diffusion/POEMS/
   ! [remote rejected] blarp -> blarp (pre-receive hook declined)
  error: failed to push some refs to 'ssh://dweller@localhost/diffusion/POEMS/'

Here's a transcript, showing that all the field values populate sensibly:

{F90453}

Here's a rule:

{F90454}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7782
2013-12-17 15:23:55 -08:00
epriestley
6b1ec35cf3 Allow Herald rules to check for revisions with no reviewers
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
2013-12-11 14:46:39 -08:00
Bob Trahan
f35ce505a1 Herald - add ability to conditionalize on Maniphest Task projects
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
2013-11-11 14:20:31 -08:00
James Rhodes
7ffea0463e Use herald to trigger builds of revisions and commits.
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
2013-11-08 16:58:39 -08:00
epriestley
038f5323c0 Allow Herald's "Reviewers" field to select project reviewers
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
2013-11-07 11:23:31 -08:00
epriestley
2c3d071a26 Improve exception behavior for Herald commit rules which fail to load diff context
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
2013-10-23 08:28:58 -07:00
epriestley
c6f9316a77 Add missing case for personal ruls which create blocking reviewers
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
2013-10-07 11:18:00 -07:00
epriestley
c4ecdfa2a5 Fix missing property on revision adapter. 2013-10-07 03:41:00 -07:00
epriestley
8aa8ef49da Provide an "Add blocking reviewer..." Herald action
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
2013-10-06 17:09:24 -07:00
epriestley
64e4b3aef4 Remove loadMemberPHIDs from PhabricatorProject
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
2013-10-06 17:07:08 -07:00
epriestley
b7297a3278 Add an "Author's projects" Herald field to Differential
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
2013-10-05 14:10:53 -07:00
epriestley
4c0ec01ce5 Allow Herald rules to add reviewers
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
2013-10-05 14:10:51 -07:00
epriestley
e6d8e1a00a Make Herald rules obey policies during application
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
2013-10-05 12:55:34 -07:00
Juan Pablo Civile
562da7e98b Stop using loadRelationships in differential related herald adapters
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
2013-10-04 16:40:42 -07:00
epriestley
ee4bdb501b Make Herald transcripts policy-aware
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
2013-10-04 15:17:18 -07:00
epriestley
c8127edfe9 Tighten up some policy interactions in Herald
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
2013-10-04 15:15:48 -07:00
Neal Poole
1edb875978 Adding support for 'adds' and 'removes' in diff content.
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
2013-10-04 06:37:39 -07:00
Bob Trahan
3cf17cc67f Herald - add field + condition for Diffusion Commits for "On autoclose branch"
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
2013-10-03 17:53:12 -07:00
Bob Trahan
5ab15b51a3 Herald - tighten up rule display
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
2013-09-27 12:05:58 -07:00
epriestley
874a9b7fe3 When creating or updating a revision, infer the repository from the diff
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
2013-09-26 15:28:42 -07:00
Bob Trahan
fb4c9b6345 Maniphest + Herald - add support for assigning tasks and adding projects
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
2013-09-26 15:04:55 -07:00
Bob Trahan
477d4e9db1 Herald - add support for "content source" conditions
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
2013-09-26 14:20:56 -07:00
epriestley
9b3d7b0dba Make most Differential reads policy-aware
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
2013-09-26 12:37:19 -07:00
epriestley
c467cc464f Make most repository reads policy-aware
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
2013-09-25 16:54:48 -07:00
Bob Trahan
660f1f3e64 Add herald support for Maniphest
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
2013-09-25 11:50:15 -07:00
Bob Trahan
0ce85df708 Make Herald have "exists" and "not exists" options for differential reviewers on commit rules
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
2013-09-12 16:00:09 -07:00
Bob Trahan
b902005bed Kill PhabricatorObjectDataHandle
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
2013-09-11 12:27:28 -07:00
epriestley
b932c606cb Implement "Add to CC" rule for Audits
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
2013-08-29 17:03:40 -07:00
Eric Stern
9a95b1e7d1 add committer as supported field for herald rules
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
2013-08-29 06:07:53 -07:00
Bob Trahan
f909a295f7 Integrate Pholio with Herald
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
2013-08-15 13:10:45 -07:00
Bob Trahan
ada749236a Herald - restore create an audit as an action for commit objects
Summary: I think we accidentally forgot to include this action in D6660.

Test Plan: verified it showed up in the UI to have the action be an audit

Reviewers: chad, epriestley

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6712
2013-08-09 13:01:00 -07:00
epriestley
4f49ec1cff Remove HeraldDryRunAdapter
Summary: Ref T2769. This isn't a real adapter and its methods are increasingly hacky messes. Make "dry run" a first-class concept on the HeraldEngine instead and remove the adapter.

Test Plan: Ran Herald via test console and via CLI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6693
2013-08-07 18:04:40 -07:00
epriestley
ce163536ca Add a normal "view" page for Herald rules
Summary:
Ref T2769. This will house the transaction list and replace the "edit log" stuff.

The UI is a little bit rough and can probably share more code with the transaction history, but seems mostly-reasonable.

Test Plan: {F53253}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6690
2013-08-07 18:04:39 -07:00
epriestley
b767bd3f2d Move Herald rule querying into HeraldRuleQuery
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
2013-08-07 18:04:38 -07:00
epriestley
75e43513c2 Remove HeraldActionConfig, HeraldFieldConfig
Summary:
Ref T2769. Move all of this stuff into Adapters and get rid of the hard-coded classes.

I cheated in two places.

Test Plan: Edited and activated Herald rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6688
2013-08-07 18:04:37 -07:00
epriestley
2c2fcc58ca Remove HeraldConditionConfig
Summary: Ref T2769. Moves all traces of HeraldConditionConfig into Adapters.

Test Plan: Edited rules and used Test Console to exercise both affected code paths. Tried to save invalid rules to hit error pat.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6679
2013-08-07 18:04:36 -07:00
epriestley
ca66eeb07c Remove HeraldContentTypeConfig and move repetition to Adapters
Summary: Ref T2769. Get rid of the last use of `HeraldContentTypeConfig` by moving repetition options into Adapters.

Test Plan: Viewed / edited Herald rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6664
2013-08-07 18:04:35 -07:00
epriestley
0640931d30 Use Adapters to render Herald transcripts
Summary: Ref T2769. Use Adapters to build all the strings for transcripts, then get rid of the old maps.

Test Plan: Viewed revision and commit transcripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6663
2013-08-07 18:04:34 -07:00
epriestley
78eb81ffd0 Remove almost all instances of HeraldContentTypeConfig
Summary: Ref T2769. This cleans up almost every use of the HeraldContentTypeConfig class.

Test Plan: Viewed and edited Herald rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6662
2013-08-07 18:04:33 -07:00
epriestley
9c637604a2 Move most Herald actions and values into dynamic adapters
Summary: Ref T2769. Shift the bulk of value and action config into Adapters.

Test Plan: Viewed and edited Herald list and rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, vrana

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6660
2013-08-07 18:03:54 -07:00
epriestley
2e87f9f53c Move most Herald condition config into dynamic adapters
Summary: Ref T2769. Pushes most condition configuration into Adapters, out of the hard-coded class.

Test Plan: Looked at, edited, and dry-run'd Herald rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6658
2013-08-07 18:03:53 -07:00
epriestley
3490b6dd11 Move most Herald field configuration into dynamic Adapters
Summary: Ref T2769. Herald has a giant hard-coded list of fields. Primarily make these dynamic and adapter-based.

Test Plan: Viewed and edited Herald rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6657
2013-08-07 18:03:52 -07:00
epriestley
6badb05d64 Make Herald adapters provide content types
Summary:
Ref T2769. Get content types out of hard-coded config and into dynamic adapters.

This removes the "MERGE" and "OWNERS" content types, which were vestigal. These needs are likely better addressed through subscriptions/transactions, and are obsolete, and haven't existed for 2+ years and no one has asked for them to be restored.

Test Plan: Mostly a bunch of grep. Viewed rule list, rule edit. Edited a revision.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6656
2013-08-07 18:03:51 -07:00