1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-13 09:06:14 +01:00
Commit graph

443 commits

Author SHA1 Message Date
austinkelleher
2e5065feb5 Update function name to follow naming convention.
See: <http://github.com/facebook/phabricator/pull/575>

Reviewed by: epriestley
2014-04-20 08:37:37 -07:00
epriestley
6899fbcf29 Add DifferentialHunkQuery to start hiding hunk storage details
Summary:
Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.

This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.

Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4045

Differential Revision: https://secure.phabricator.com/D8765
2014-04-14 12:06:26 -07:00
epriestley
aaf1320b02 Simplify Herald logic for loading Differential changes
Summary: Ref T4045. These three methods are fairly copy-pastey. Provide a more formal DifferentialHunk API for querying various types of line ranges.

Test Plan: Used test console to verify that "added content", "removed content", and "changed content" rules still produce the same data.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4045

Differential Revision: https://secure.phabricator.com/D8764
2014-04-14 12:06:20 -07:00
Bob Trahan
4b56dbed3a Herald - make tokenizers have the purdy icons
Summary: ...use the prefab stuff as it does fancier things than we were doing. Only trick then really is to pass username and the map of handle phids => icons to the client so prefab can work nicely. Fixes T4775.

Test Plan: made a herald rule with projects and users. Saw nice icons. Reloaded page and still saw nice icons.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4775

Differential Revision: https://secure.phabricator.com/D8749
2014-04-10 12:38:15 -07:00
Bob Trahan
d5ded805b2 Herald - fix change type bug
Summary: wasn't working due to some type issues. Fixes T4756. I also made it display nicer while I was debugging this.

Test Plan: created a herald rule to block changes that added refs. git tag -a "test" -m "test test"; git push origin test got me blocked!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4756

Differential Revision: https://secure.phabricator.com/D8724
2014-04-08 11:58:28 -07:00
epriestley
e3b5737d02 Support CustomField in Herald, mostly
Summary: Ref T655. Ref T418. This mostly supports CustomFields in Herald, for conditions only.

Test Plan: {F137845}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T418, T655

Differential Revision: https://secure.phabricator.com/D8695
2014-04-03 18:43:49 -07:00
Bob Trahan
b50426a98f Herald - print out rule monogram rather than rule phid on transcript controller
Summary: Fixes T4632.

Test Plan: viewed a transcript for rule x which depends on rule y and noted "rule y" printed out rather than "PHID-BLAH-BLAH"

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4632

Differential Revision: https://secure.phabricator.com/D8678
2014-04-02 11:59:50 -07:00
epriestley
1aad40b7bf Allow users to receive email about pushes via Herald
Summary:
Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries.

For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed.

Overall, this is basically the same as commit email, but more suitable for some workflows.

Test Plan:
Wrote some rules, then made a bunch of pushes. Got email like this:

{F134929}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8618
2014-03-26 13:51:15 -07:00
epriestley
03c6bf0d09 Make Herald less ambitious about resolving repositories for revisions
Summary:
Fixes T4636. If a user manually deletes a "repository" setting from a revision, Herald attempts to resolve it. Instead, Herald should now just trust Differential. Generally, the new logic is:

  - When diffs are created, figure out repository information.
  - When revisions are updated, copy info from diffs.
  - Everywhere else, just trust the revision field.

Test Plan:
  - Created revisions.
  - Used Herald to dry-run revisions before and after a manual edit to remove the repository setting.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4636

Differential Revision: https://secure.phabricator.com/D8576
2014-03-21 14:39:56 -07:00
epriestley
7167a729bf Fail more gracefully when rendering transcripts if handle is missing
Summary: Fixes T4628. I can only partially reproduce the root cause here, but if transcript display rules aren't quite right we should just degrade here rather than fatalling. Transcripts are a messy business by any measure.

Test Plan: Sort-of-reproing transcript renders OK now.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4628

Differential Revision: https://secure.phabricator.com/D8554
2014-03-17 15:02:10 -07:00
epriestley
38cc38eaf6 Modernize documentation links
Summary:
  - Point them at the new Diviner.
  - Make them a little less cumbersome to write.

Test Plan: Found almost all of these links in the UI and clicked them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8553
2014-03-17 15:01:31 -07:00
epriestley
f54bc8ae58 Add "Send an email" action to Herald for Maniphest
Summary: Fixes T4403. Supports the "send an email" action in Maniphest.

Test Plan: Wrote a "email duck" rule, then commented on a task and saw "duck" get an email.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4403

Differential Revision: https://secure.phabricator.com/D8529
2014-03-14 11:52:31 -07:00
Michael Peters
3f7f5a47ff Fix a small typo when creating a Herald rule
Summary: "Users who an edit" to "Users who can edit"

Test Plan: Verified that typo is gone after the change

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8511
2014-03-12 16:12:43 -07:00
epriestley
193e8a54fc Add "pusher is committer" to Herald as a pre-commit rule
Summary:
Fixes T4594. Also, allow "exists" / "does not exist" to be run against author/committer. This allows construction of rules like:

  - Committer identities must be authentic.
  - Committer identities must be resolvable.
  - Author identities must be resolvable.

Test Plan: Created some rules using these new rules and ran them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4594

Differential Revision: https://secure.phabricator.com/D8507
2014-03-12 15:24:33 -07:00
Neal Poole
8818252f52 [herald] Add support for Arcanist Project as a field for Differential revisions
Summary: Useful in cases where there is an Arcanist Project but not a repository tracked by Phabricator for a particular revision.

Test Plan: Created a new rule to flag Differential revisions with a particular Arcanist project, verified that it applied as expected via the test console to revisions with the project specified and with a different project specified.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8463
2014-03-11 13:15:14 -07:00
epriestley
592591e715 Clean up various pieces of dead/obsolete Differential code
Summary:
Ref T2222.

  - Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
  - Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
  - Moves Releeph churn field off `DifferentialCommentQuery`.
  - Removes dead code in `DifferentialRevisionViewController`.
  - Removes `DifferentialException` (no references).
  - Removes `DifferentialRevision->loadComments()` (no callsites).
  - Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
  - Removes `DifferentialCommentQuery` (all callsites updated).

Test Plan: Mostly a lot of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8476
2014-03-11 13:02:19 -07:00
epriestley
2ceffadee7 Support Herald rules for new Differential edits
Summary:
Ref T2222. Ref T4484. See D8404 for discussion.

When a revision is updated with the new Editor, apply Herald rules. Additionally, apply them in a modern way which generates transactions.

Test Plan: {F122299}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Maniphest Tasks: T2222, T4484

Differential Revision: https://secure.phabricator.com/D8405
2014-03-05 12:07:13 -08:00
Joshua Spence
6270114767 Various linter fixes.
Summary:
- Removed trailing newlines.
- Added newline at EOF.
- Removed leading newlines.
- Trimmed trailing whitespace.
- Spelling fix.
- Added newline at EOF

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: hach-que, chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8344
2014-02-26 12:44:58 -08:00
epriestley
438915032a Minor, mark SERIALIZATION_PHP fields as BINARY in Lisk 2014-02-23 16:35:51 -08:00
Bob Trahan
20a3ee24f9 Herald - add application search for transcripts
Summary: this diff also makes the "test console" appear with the main search nav *and* updates application search to use the page title as the crumb rather than just search. Fixes T4399.

Test Plan: queried for transcript ids - success! queried for TX and MX - success! saved the TX and MX query and it worked again!

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4399

Differential Revision: https://secure.phabricator.com/D8297
2014-02-21 12:51:25 -08:00
epriestley
094922bcb9 Support "only the first time" in Maniphest
Summary:
Ref T4403. Implements "only the first time" for Maniphest rules, and fixes the trigger itself.

The trigger would never fire and block rules because it was comparing a string (like "first") to an int (like 0).

The "only" vs "every" stuff is contributed and I should have pushed back harder on this toInt / toString stuff. Maybe I'll just get rid of it; it purely causes confusion and problems.

Test Plan: Wrote an "only the first time" rule, ran it twice, it applied once.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4403

Differential Revision: https://secure.phabricator.com/D8193
2014-02-11 07:45:26 -08:00
Bob Trahan
1830868007 Herald - make herald condition of herald rule display better
Summary: ...by the surprising step of changing how this data is stored from id to phid. Also a small fix to not allow "disabled" rules to be used as herald rule conditions, i.e. can't make a rule that depends on a disabled rule.

Test Plan: viewed existing herald rule that had a rule condition and noted nice new display using handle. made a new rule that had a rule condition and verified it worked correctly.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8186
2014-02-10 14:40:09 -08:00
Bob Trahan
0cc1f50170 Herald - make flag color display
Summary: It used to say "Mark with flag 7" or whatever, and now it says "Mark with flag Checkered"

Test Plan: noted previous rule I made was more understandable

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8158
2014-02-06 13:06:11 -08:00
Bob Trahan
a4871d034a Herald - unleash some recursive madness!
Summary: turn on herald rules ability to specify other herald rules. Fixes T4294.

Test Plan: made a rule to be cc'd on new tasks. made another rule to flag a task if it contained "test test" in the title AND the cc'd rule for new tasks matched. Made some new tasks and verified new "test test" tasks were flagged.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4294

Differential Revision: https://secure.phabricator.com/D8157
2014-02-06 12:43:36 -08:00
Bob Trahan
1527a967c0 Herald - add support for task priority
Summary: adds a new FIELD and a new VALUE to support this. Slightly dodgy because priorities do not have phids so we have to special case how we handle this in a few spots. Ref T4294.

Test Plan: made a new rule to get cc'd on unbreak now and wishlist tasks. verified got cc'd correctly and not cc'd correctly.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4294

Differential Revision: https://secure.phabricator.com/D8156
2014-02-06 11:42:31 -08:00
Bob Trahan
9be4df02c2 Herald - Add "new" field to herald
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
2014-02-04 10:43:31 -10:00
epriestley
e9be9ecfbc Add a "branches" rule for commits
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
2014-01-28 14:43:13 -08:00
epriestley
56d44f1503 Modularize the Garbage Collector
Summary:
The GC is a big block of hard-coded application GCs right now. Among other things, this means third parties can't tap into the infrastructure.

Modularize it into `GarbageCollector` classes. This implements only one to prove the new stuff works; I'll followup with the rest in the next diff or few depending on how much mess I run into.

Test Plan: Used `bin/phd debug garbage` to run the collector in debug mode, observed reasonable output and behavior.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7970
2014-01-15 10:02:24 -08:00
epriestley
6cc5f952a4 Fix Herald field type error
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
2014-01-13 16:08:30 -08:00
Chad Little
b74c7a3d37 Simplify PHUIObjectBoxViews handling of Save and Error states
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.

Test Plan: Test out the forms, see errors without the text.

Reviewers: epriestley, btrahan

CC: Korvin, epriestley, aran, hach-que

Differential Revision: https://secure.phabricator.com/D7924
2014-01-10 09:17:37 -08:00
epriestley
efe187d5be Support "Repository's projects" field in Commit and Differential Revision rules
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
2014-01-09 15:56:24 -08:00
Bob Trahan
8867e50d63 Herald - tweak accepted differential revision check slightly
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
2014-01-08 11:53:13 -08:00
epriestley
8c360ab703 Rename "Apply build plans" to "Run build plans" in Herald
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
2014-01-06 12:14:21 -08:00
epriestley
3627e73e5e Apply "enormous changes" rules to pre-commit content rules too
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
2014-01-06 12:12:30 -08:00
epriestley
8ddf883d2e Cut Herald rules off at 1GB of diff text
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
2014-01-03 12:27:19 -08:00
epriestley
2cfc3acf32 Allow Herald pre-commit rules to act on repository projects
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
2014-01-03 12:24:28 -08:00
epriestley
140c88e971 Implement basic object rules for Herald
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
2013-12-30 16:48:14 -08:00
epriestley
472b0f983e Allow Herald Adapters to choose applicable rule types (global, personal, etc).
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
2013-12-30 16:48:07 -08:00
epriestley
f5fb3f05dc Lay most groundwork for Herald object rules
Summary:
Ref T4264. This gets most of the plumbing in for "object" rules, which will bind to a specific object, like a repository or project.

It does not yet let you actually create these rules.

Test Plan: Ran `storage upgrade`, created/edited rules, browsed Herald.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4264

Differential Revision: https://secure.phabricator.com/D7847
2013-12-27 13:17:10 -08:00
epriestley
f38a565aa5 Use radio buttons with explanatory text to select commit rule types
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
2013-12-27 13:16:33 -08:00
epriestley
79f57cf517 Split Herald rule creation across several steps
Summary:
Ref T4264. Currently, you choose a rule's content type (revision, commit, hook) and rule type (global, personal) on the same screen.

  - I want to make some rule types unavailable for some content types (e.g., personal hooks make little sense).
  - I want to make content type selection use a radio control instead of a dropdown, so it can explain what the content types do in more detail.
  - For new "object" hooks, I want to add a third step where you'll pick an object to bind to.

Split rule creation out into two steps. I think this won't get complicated enough for `PHUIPagedFormView`, but maybe I'll swap it in if this gets messier than I think.

Test Plan: Created some Herald rules, used back/cancel/etc.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4264

Differential Revision: https://secure.phabricator.com/D7845
2013-12-27 13:16:25 -08:00
epriestley
9f38aaa5de Add "raw author name" and "raw committer name" as Herald fields for commit content hooks
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
2013-12-27 13:16:00 -08:00
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
151f01ae94 Implement "Body" field in Herald pre-commit content hooks
Summary: Ref T4195. Adds support for writing rules against commit message bodies.

Test Plan: Pushed git, hg, svn commits and verified their bodies populated correctly in transcripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7796
2013-12-19 06:56:01 -08:00
epriestley
a5dc9067af Provide convenience method addTextCrumb() to PhabricatorCrumbsView
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.

Test Plan:
  - This was mostly automated, then I cleaned up a few unusual sites manually.
  - Bunch of grep / randomly clicking around.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: hach-que, aran

Differential Revision: https://secure.phabricator.com/D7787
2013-12-18 17:47:34 -08:00
epriestley
5f4df0f3e3 Support "changed filename" and "file content" fields for commit content Herald rules
Summary: Ref T4195. Adds support for diff content rules.

Test Plan: Pushed SVN and Git changes through, saw them generate reasonable transcripts. Mercurial still isn't hooked up to this phase.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7791
2013-12-18 14:18:58 -08:00
epriestley
e115f11f80 Provide basic commit content hooks for Herald
Summary: Ref T4195. This doesn't provide any interesting fields yet (content, affected paths, commit message) but fires the hook correctly.

Test Plan: Added a blocking hook and saw it fire.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7789
2013-12-18 14:18:45 -08:00
epriestley
1ff3ef382d Give Herald rules a standard "Hnnn" object name
Summary: Allow Herald rules to be referred to with `H123`, etc., like other object types are. Herald rules now have proper PHIDs and an increasingly prominent role in triggering application actions. Although I suspect users will rarely use `H123` in Remarkup to mention rules, this can simplify some of the interfaces which relate objects across systems.

Test Plan: Looked at various interfaces and saw `H123` names. Mentioned `H123` in remarkup.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7786
2013-12-18 12:00:18 -08:00
epriestley
181bfffaa1 Truncate object fields in Herald transcripts
Summary:
A few users have hit cases where Herald transcripts of large commits exceed the MySQL packet limit, because one of the fields in the transcript is an enomrous textual diff.

There's no value in saving these huge amounts of data. Transcripts are useful for understanding the action of Herald rules, but can be reconstructed later. Instead of saving all of the data, limit each field to 4KB of data.

For strings, we just truncate at 4KB. For arrays, we truncate after 4KB of values.

Test Plan: Ran unit tests. Artificially decreased limit and ran transcripts, saw them truncate properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: frgtn, aran

Differential Revision: https://secure.phabricator.com/D7783
2013-12-18 11:59:53 -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
baa756a027 Add rulePHID to HeraldEffect
Summary: Ref T4195. Herald rules gained PHIDs only recently, propagate them to HeraldEffect to make some of the hook stuff eaiser.

Test Plan: iiam

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7781
2013-12-17 15:23:45 -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
Richard van Velzen
16a7eaa700 Fix wrong link to "Create Rule" in Herald mobile view
Summary: The link pointed to `create/`, which gives as `404`.

Test Plan: clicked the link. It worked.

Reviewers: epriestley, #blessed_reviewers, chad

Reviewed By: chad

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7698
2013-12-04 04:57:34 -08:00
epriestley
7f11e8d740 Improve handling of email verification and "activated" accounts
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:

  - Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
    - Migrate all the existing users.
    - When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
    - Just make the checks look at the `isEmailVerified` field.
  - Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
  - Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
    - When the queue is enabled, registering users are created with `isApproved = false`.
    - Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
    - They go to the web UI and approve the user.
    - Manually-created accounts are auto-approved.
    - The email will have instructions for disabling the queue.

I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.

Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.

Test Plan:
  - Ran migration, verified `isEmailVerified` populated correctly.
  - Created a new user, checked DB for verified (not verified).
  - Verified, checked DB (now verified).
  - Used Conduit, People, Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D7572
2013-11-12 14:37:04 -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
Jakub Vrana
a29b5b070f Replace some hsprintf() by phutil_tag()
Test Plan: Looked at a diff with inline comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7549
2013-11-11 09:23:23 -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
Bob Trahan
da84546058 Add filter by object ability to flag query
Summary: See title. Fixes T1809.

Test Plan:
verified each type that has flaggable interface still can be flagged

verified that new custom query filter works

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T1809

Differential Revision: https://secure.phabricator.com/D7392
2013-10-25 12:52:00 -07: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
2a5c987c71 Lock policy queries to their applications
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.

This has several parts:

  - For PolicyAware queries, provide an application class name method.
  - If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
  - For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.

Test Plan:
  - Added a unit test to verify I got all the class names right.
  - Browsed around, logged in/out as a normal user with public policies on and off.
  - Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7367
2013-10-21 17:20:27 -07:00
epriestley
19968e31f4 Generalize Herald account sources
Summary: The "user" and "user/project" sources exclude system agents and disabled users, but should not.

Test Plan: Added system agents to Herald rules.

Reviewers: btrahan, bigo

Reviewed By: bigo

CC: aran

Differential Revision: https://secure.phabricator.com/D7319
2013-10-14 19:38:35 -07:00
epriestley
073cb0e78c Make PhabricatorPolicyInterface require a getPHID() method
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.

Some policy objects don't have real PHIDs:

  PhabricatorTokenGiven
  PhabricatorSavedQuery
  PhabricatorNamedQuery
  PhrequentUserTime
  PhabricatorFlag
  PhabricatorDaemonLog
  PhabricatorConduitMethodCallLog
  ConduitAPIMethod
  PhabricatorChatLogEvent
  PhabricatorChatLogChannel

Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.

Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.

Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7306
2013-10-14 14:35:47 -07:00
Chad Little
97c690fc0f PHUIPropertyListView
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.

Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7283
2013-10-11 07:53:56 -07:00
epriestley
650dc0cc30 Remove the "create rules" Herald capability
Summary:
Ref T603. In thinking about this, I think I went mad with power in creating this capability. I can't imagine any reason to give users access to Herald but not let them create rules.

We can restore this later if some install comes up with a good reason to have it, but in the interest of keeping policies as simple as possible, I think we're better off without it. In particular, if you don't want a group of users creating rules, just lock them out of the application entirely.

The "Manage Global Rules" capability is still around, I think that one's super good.

Test Plan: Edited Herald policies, created a rule.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7268
2013-10-09 13:55:44 -07:00
epriestley
3147a6ca57 Improve messaging of special policy rules in applications
Summary: Ref T603. When the user encounters an action which is controlled by a special policy rule in the application, make it easier for applications to show the user what policy controls the action and what the setting is. I took this about halfway before and left a TODO, but turn it into something more useful.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7265
2013-10-09 13:52:04 -07:00
epriestley
7a97a71e20 Move Herald application capabilities to newer infrastructure
Summary: Ref T603. Use the new hotness.

Test Plan: Edited Herald in Applications, tried to create rules / global rules without capabilities, got reasonable error messages.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7263
2013-10-09 13:44:41 -07:00
epriestley
0598600476 Always pass handles to tokenizers, not <phid -> name> maps
Summary: Ref T1279. Prerequisite for adding icons or other type information to tokenizers, since we don't currently have enough information to prefill them when rendering things from the server side. By passing handles in, the tokenizer can extract type information.

Test Plan:
- Searched by user in Audit.
- Sent Conpherence from profile page.
- Tried to send an empty conpherence.
- Searched Countdown by user.
- Edited CCs in Differential.
- Edited reviewers in Differential.
- Edited a commit's projects.
- Searched lint by owner.
- Searched feed by owner/project.
- Searched files by owner.
- Searched Herald by owner.
- Searched Legalpad by owner.
- Searched Macro by owner.
- Filtered Maniphest reports by project.
- Edited CCs in Maniphest.
- Searched Owners by owner.
- Edited an Owners package.
- Searched Paste by owner.
- Searched activity logs by owner.
- Searched for mocks by owner.
- Edited a mock's CCs.
- Searched Ponder by owner.
- Searched projects by owner.
- Edited a Releeph project's pushers.
- Searched Releeph by requestor.
- Edited "Uses Symbols" for an Arcanist project.
- Edited all tokenizers in main search.
- Searched Slowvote by user.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7248
2013-10-07 12:51:24 -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
953ff197bf Allow Herald rules to be disabled, instead of deleted
Summary:
Ref T603. Ref T1279. Further improves transaction and policy support for Herald.

  - Instead of deleting rules (which wipes out history and can't be undone) allow them to be disabled.
  - Track disables with transactions.
  - Gate disables with policy controls.
  - Show policy and status information in the headers.
  - Show transaction history on rule detail screens.
  - Remove the delete controller.
  - Support disabled queries in the ApplicationSearch.

Test Plan:
  - Enabled and disabled rules.
  - Searched for enabled/disabled rules.
  - Verified disabled rules don't activate.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279, T603

Differential Revision: https://secure.phabricator.com/D7247
2013-10-06 17:10:29 -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
a235768d58 Modernize "Test Console" and fix a minor display bug with "Always"
Summary:
  - Use the box view in the test console.
  - Let the test console load tasks and mocks. We should move this to the adapters (`canAdaptObject($object)` or something).
  - Fix a minor issue with "Always": hiding the whole cell could make the table layout weird in Safari, at least. Just hide the select instead.

Test Plan:
  - Used test console on task.
  - Used test console on mock.
  - Created (silly) rule with "Always" and also some other conditions.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7220
2013-10-04 15:17:01 -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
epriestley
b592630d72 Provide more structure to PHUIObjectBoxView
Summary:
Three changes here.

  - Add `setActionList()`, and use that to set the action list.
  - Add `setPropertyList()`, and use that to set the property list.

These will let us add some apropriate CSS so we can fix the border issue, and get rid of a bunch of goofy `.x + .y` selectors.

  - Replace `addContent()` with `appendChild()`.

This is just a consistency thing; `AphrontView` already provides `appendChild()`, and `addContent()` did the same thing.

Test Plan:
  - Viewed "All Config".
  - Viewed a countdown.
  - Viewed a revision (add comment, change list, table of contents, comment, local commits, open revisions affecting these files, update history).
  - Viewed Diffusion (browse, change, history, repository, lint).
  - Viewed Drydock (resource, lease).
  - Viewed Files.
  - Viewed Herald.
  - Viewed Legalpad.
  - Viewed macro (edit, edit audio, view).
  - Viewed Maniphest.
  - Viewed Applications.
  - Viewed Paste.
  - Viewed People.
  - Viewed Phulux.
  - Viewed Pholio.
  - Viewed Phame (blog, post).
  - Viewed Phortune (account, product).
  - Viewed Ponder (questions, answers, comments).
  - Viewed Releeph.
  - Viewed Projects.
  - Viewed Slowvote.

NOTE: Images in Files aren't on a black background anymore -- I assume that's on purpose?

NOTE: Some jankiness in Phortune, I'll clean that up when I get back to it. Not related to this diff.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7174
2013-09-30 09:36:04 -07:00
Chad Little
94d0704fdb Add objectheaders to new View
Summary: This adds the 'PHUIObjectBox' to nearly every place that should get it. I need to comb through Diffusion a little more. I've left Differential mostly alone, but may decide to do it anyways this weekend. I'm sure I missed something else, but these are easy enough to update.

Test Plan: tested each new layout.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7162
2013-09-28 15:55:38 -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
2e5ac128b3 Explain policy exception rules to users
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".

This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.

Test Plan: See screenshot.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7150
2013-09-27 08:43:41 -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
Chad Little
9be7a948f9 Move PHUIFormBoxView to PHUIObjectBoxView
Summary: I'd like to reuse this for other content areas, renaming for now. This might be weird to keep setForm, but I can fix that later if we need.

Test Plan: reload a few forms in maniphest, projects, differential

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7120
2013-09-25 11:23:29 -07:00
Chad Little
e8bb24fd60 Policy, Status in PHUIHeaderView
Summary: The adds the ability to set 'properties' such as state, privacy, due date to the header of objects.

Test Plan: Implemented in Paste, Pholio. Tested various states.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7016
2013-09-17 09:12:37 -07:00
Bob Trahan
c41c593388 Herald - make dry runs work for "apply once" rules after they have been applied
Summary: Fixes T3719

Test Plan: https://secure.phabricator.com/T3719#comment-7

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3719

Differential Revision: https://secure.phabricator.com/D6968
2013-09-13 11:38:49 -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
Bob Trahan
07b8becfc6 Policy - introduce parentQuery and pass around policy configuration from parent to child
Summary: Ref T603. Ref D6941.

Test Plan: Clicked around all over - looked good. I plan to re-test D6941 to make sure the executeOne case works now as intended

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6944
2013-09-11 12:19:34 -07:00
Chad Little
5ba20b8924 Move PhabricatorObjectItem to PHUIObjectItem, add 'plain' setting for lists.
Summary: Adds plain support for object lists that just look like lists

Test Plan: review UIexamples and a number of other applications

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6922
2013-09-09 14:14:34 -07:00
epriestley
0ae7335316 Add missing (int) casts to Herald rule editing
Summary: Fixes T3792. These raise errors if the database is in strict mode and you try to create an "any" rule.

Test Plan: Created a rule with "any".

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3792

Differential Revision: https://secure.phabricator.com/D6883
2013-09-04 12:07:26 -07:00
Gareth Evans
fcba0c74d9 Replace all "attach first..." exceptions with assertAttached()
Summary:
Ref T3599
Go through everything, grep a bit, replace some bits.

Test Plan: Navigate around a bit

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3599

Differential Revision: https://secure.phabricator.com/D6871
2013-09-03 06:02:14 -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
epriestley
f1c75a6382 Allow construction of ApplicationSearch queries with GET
Summary:
Ref T3775 (discussion here). Ref T2625.

T3775 presents two problems:

  # Existing tools which linked to `/differential/active/epriestley/` (that is, put a username in the URL) can't generate search links now.
  # Humans can't edit the URL anymore, either.

I think (1) is an actual issue, and this fixes it. I think (2) is pretty fluff, and this doesn't really try to fix it, although it probably improves it.

The fix for (1) is:

  - Provide a helper to read a parameter containing either a list of user PHIDs or a list of usernames, so `/?users[]=PHID-USER-xyz` (from a tokenizer) and `/?users=alincoln,htaft` (from an external program) are equivalent inputs.
  - Rename all the form parameters to be more digestable (`authorPHIDs` -> `authors`). Almost all of them were in this form already anyway. This just gives us `?users=alincoln` instead of `userPHIDs=alincoln`.
  - Inside ApplicationSearch, if a request has no query associated with it but does have query parameters, build a query from the request instead of issuing the user's default query. Basically, this means that `/differential/` runs the default query, while `/differential/?users=x` runs a custom query.

Test Plan: {F56612}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2625, T3775

Differential Revision: https://secure.phabricator.com/D6840
2013-08-29 11:52:29 -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
Chad Little
fe2a96e37f Update Form Layouts
Summary:
This attempts some consistency in form layouts. Notably, they all now contain headers and are 16px off the sides and tops of pages. Also updated dialogs to the same look and feel. I think I got 98% of forms with this pass, but it's likely I missed some buried somewhere.

TODO: will take another pass as consolidating these colors and new gradients in another diff.

Test Plan: Played in my sandbox all week. Please play with it too and let me know how they feel.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6806
2013-08-26 11:53:11 -07:00
epriestley
f034fd80db Remove getApplicationObjectTypeName from ApplicationTransactions
Summary:
We can get this out of PHIDType reasonably in all cases and simplify implementation here.

None of these translate correctly anyway so they're basically debugging/development strings.

Test Plan: `grep`, browsed some transactions

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D6786
2013-08-21 12:32:06 -07:00
epriestley
751cd547c2 Remove dust from page construction
Summary:
  ^\s+(['"])dust\1\s*=>\s*true,?\s*$\n

Test Plan: Looked through the diff.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D6769
2013-08-19 18:09:35 -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
epriestley
307a41e895 Rename "HeraldObjectAdapter" to "HeraldAdapter"
Summary: Ref T2769. The term "Object" is redundant.

Test Plan: grep

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6655
2013-08-07 18:03:50 -07:00
epriestley
b1c4a258c9 Add ApplicationTransactions to Herald
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
2013-08-07 18:03:49 -07:00
epriestley
589ae8d26d Use ApplicationSearch in Herald
Summary: Ref T2769. Ref T2625. Herald is currently a giant mishmash of hard-codes and weird special cases. Move toward modernization and normality.

Test Plan: {F52716}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2625, T2769

Differential Revision: https://secure.phabricator.com/D6652
2013-08-07 18:03:47 -07:00
epriestley
a7ce55e3ca Remove extra side navs in Herald
Summary: Ref T2769. Removes some nonstandard side navs.

Test Plan: Viewed affected pages.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6651
2013-08-07 18:03:46 -07:00
epriestley
ceb7f830a4 Make HeraldRuleQuery policy-aware
Summary: Ref T2769. dem policy checks

Test Plan: Loaded `/herald/`; loaded rule editor.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6650
2013-08-07 18:03:45 -07:00
epriestley
8eed5b1f14 Make HeraldRule implement PhabricatorPolicyInterface
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
2013-08-07 18:03:44 -07:00
epriestley
2820fdc89b Add PHIDs to Herald Rules
Summary: Ref T2769. Precursor to various Herald-related modernizations.

Test Plan: Ran migration; loaded Herald via web.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6648
2013-08-07 18:03:37 -07:00
Chad Little
f1bf27959f PHUIList, PHUIDocument updates
Summary:
This diff covers a bit of ground.

- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView

Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.

Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6130
2013-06-05 08:41:43 -07:00
Mihaly Zagon
bd5aea33cd Fix wrong argument order when calling DiffusionQuery::callConduitWithDiffusionRequest 2013-05-30 17:05:46 +02:00
Chad Little
c5e7222f9e Modernize Herald
Summary: Convert to responsive layout, pht, etc.

Test Plan: Test Herald on desktop and mobile.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5976
2013-05-20 08:24:07 -07:00
epriestley
571d191a90 Fix missing 'user' in PhabricatorOwnerPathQuery
Summary:
See:

  - https://github.com/facebook/phabricator/issues/317
  - https://github.com/facebook/phabricator/issues/319

This callsite was just overlooked in D5928, I think.

Test Plan: Ran `scripts/repository/reparse.php --owners rXnnnn` without hitting an exception.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D5930
2013-05-15 08:12:39 -07:00
Bob Trahan
016b62a7ef Diffusion - move some DiffusionRequest queries to occur over Conduit
Summary: Ref T2784. This one was a wee bit complicated. Had to add PhabricatorUser and concept of initFromConduit (or not) to DiffusionRequest.

Test Plan: foreach repo, visited CALLSIGN and clicked a commit and verified they laoded correctly. Hacked code to hit NOT via Conduit and repeated tests to great success.

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T2784

Differential Revision: https://secure.phabricator.com/D5928
2013-05-14 15:32:19 -07:00
Bob Trahan
c1d771d86c Diffusion - move DiffQuery to Conduit
Summary: title does not say it all; also added conduit wrappers for rawdiffquery and lastmodifiedquery. These get the wrapper treatment since they are used in daemons. Ref T2784.

Test Plan: for each of the 3 VCS, did the following: 1) loaded up CALLSIGN and verified 'Modified' column data showed up correctly in Browse Repository box. 2) loaded up the "change" view for a specific file and verified content showed up correctly. 3) loaded up a specific commit and noted the changes ajax loaded A-OK

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T2784

Differential Revision: https://secure.phabricator.com/D5896
2013-05-14 13:53:32 -07:00
Chad Little
43ff24b0f3 Update form styles, implement in many places
Summary:
This creates a common form look and feel across the site. I spent a bit of time working out a number of kinks in our various renderings. Some things:

- Font Styles are correctly applied for form elements now.
- Everything lines up!
- Selects are larger, easier to read, interact.
- Inputs have been squared.
- Consistant CSS applied glow (try it!)
- Improved Mobile Responsiveness
- CSS applied to all form elements, not just Aphront
- Many other minor tweaks.

I tried to hit as many high profile forms as possible in an effort to increase consistency. Stopped for now and will follow up after this lands. I know Evan is not a super fan of the glow, but after working with it for a week, it's way cleaner and responsive than the OS controls. Give it a try.

Test Plan: Tested many applications, forms, mobile and tablet.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5860
2013-05-07 14:07:06 -07:00
epriestley
0a069cb55a Require a viewer to load handles
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
2013-02-28 17:15:09 -08:00
epriestley
a22bea2a74 Apply lint rules to Phabricator
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such

Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D5002
2013-02-19 13:33:10 -08:00
Edward Speyer
16accb591c STRICT_ALL_TABLES fix for a Herald column
Summary: Casting a PHP bool to a MySQL `TINYINT(1)`!

Test Plan: This broke during arc diff; with this patch, arc diff now works!

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4999
2013-02-18 16:58:35 +00:00
epriestley
ef7f16180c Restore merge of phutil_tag. 2013-02-13 14:51:18 -08:00
epriestley
73cce6e131 Revert "Promote phutil-tag again"
This reverts commit 8fbabdc06d, reversing
changes made to 2dab1c1e42.
2013-02-13 14:08:57 -08:00
vrana
4eb84149c2 Convert everything to safe HTML
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.

Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.

Reviewers: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4927
2013-02-13 12:35:40 -08:00
vrana
868ca71451 Fix some HTML problems
Summary: I'm too lazy to attaching them for diffs where they were introduced.

Test Plan:
/
/D1, wrote comment with code snippet
DarkConsole
commit detail, wrote comment
task detail, wrote comment

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4911
2013-02-11 18:18:26 -08:00
vrana
9b8da73765 Convert AphrontTableView to safe HTML
Summary:
Lots of killed `phutil_escape_html()`.

Done by searching for `AphrontTableView` and then `$rows` (usually) backwards.

Test Plan:
Looked at homepage.

  echo id(new AphrontTableView(array(array('<'))))->render();

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4884
2013-02-09 15:11:38 -08:00
vrana
d817dfa8fc Convert some phutil_escape_html() to hsprintf()
Summary: Found by `sgrep_php -e '"...".phutil_escape_html(...)'`.

Test Plan:
/
/D1
/uiexample/
/countdown/1/
/herald/transcript/1/all/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4869
2013-02-08 15:59:02 -08:00
vrana
afc5333bb3 Convert AphrontFormView to safe HTML
Summary: Searched for `AphrontFormView` and then for `appendChild()`.

Test Plan: /login/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4855
2013-02-07 18:01:00 -08:00
vrana
059920c2da Convert AphrontErrorView to safe HTML
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.

Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4843
2013-02-07 17:26:01 -08:00
vrana
2f508bf0dc Delete some phutil_safe_html()
Test Plan: Displayed revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4829
2013-02-05 15:52:48 -08:00
vrana
6bb7a282b1 Convert AphrontFormControl to safe HTML
Summary: Everything here now should properly handle plain strings and safe HTML.

Test Plan: /settings/panel/display/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4826
2013-02-05 15:52:46 -08:00
epriestley
94f6b6ca4e Fix every HTML issue I could find
Summary:
I attempted to test every interface. I probably missed some stuff, but I at least have some level of confidence that the `phutil_tag` branch is fairly stable.

Fixed these issues:

[1] Fixed a Herald issue with object links in transcripts. Some actions return
links; this was previously hard-coded.
[2] DarkConsole refactoring created an issue where the "`" event handler registered too many times.
[3] Fixed a bug where `strlen($value)` was being checked, but fields may now return array(). Possibly we should implement phutil_is_empty_html() or similar.
[4] Fixed a undefined variable issue for image edit transactions.
[5] Fixed an issue with rendering participant transactions. This required phutil_safe_html() because `pht()` can't handle `array()` for `%s`.
[6] Fixed an issue where feed was entirely overescaped by reverting an overly ambitious render_tag -> tag.
[7] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[8] Fixed an issue where &bull; was shown escaped.
[9] Fixed an issue where "no data" was overescaped.
[10] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[11] Fixed an issue with strict tables and inserting `''`.
[12] Fixed an issue with missing space after ":" for mini panels.

Encountered (but did not fix) these issues:

[X1] "e" works incorrectly on comments you are not allowed to edit. Did not fix.
[X2] Coverage currently interacts incorrectly with "--everything" for Phutil tests.

Test Plan:
  - Viewed Differential.
  - Created a diff via copy/paste.
  - Viewed standalone diff.
  - Jumped to diff via changeset table.
  - Created a revision.
  - Updated revision.
  - Added a comment.
  - Edited revision dependencies.
  - Edited revision tasks.
  - Viewed MetaMTA transcripts.
  - Viewed Herald transcripts [1].
  - Downloaded raw diff.
  - Flagged / unflagged revision.
  - Added/edited/deleted inline comment.
  - Collapsed/expanded file.
  - Did show raw left.
  - Did show raw right.
  - Checked previews for available actions.
  - Clicked remarkup buttons
  - Used filetree view.
  - Used keyboard: F, j, k, J, K, n, p, t, h, "?" [2] [X1].
  - Created a meme.
  - Uploaded a file via drag and drop.
  - Viewed a revision with no reviewers.
  - Viewed a revision with >100 files.
  - Viewed various other revisions [3].
  - Viewed an image diff.
  - Added image diff inline comments.
  - Viewed Maniphest.
  - Ran various queries.
  - Created task.
  - Created similar task.
  - Added comments to tasks.
  - Ran custom query.
  - Saved custom query.
  - Edited custom queries.
  - Drag-reordered tasks.
  - Batch edited tasks.
  - Exported tasks to excel.
  - Looked at reports (issue in T2311 notwithstanding).
  - Viewed Diffusion.
  - Browsed Git, SVN, HG repositories.
  - Looked at history, browse, change, commit views.
  - Viewed audit.
  - Performed various audit searches.
  - Viewed Paste.
  - Performed paste searches.
  - Created, edited, forked paste.
  - Viewed Phriction.
  - Edited a page.
  - Viewed edit history.
  - Used search typeahead to search for user / application.
  - Used search to search for text.
  - Viewed Phame.
  - Viewed Blog, Post.
  - Viewed live post.
  - Published/unpublished post.
  - Previewed post.
  - Viewed Pholio.
  - Edited/commented mock.
  - Viewed ponder.
  - Viewed question.
  - Added answer/comment.
  - Viewed Diviner.
  - Viewed Conpherence [4] [5].
  - Made Conpherence updates.
  - Viewed calendar.
  - Created status.
  - Viewed status.
  - Viewed Feed [6].
  - Viewed Projects.
  - Viewed project detail.
  - Edited project.
  - Viewed Owners.
  - Viewed package detail.
  - Edited package [7].
  - Viewed flags.
  - Edited flag.
  - Deleted flag.
  - Viewed Herald.
  - Viewed rules.
  - Created rule.
  - Edited rule.
  - Viewed edit log.
  - Viewed transcripts.
  - Inspected a transcript.
  - Viewed People.
  - Viewed list.
  - Administrated user.
  - Checked username/delete stuff.
  - Looked at create/import LDAP/activity logs.
  - Looked at a user profile.
  - Looked at user about page.
  - Looked at Repositories.
  - Edited repository.
  - Edited arcanist project.
  - Looked at daemons.
  - Looked at all daemons [8].
  - Viewed combined log.
  - Looked at configuration.
  - Edited configuration.
  - Looked at setup issues [9].
  - Looked at current settings.
  - Looked at application list.
  - Installed / uninstalled applications [10].
  - Looked at mailing lists.
  - Created a mailing list.
  - Edited a mailing list.
  - Looked at sent mail.
  - Looked at received mail.
  - Looked at send/receive tests.
  - Looked at settings.
  - Clicked through all the panels.
  - Looked at slowvote.
  - Created a slowvote [11].
  - Voted in a slowvote.
  - Looked at Macro.
  - Created a macro.
  - Edited a macro.
  - Commented on a macro.
  - Looked at Countdown.
  - Created a Countdown.
  - Looked at it.
  - Looked at Drydock.
  - Poked around a bit.
  - Looked at Fact.
  - Poked around a bit.
  - Looked at files.
  - Looked at a file.
  - Uploaded a file.
  - Looked at Conduit.
  - Made a Conduit call.
  - Looked at UIExamples.
  - Looked at PHPAST.
  - Looked at PHIDs.
  - Looked at notification menu.
  - Looked at notification detail.
  - Logged out.
  - Logged in.
  - Looked at homepage [12].
  - Ran `arc unit --everything --no-coverage` [X2].

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4807
2013-02-04 17:06:34 -08:00
epriestley
820c6279f3 More render_tag -> tag (misc, form inset view)
Summary: Converts some callsites.

Test Plan: Grepped; looked -- see inlines.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4688
2013-01-28 18:42:57 -08:00
epriestley
3093d1663d Add javelin_tag(), convert easy callsites
Summary:
  - Implements `javelin_tag()`, which is `javelin_render_tag()` on top of `phutil_tag()` instead of `phutil_render_tag()`.
  - Manually converts all or almost all of the trivial callsites.

Test Plan:
  - Site does not seem any more broken than before.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4639
2013-01-25 12:57:17 -08:00
vrana
20768d65d5 Convert phutil_render_tag(X, Y, '...') to phutil_tag
Summary:
Created with spatch:

  lang=diff
  - phutil_render_tag
  + phutil_tag
    (X, Y, '...')

Then searched for `&` and `<` in the output and replaced them.

Test Plan: Loaded homepage.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4503
2013-01-24 19:20:27 -08:00
vrana
48561a8b1f Convert phutil_render_tag(X, Y, phutil_escape_html(Z)) to phutil_tag
Summary:
Created with spatch:

  lang=diff
  - phutil_render_tag
  + phutil_tag
    (X, Y,
  - phutil_escape_html(
    Z
  - )
    )

Test Plan: Loaded homepage

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4501
2013-01-24 19:08:55 -08:00
epriestley
73f63158a8 Fix double-menus in Herald transcripts
Summary: Fixes T2329. Adds crumbs and gets rid of the double-depth menus.

Test Plan: Looked at transcript list, test controller and transcript detail.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T2329

Differential Revision: https://secure.phabricator.com/D4564
2013-01-21 07:46:13 -08:00
epriestley
be91064a1d Fix Herald "any changed content" rule for commits
Summary: Fixes T1330. This has been broken with a TODO since we open sourced. :/

Test Plan: Used test console to verify this produced the correct field value for several commits.

Reviewers: btrahan, chad, vrana

Reviewed By: chad

CC: aran

Maniphest Tasks: T1330

Differential Revision: https://secure.phabricator.com/D4563
2013-01-21 07:46:03 -08:00
Chad Little
117589c160 Clean up Login, Responsive Forms
Summary: Removes the panel-view on login and adds additonal responsive styles for mobile forms.

Test Plan: View in mobile browser, resize page.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4530
2013-01-19 14:30:26 -08:00
Chad Little
c2b31149ab Removie panelview backgrounds from Herald.
Summary: Prior to re-doing responsive form CSS, update the Herald app for no panel backgrounds.

Test Plan: Went through Herald application, see no panels.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4545
2013-01-19 12:15:41 -08:00
Chad Little
38626dce64 Remove spacer from sidebars.
Summary: This removes all calls to addSpacer and the method. We were applying it inconsistently and it was causing spacing issues with redesigning the sidenav. My feeling is we can recreate the space in CSS if the design dictates, which would apply it consistently.

Test Plan: Go to Applications, click on every application.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4420
2013-01-13 08:17:12 -08:00
vrana
ef214e94ce Move setUser() to AphrontView
Summary: This is used in every other view.

Test Plan: Browsed around.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4248
2012-12-20 14:49:52 -08:00
epriestley
f306cab653 Use application icons for "Eye" menu and Crumbs
Summary:
Issues here:

  - Need an application-sized "eye", or a "home" icon for "Phabricator Home".
  - Some of the "apps_lb_2x" sliced images are the "_dark_" versions, not the light versions.
  - If you slice an application-sized "logout" (power off) icon and application-sized "help" (questionmark in circle) icon I can replace the current menu icons and nearly get rid of "autosprite".
  - To replace the icons on /applications/, the non-retina size is "4x", so we'd need "8x" for retina. Alternatively I can reduce the icon sizes by 50%.
  - The "Help", "Settings" and "Logout" items currently have a "glowing" hover state, which needs a variant (or we can drop it).
  - The /applications/ icons have a white hover state (or we can drop it).
  - The 1x application (14x14) icons aren't used anywhere right now, should they be? Maybe in the feed in the future, etc?
  - The "apps-2x" and "apps-large" sheets are the same image, but getting them to actually use the same file is a bit tricky, so I just left them separate for now.

Test Plan:
{F26698}
{F26699}

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1960

Differential Revision: https://secure.phabricator.com/D4108
2012-12-07 13:37:28 -08:00
epriestley
20ee3003b5 Replace all instances of AphrontSideNavView with AphrontSideNavFilterView
Summary:
AphrontSideNavView is an old class which required you to do a lot of work; it was obsoleted by AphrontSideNavFilterView. Remove all direct callsites so I can clean it up.

This is a precursor to letting me render a filter menu as a dropdown menu for T1960.

Test Plan:
Examined each interface for correct filter construction and selection:

  - Browsed Diffusion
  - Browsed Differential
  - Browsed Files
  - Browsed Slowvote
  - Browsed Phriction
  - Browsed repo edit interface

Grepped for `AphrontSideNavView`. The only remaining instances are in `AphrontSideNavView` itself and `AphrontSideNavFilterView` (which currently uses it).

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1960

Differential Revision: https://secure.phabricator.com/D4034
2012-12-07 13:30:31 -08:00
vrana
ef85f49adc Delete license headers from files
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
2012-11-05 11:16:51 -08:00
epriestley
c02a588b4e Remove "admin page" color/styles
Summary: These haven't had any effect since the redesign, and we're moving to a more granular policy model so it probably doesn't make sense to ever restore them.

Test Plan: Grepped for "admin" and removed all relevant code.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D3710
2012-10-16 10:33:37 -07:00
vrana
22cb8f5d08 Require canonical numbers in routes
Summary:
D03646 works, I don't want it to work.
Theoretically, it can cause us some troubles if we use this string in JS number context where 030 is 24.

Test Plan: D03646, D3646

Reviewers: epriestley, edward

Reviewed By: edward

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3646
2012-10-05 18:07:54 -07:00
vrana
5bde6c71ce Declare used properties
Summary: Also fix couple of other errors.

Test Plan: Executed controllers.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3627
2012-10-04 14:46:06 -07:00
epriestley
78784aa1fe Group applications into groups on /applications/
Summary: So they're maybe a little easier to deal with? I'm going to take this formally to "plz @chad plz help" land.

Test Plan: {F20329}

Reviewers: btrahan, vrana, chad

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D3609
2012-10-03 15:46:19 -07:00
epriestley
270256d336 Restore flavor text to applications
Summary: D3581 removed some flavor text. Allow applications to provide flavor text instead of status information if they so desire.

Test Plan: {F20325}

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran, chad

Differential Revision: https://secure.phabricator.com/D3608
2012-10-03 15:16:26 -07:00
epriestley
14fe3f5d14 Implement getHelpURI() for new applications
Summary: A bunch of recently-created applications have help available; link to it.

Test Plan: Clicked each app, clicked help link in menu bar, ended up in relevant documentation.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D3602
2012-10-03 12:39:18 -07:00
epriestley
a717d7db33 Add Conduit and Herald applications
Summary: See D3572. Adds applications for Conduit and Herald.

Test Plan: Viewed `/applications/`, saw applications. Clicked buttons, got to applications.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D3578
2012-10-01 12:56:10 -07:00
vrana
8ff52c0b6c Set viewer for all handles loaded in controllers
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.

Test Plan: Displayed revision with sporadic author.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3432
2012-09-04 23:14:26 -07:00
epriestley
f5e738b7ca Minor, fix a name collision between HeraldRuleController and PhabricatorController.
Auditors: btrahan
2012-08-15 14:11:40 -07:00
vrana
ea0fe6d64b Optimize matching regexps in Herald rules
Summary:
We spend 6.37 s in this condition on a big diff.
By adding the 'S' flag, the time is down to 2.15 s.

Test Plan: `DifferentialRevisionEditor::newRevisionFromConduitWithDiff()`

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3284
2012-08-14 15:14:02 -07:00
epriestley
66cee129b6 Remove all code referencing old tab navigation
Summary:
  - Add getHelpURI() to PhabricatorApplication for application user guides.
  - Add a new "help" icon menu item and skeletal Diviner application.
  - Move help tabs to Applications where they exist, document the other ones that don't exist yet.
  - Grep for all tab-related stuff and delete it.

Test Plan: Clicked "help" for some apps. Clicked around randomly in a bunch of other apps.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D3267
2012-08-13 15:28:41 -07:00
epriestley
ed4a155c91 Rename "IDPaged" to "CursorPaged", "executeWithPager" to "executeWith[Cursor|Offset]Pager"
Summary:
I'm trying to make progress on the policy/visibility stuff since it's a blocker for Wikimedia.

First, I want to improve Projects so they can serve as policy groups (e.g., an object can have a visibility policy like "Visible to: members of project 'security'"). However, doing this without breaking anything or snowballing into a bigger change is a bit awkward because Projects are name-ordered and we have a Conduit API which does offset paging. Rather than breaking or rewriting this stuff, I want to just continue offset paging them for now.

So I'm going to make PhabricatorPolicyQuery extend PhabricatorOffsetPagedQuery, but can't currently since the `executeWithPager` methods would clash. These methods do different things anyway and are probably better with different names.

This also generally improves the names of these classes, since cursors are not necessarily IDs (in the feed case, they're "chronlogicalKeys", for example). I did leave some of the interals as "ID" since calling them "Cursor"s (e.g., `setAfterCursor()`) seemed a little wrong -- it should maybe be `setAfterCursorPosition()`. These APIs have very limited use and can easily be made more consistent later.

Test Plan: Browsed around various affected tools; any issues here should throw/fail in a loud/obvious way.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D3177
2012-08-07 11:54:06 -07:00
Edward Speyer
83a5f1479d Arrayish fields should always return arrays
Summary:
Array-like fields in HerladCommitAdapter should always return things of
type array.  If they return array(...) or null, then array_*() functions
sporadically break.

Test Plan:
Ran a PhabricatorRepositoryCommitHeraldWorker for a troublesome commit.
Failed before; fails no more.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, Korvin, vdt

Maniphest Tasks: T1385

Differential Revision: https://secure.phabricator.com/D2793
2012-06-19 11:18:38 -07:00
vrana
2e484e257d Fix lint errors found by Nemo
Summary:
See also:

- https://github.com/tpyo/amazon-s3-php-class/pull/33
- https://github.com/stripe/stripe-php/pull/13

Test Plan: Ran a script analyzing sources by HPHP.

Reviewers: btrahan, jungejason, epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D2713
2012-06-11 19:09:42 -07:00
vrana
6cc196a2e5 Move files in Phabricator one level up
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
2012-06-01 12:32:44 -07:00
epriestley
09c8af4de0 Upgrade phabricator to libphutil v2
Summary: Mechanical changes from D2588. No "Class.php" moves yet.

Test Plan: See D2588.

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1103

Differential Revision: https://secure.phabricator.com/D2591
2012-05-30 14:26:29 -07:00
vrana
347bc357fd Display Browse in Diffusion and Open in Editor links in commit detail
Test Plan:
/rX1
Browse in Diffusion
Open in Editor

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2180
2012-04-10 09:54:38 -07:00
epriestley
a5903d2a53 Use head_key() and last_key() to explicitly communicate intent
Summary:
PHP arrays have an internal "current position" marker. (I think because foreach() wasn't introduced until PHP 4 and there was no way to get rid of it by then?)

A few functions affect the position of the marker, like reset(), end(), each(), next(), and prev(). A few functions read the position of the marker, like each(), next(), prev(), current() and key().

For the most part, no one uses any of this because foreach() is vastly easier and more natural. However, we sometimes want to select the first or last key from an array. Since key() returns the key //at the current position//, and you can't guarantee that no one will introduce some next() calls somewhere, the right way to do this is reset() + key(). This is cumbesome, so we introduced head_key() and last_key() (like head() and last()) in D2161.

Switch all the reset()/end() + key() (or omitted reset() since I was feeling like taking risks + key()) calls to head_key() or last_key().

Test Plan: Verified most of these by visiting the affected pages.

Reviewers: btrahan, vrana, jungejason, Koolvin

Reviewed By: jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2169
2012-04-09 11:08:59 -07:00
mkedia
591d50008f Allow projects to own packages
Summary:
- The UI is pretty straightforward, since Handle just works (tm)
- Added two methods to the owners object to handle the new layer of
  indirection. Then ran git grep PhabricatorOwnersOwner and changed
  callsites as appropriate.

Sending this to get a round of feedback before I test the non-trivial
changes in this diff.

Test Plan:
- owners tool: edit, view, list for basic functionality.
- phlog for the two new methods I added

Reviewers: epriestley, blair, jungejason

CC: aran

Differential Revision: https://secure.phabricator.com/D2079
2012-04-03 16:20:07 -07:00
vrana
8813c7be0e Use assert_instances_of() everywhere but Differential and Diffusion
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
2012-04-03 14:53:20 -07:00
epriestley
ffb074424d Quick fix, unbreak Herald. Proper fix should not go down this path.
Auditors: btrahan, makinde
2012-04-02 13:11:02 -07:00
epriestley
fcec4c368c Allow users to add flags via Herald rules
Summary: Add "Mark with flag" rules to Herald.

Test Plan: Created / edited a "Mark with flag" rule. Parsed revisions / commits, got flags added.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, vrana

Maniphest Tasks: T1041

Differential Revision: https://secure.phabricator.com/D2060
2012-03-30 13:51:54 -07:00
epriestley
83d6bbeb29 Minor, jumped the gun on review feedback in D2040.
Auditors: btrahan
2012-03-30 10:50:38 -07:00
epriestley
698ec68327 General Herald refactoring pass
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
2012-03-30 10:49:55 -07:00
Hafsteinn Baldvinsson
fffc1e51d0 Inset view controller for inset elements of forms.
Summary:
T937 suggests 'inset' could have its own view controller.

It has the following methods:
 - setTitle         for title
 - setRightbutton   if you have to place something (preferably a button)
                    on the right side of the form
 - setDescription   if you want to describe what it does
 - setContent       for the main content
 - addDivAttributes REALLY not sure about this one but it had to be included
                    because of a single controller (see owners/controller/edit/PhabricatorOwnersEditController.php:238)
 - appendChild      works as usual if your form is complex but you still want to remove
                    ->appendChild('<div class..') ->appendChild('</div>');

It might be an overkill so maybe some could be dropped:
 - addDivAttributes() and just rewrite how PhabricatorOwnersEditController.php works
 - setContent() and use appendChild for the main content?

Test Plan:
 - Looked at the controllers in phabricator
 - Changed the controller
 - Opened the page in another tab
 - If something didnd't look the same I fixed it.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1926
2012-03-15 17:10:38 -07:00
epriestley
d0af617818 Add "final" to (almost) everything else
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
2012-03-13 16:21:04 -07:00
epriestley
b2890eeb0e Add "final" to all Phabricator "Controller" classes
Summary:
These are all unambiguously unextensible. Issues I hit:

  - Maniphest Change/Diff controllers, just consolidated them.
  - Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
  - D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.

Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T795

Differential Revision: https://secure.phabricator.com/D1843
2012-03-09 15:46:25 -08:00
epriestley
d94129b739 Migrate "PhabricatorOwnersPackageCommitRelationship" to "PhabricatorRepositoryAuditRequest"
Summary:
  - Move table to Repository, since we have no Owners joins in the application anymore but would like to do a Repository join.
  - Rename "packagePHID" to "auditorPHID", since this column may contain package, project, or user PHIDs.

Test Plan:
  - Browsed Owners, Audit, and Differential interfaces to the Audit tool.
  - Made comments and state changes.
  - Ran "reparse.php --herald --owners" on several commits.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, nh, vrana

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1787
2012-03-05 13:17:29 -08:00
epriestley
1094527072 Allow Herald to trigger audits for users or projects
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
2012-02-27 09:36:30 -08:00
epriestley
ba05ac595c Minor, fix a fatal on Herald Admin controller
Summary:
Just a bad merge for the edit history, I think. We need to pass $user or we
fatal trying to render timestamps.

https://secure.phabricator.com/herald/all/view/differential/

Test Plan: Looked at Herald admin view.

Reviewers: jungejason, xela, nh, btrahan, fratrik

Reviewed By: fratrik

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1611
2012-02-14 15:40:04 -08:00
epriestley
8b4c057c75 Disable personal Herald rules from invalid owners
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
2012-01-31 12:11:31 -08:00
epriestley
5954ae84aa Improve Herald personal/global UI/UX
Summary:
  - Default "personal" vs "global" choice to "personal".
  - Don't show global rules under "My Rules".
  - After editing or creating a global rule, redirect back to global rule list.
  - Use radio buttons for "personal" vs "global" and add captions explaining the
difference.
  - For "global" rules, don't show the owner/author in the rule detail view --
they effectively have no owner (see also D1387).
  - For "global" rules, don't show the owner/author in the rule list view, as
above.
  - For admin views, show rule type (global vs personal).

Test Plan:
  - Created and edited new global and personal rules.
  - Viewed "my", "global" and "admin" views.

Reviewers: btrahan, jungejason, nh, xela

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1518
2012-01-31 12:09:29 -08:00
awyler
56df2bc7be Add basic edit history to herald rules
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
2012-01-30 11:52:44 -08:00
epriestley
dbd91d6818 Allow differential.query to find accepted and committed revisions; fix a fatal
Summary:
  - Expose existing 'committed' filter.
  - Add an 'accepted' filter.
  - Fix a fatal where $repository may not be defined (for diffs not linked to a
repository).

Test Plan: Ran accepted / committed queries. Viewed a previously fataling diff.

Reviewers: btrahan, vrana, Makinde

Reviewed By: Makinde

CC: Koolvin, aran, epriestley

Differential Revision: https://secure.phabricator.com/D1490
2012-01-25 14:50:34 -08:00
epriestley
5b463e634c Write fewer "applied" rows and clean up excess historical rows
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
2012-01-25 11:53:39 -08:00
epriestley
3142fe4419 Remove massive "rule applied" query
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
2012-01-24 19:29:54 -08:00
Nick Harper
76dd84e307 Replace HeraldActionConfig::getActionMap() in HeraldTranscriptController
Summary:
D1449 removed HeraldActionConfig::getActionMap(), but it was still used in
HeraldTranscriptController. This fixes the controller to use the method that
replaced getActionMap.

Test Plan: loaded a herald transcript

Reviewers: epriestley, xela

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1466
2012-01-23 13:29:44 -08:00
awyler
6080d74112 Created personal vs. global herald rule distingtion
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
2012-01-19 11:21:49 -08:00
awyler
14d16eab17 Enable Phabricator admin to change the owner of a herald rule
Summary:
Added a typeahead in the edit herald rule page that allows an admin or
owner to change the current owner of a rule.  If the typeahead is emptied, the
current owner will remain owner.

Test Plan:
Created a test rule.  Changed the owner.  Deleted the owner in the
typahead. Verified expected behavior.

Reviewers: jungejason, epriestley

Reviewed By: epriestley

CC: aran, jungejason, epriestley, xela

Differential Revision: https://secure.phabricator.com/D1322
2012-01-18 11:59:35 -08:00
jungejason
4faab06c3c Enable herald rule for commits that need auditing
Summary:
enable herald commit rules to have access to auditing info.

Note that the new herald condition I added contains info for the
packages. I thought about using a simpler herald condition like
"Requires audit is true or false" and let it work together with the
existing "Affected package contains any of the package". It doesn't work
because we need the info about the package to decide if the commit
requires audit, but the herald conditions work separately.

Test Plan:
- A commit requiring auditing was detected by a herald rule that checks
  the auditing status
- A commit not requiring auditing was not detected by a herald rule
  which checks auditing status, but was detected by a rule which doesn't
  check the auditing status

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1399
2012-01-17 09:13:07 -08:00
Bob Trahan
890f0ff7fa ...fix my fat finger period to a comma
Summary: see above

Test Plan: no errors on herald controller

Reviewers: epriestley
2011-12-29 08:55:54 -08:00
Bob Trahan
46baa3b7ae Herald - Kill Tabs
Summary: makes a nice side filter for most UI elements.  only place this getds a
little funky is on the test console; a second, inner filter list appears for the
"affected" filters.

Test Plan: viewed each side filter and verified ui.  for each filter, interacted
with the ui and made sure things looked right and there were no errors

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T631

Differential Revision: https://secure.phabricator.com/D1289
2011-12-29 08:51:21 -08:00
jungejason
c13b7da290 Add Related Commits for Owners
Summary:
For each commit, find the affected packages, and provide a way to
search by package.

Test Plan:
create commits that touch and don't touch two packages, and verify
that they display correctly in all the UI pages.

Reviewers: epriestley, blair, nh, tuomaspelkonen

Reviewed By: epriestley

CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi

Maniphest Tasks: T83

Differential Revision: 1208
2011-12-14 22:48:57 -08:00
epriestley
2efd8fe971 Fix a bad %d for PHID
Summary: D1174 caught this issue -- we mean to load all //your// rules, but
actually load //all// rules. Use %s correctly.

Test Plan: Hit /herald/rule/ without an exception.

Reviewers: fmoo, btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Differential Revision: 1203
2011-12-13 18:30:53 -08:00
Jason Ge
42383214ea Enable admin to view and delete other users' herald rules
Summary:
enable admin to delete user's herald rules. This is useful for
managing non-active users' rules. For example, ex-employees' rules. The
code change includes:

 - Added a 'All' tab which is only accessible to admin.
 - Refactor out a HeraldRuleListView which is used by both the home
   controller and the all rule controller

Test Plan:
delete an ex-employee rule as an admin; disable myself as
admin and verified that I don't have access to view other user's rules
and I'am not be able to delete them; also verified that as a non-admin,
I can still view, create and delete my own rules.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley, jungejason

Differential Revision: 1064
2011-11-15 16:21:51 -08:00
Jason Ge
f46e12d0ca Refactor some Herald code
Summary:
I was reading herald code for a task and realized that the method was
really long. So I refactor it to shorter methods.

Test Plan:
was still able to create a differential rule and commit rule; and
verified that dry-run still worked.

Reviewers: epriestley, tuomaspelkonen

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1077
2011-11-04 16:32:00 -07:00
epriestley
cd3a3bf759 Make Herald Rules sticky in X-Herald-Rules
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
2011-08-17 10:38:29 -07:00
epriestley
62532ef26d Fix Herald to accept new JSON encoding of sparse arrays
Summary: JX.JSON was recently changed to use JSON.stringify (the native
implementation) if it is available. The native implementation has a behavioral
difference from the Javelin implementation, in that it does not compact sparse
arrays. Ignore nulls resulting from removals when processing the encoded action
and condition lists.
Test Plan: Removed conditions from Herald rules.
Reviewed By: aran
Reviewers: aran, cpojer, jungejason
CC: aran, epriestley
Differential Revision: 606
2011-07-07 15:27:12 -07:00
epriestley
4ef918e213 Add a garbage collector daemon
Summary:
Phabricator generates a bunch of data that we don't need to keep around forever,
add a GC daemon to get rid of it with some basic configuration options.

This needs a couple more diffs to get some of the details but I think this is a
reasonable start.

I also fixed a couple of UI things related to this, e.g. the daemon logs page
going crazy when a daemon gets stuck in a loop and dumps tons of data to stdout.

Test Plan:
  - Ran gc daemon in 'phd debug' mode and saw it delete stuff, then sleep once
it had cleaned everything up.
  - Mucked around with TTLs and verified they work correctly.
  - Viewed gc'd transcripts in the web interface and made sure they displayed
okay.
  - Viewed daemon logs before/after garbage collection.
  - Running some run-at / run-for tests now, I'll update if the daemon doesn't
shut off in ~10-15 minutes. :P

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 583
2011-07-05 13:49:11 -07:00
epriestley
3ec76f5f4a Index "owner" relationship in search correctly for Revisions
Summary:
The owners of a revision are only really the reviewers when the revision is in
NEEDS_REVIEW.

Also build a raw indexed document viewer so you can look at the index of a
document from the web interface.

Finally, reindex revisions when comments are added, not just when the revision
itself is edited.

Test Plan:
Toggled abandon/reclaim on a revision and verified the relationships indexed
properly.

Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 470
2011-06-20 05:16:27 -07:00
Andrew Toulouse
9b522982fa Add timezone support
Summary:
Allows user-configurable timezones. Adds a preference panel, and migrates to the
new date rendering in easily-modified areas of the code. ***In progress***.

Test Plan:
Check database to make sure the field is being changed when the settings are
changed; check affected views to see how they render times.

Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, toulouse
Differential Revision: 475
2011-06-18 13:07:43 -07:00
epriestley
d33670c416 Update Herald documentation. 2011-06-14 11:09:40 -07:00
epriestley
1e5fd3a386 Fix Herald exception when updating a diff that has carryover CCs
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
2011-06-09 14:10:40 -07:00
gc3
22c1b38655 herald: add the ability to execute a rule the first time only
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
2011-06-09 10:35:37 -07:00
jungejason
4a76174118 Adding paging to transcript pages
Summary:
enable paging by adding a AphrontPagerView to the two
transcript page.

Test Plan:
test the paging on both pages. Also test it with a certain
phid is given.

Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: epriestley, jungejason
Differential Revision: 116
2011-05-08 15:59:39 -07:00
epriestley
8370f93048 Make X-Herald-Rules header sticky
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
2011-05-03 06:06:57 -07:00
tuomaspelkonen
ea455376e6 Added a link to the actual object in Herald transcript.
Summary:
We were showing all kinds of information about the object in object
transcript, but there was no link to the actual object.

Test Plan:
Checked that links were working correctly for both differential and
commit objects.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 125
2011-04-12 11:35:26 -07:00
jungejason
cca849c762 Fix 'Transcript' links in Differential
Summary:
add filtering for MetaMTA transcripts, add Herald
transcripts, also fixed PhabricatorObjectHandleData to support commits.
Note that paging in the transcripts pages will be in a different diff.

Test Plan:
test the transcripts for both MetaMTA and Herald.

Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: jungejason, epriestley
Differential Revision: 114
2011-04-08 01:10:05 -07:00
epriestley
68687a059a Reintegrate Herald with Differential. 2011-04-07 20:59:51 -07:00
epriestley
361ec78b03 Add missing includes from XHPAST parse bug. 2011-04-06 23:14:58 -07:00
epriestley
11ea93260a Sync up UUIDs and create project configs. 2011-04-05 21:55:04 -07:00
epriestley
d0c9db125b Fix a herald rule editor warning. 2011-04-04 14:13:57 -07:00
epriestley
cc66c4890d Commit + Herald integration. 2011-04-04 14:13:14 -07:00
epriestley
2834e9f6ce HeraldCommitAdapter and some related junk. 2011-04-03 23:23:36 -07:00
epriestley
e407b2311e Typeahead, handle and herald integration for packages. 2011-04-03 22:23:31 -07:00
epriestley
370ba966db Rough cut of herald transcripts and Differential adapter. 2011-03-24 21:32:26 -07:00
epriestley
0b68b0f5bc Very rough cut at Herald test console. 2011-03-24 13:49:21 -07:00
epriestley
6c31ce4980 Herald CSS, plus edit/save/delete. 2011-03-24 11:07:36 -07:00
epriestley
deb12c9fe8 Some herald improvements. 2011-03-22 20:43:19 -07:00
epriestley
08fc13598d Improve search relevance. 2011-03-22 17:19:52 -07:00
epriestley
b060f0a80f Herald JS basics. 2011-03-22 17:08:08 -07:00
epriestley
9c5e7bb71d Horrendously rough cut of Herald rule editor. 2011-03-22 15:28:09 -07:00
epriestley
e8c960910c Herald 'new rule type' interface. 2011-03-22 14:35:11 -07:00
epriestley
75fcf56b67 Rough cut of Herald home controller. 2011-03-22 13:50:08 -07:00
epriestley
084c79d85a Herald skeleton. 2011-03-22 13:22:55 -07:00