Summary:
Ref T1279. This is a logical change.
- "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
- "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".
Practically, the primary net effect of this is just to make blocking reviews actually block.
This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.
Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7245
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.
Test Plan: Added Herald rules and updated revisions; see screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7244
Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.
This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.
Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7243
Summary: Ref T1279. If you accept a revision, also accept on behalf of all the projects you have authority to accept for.
Test Plan:
- Accepted a revision which I was a reviewer on, saw my own status and an authority project's status change to "Accepted".
- Accepted a revision which I was not a reviewer on, saw my own status be added (as "Accepted") and the project's status update.
Also, see screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7242
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:
- It puts queries very close to the display layer.
- We have to query for each revision if we want to figure out authority for several.
- We need to figure it out in several places, so we'll end up with copies of this logic.
- The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
- We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.
Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.
Test Plan:
- Looked at some revisions, verified the correct lines were highlighted.
- Looked at a revision I created and verified that projects I was a member of were not highlighted.
- With self-accept enabled, these //are// highlighted.
- Looked at a revision I did not create and verified that projects I was a member of were highlighted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7241
Summary: Ref T603. Fixes T3921. Tightens up policy controls for file/object relationships in existing applications.
Test Plan:
- Uploaded new project image, verified it got an edge to the project.
- Uploaded new profile image, verified it got an edge to me.
- Uploaded new macro image, verified it got an edge to the macro.
- Uploaded new paste via web UI and conduit, verified it got attached.
- Replaced, added images to a mock, verified they got edges.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3921, T603
Differential Revision: https://secure.phabricator.com/D7254
Summary:
Ref T603. Move toward stamping out all the Project / ProjectProfile query irregularities with respect to policies.
- Fixes a bug with Asana publishing when the remote task is deleted.
- Fixes an issue with Herald commit rules.
Test Plan:
- Viewed projects;
- edited projects;
- added and removed members from projects;
- republished Asana-bridged feed stories about commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7251
Summary: This adds setActionList to PropertyListView and properly places it in an archaic HTML 1.0 table.
Test Plan: test layouts with actions really tall or properties really tall. Always see a full height border.
Reviewers: epriestley, btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7239
Summary:
Ref T1279. This allows installs to implement two different flavors of project review. They can either implement this rule:
When:
[ ... ] [ ... ]
Take Action:
[ Add blockign reviewers ] [ Security ]
...which means "every revision matching X needs to be signed off by someone else on the Security team, //even if the author is on that team//". The alternative is to implement this rule:
When:
[ Author's projects ] [ do not include ] [ Security ]
[ ... ] [ ... ]
Take Action:
[ Add blocking reviewers ] [ Security ]
...which means that people on the Security team don't need a separate signoff from someone else on the team.
I think this weaker version maps to some of what, e.g., Google does (you need to be reviewed by someone with "readability" in a language, but if you have it that's good enough), but I could imagine cases like "Security" wanting to prevent self-review from satisfying the requirement.
@zeeg, not sure which of these use cases is relevant here, but either one should work after this.
Test Plan: Created rules with this field, verified it populated properly in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7238
Summary: Ref T1279. No logical changes, but cosmetically highlight stuff you have authority for, like we do in Diffusion.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7237
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.
I will probably write some lengthy treatise on how you shouldn't use this rule later.
Implementation is straightforward because Differential previously supported this rule.
This rule can also be used to add project reviewers.
Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7235
Summary: Ref T1279. Show separate sections for "Reviewers" and "Project Reviewers" (Differential) and for "Auditors" and "Package/Project Auditors" (Diffusion/Audit).
Test Plan:
- Looked at a commit. Saw separation.
- Looked at a revision. Saw separation.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7233
Summary:
Ref T1279. Two changes to the search/query for Differential:
- "Reviewers" now accepts users and projects.
- "Responsible Users" now includes revisions where a project you are a member of is a reviewer.
Test Plan:
- Searched for project reviewers.
- Verified that the dashboard now shows reviews which I'm only part of via project membership.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7231
Summary:
Ref T1279. No actual logical changes, but:
- You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
- You can now add projects as reviewers from the revision detail typeahead.
- You can now add projects as reviewers from the CLI (`#yoloswag`).
- Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).
I'll separate projects from users in the "Reviewers" tables in the next revision.
Test Plan:
- Added projects as reviewers using the web UI and CLI.
- Used `arc amend --show --revision Dnnn` to generate commit messages.
- Viewed revision with project reviewers in web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7230
Summary: Ref T1279. Updates status to 'accepted' or 'commented' when the user takes those actions.
Test Plan:
- Commented on a revision, got a comment icon.
- Accepted a revision, got an accept icon.
- Commented again, icon stayed as "accept".
- Faked the "old diff" states.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7229
Summary:
Ref T1279. No logical changes, just updates the reviewer display style.
We currently keep track of only "requested changes".
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7228
Summary:
Ref T1279. @champo did a lot of this work already; we've been doing double writes for a long time.
Add "double reads" (reading the edge table as both the "relationship" table and as the "reviewer status" table), and migrate all the data.
I'm not bothering to try to recover old reviewer status (e.g., we could infer from transactions who accepted old revisions) because it wold be very complicated and doesn't seem too valuable.
Test Plan:
- Without doing the migration, used Differential. Verified that reads and writes worked. Most of the data was there anyway since we've been double-writing.
- Performed the migration. Verified that everything was still unchanged.
- Dropped the edge table, verified all reviweer data vanished.
- Migrated again, verified the reviewer stuff was restored.
- Did various cc/reviewer/subscriber queries, got consistent results.
Reviewers: btrahan
Reviewed By: btrahan
CC: champo, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7227
Summary:
Ref T1279. This came to me in a dream.
The existing `differential_relationship` table has an `(objectPHID, type)` column, which theoretically is useful for queries like "revisions with X as a reviewer". In practice, I'm not sure it gets used much, but I can get it to show up in at least some query plans.
Add a similar index to the `edge` table. This sequences //before// D7227, which actually migrates the data.
Test Plan:
- Ran `bin/storage upgrade`.
- EXPLAIN'd a bunch of queries against different versions of the schema, this seemed helpful overall.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7232
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary:
Ref T603. This closes the other major policy loophole in Herald, which was that you could write a rule like:
When [Always], [Add me to CC]
...and end up getting email about everything. These rules are now enforced:
- For a //personal// rule to trigger, you must be able to see the object, and you must be able to use the application the object exists in.
- In contrast, //global// rules will //always// trigger.
Also fixes some small bugs:
- Policy control access to thumbnails was overly restrictive.
- The Pholio and Maniphest Herald rules applied only the //last// "Add CC" or "Add Project" rules, since each rule overwrote previous rules.
Test Plan:
- Created "always cc me" herald and maniphest rules with a normal user.
- Created task with "user" visibility, saw CC.
- Created task with "no one" visibility, saw no CC and error message in transcript ("user can't see the object").
- Restricted Maniphest to administrators and created a task with "user" visibility. Same deal.
- Created "user" and "no one" mocks and saw CC and no CC, respectively.
- Thumbnail in Pholio worked properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7224
Summary: Improves timeline legebility by pulling date inline with title in timeline mobile.
Test Plan: shrink screen for a task, see new layout
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7236
Summary:
Used `DifferentialRevisionQuery` with the relevant `need*()` calls in the test controller.
And started assuming the revision has reviewers and CC phids in `HeraldDifferentialRevisionAdapter`.
Test Plan:
Added herald rules that use revisions (one for revisions another for commit) and reviewers.
Created, accepted and landed a revision that matched the rules and checked all rules were applied.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6468
Conflicts:
src/applications/herald/adapter/HeraldCommitAdapter.php
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
src/applications/herald/controller/HeraldTestConsoleController.php
Summary:
maniphest tasks were fataling with priority 0 before making sure to add the return null if new object trick to the maniphest pro editor.
pholio had a problem where if you had no jpegs you were walking off array_rand. tighten the math and then just return a built-in if no uploaded user images could be found. Fixes T3889.
Test Plan: bin/lipsum generate for a few minutes and no errors
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3889
Differential Revision: https://secure.phabricator.com/D7222
Summary:
Ref T603. Herald transcripts potentially leak a bunch of content (task text, revision/commit content). Don't let users see them if they can't see the actual objects.
This is a little messy but ends up mostly reasonable-ish.
Test Plan:
- Verified that transcripts for objects I couldn't see no longer appear in the list, and reject access.
- Verified that transcripts for objects in applications I can't see reject access, albeit less gracefully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7221
Summary:
- 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
Summary: Adds an ObjectBox to Phabricator Registration
Test Plan: check logged out page for new header.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7223
Summary:
Ref T603. Herald is a bit of a policy minefield right now, although I think pretty much everything has straightforward solutions. This change:
- Introduces "create" and "create global" permisions for Herald.
- Maybe "create" is sort of redundant since there's no reason to have access to the application if not creating rules, but I think this won't be the case for most applications, so having an explicit "create" permission is more consistent.
- Add some application policy helper functions.
- Improve rendering a bit -- I think we probably need to build some `PolicyType` class, similar to `PHIDType`, to really get this right.
- Don't let users who can't use application X create Herald rules for application X.
- Remove Maniphest/Pholio rules when those applications are not installed.
Test Plan:
- Restricted access to Maniphest and uninstalled Pholio.
- Verified Pholio rules no longer appear for anyone.
- Verified Maniphest ruls no longer appear for restricted users.
- Verified users without CREATE_GLOBAL can not create global ruls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7219
Summary: Ref T603. This could be a nicer UX, but limit the amount of foot-shooting that users can possibly do. You can still manage if you're really tricky ("Members of project X", then leave the project) but this should make it hard to make a mistake. It seems very unlikely any user ever intends to lock themselves out of an application.
Test Plan: Set an application's view policy to permissive ("Administrators") and nonpermissive ("No One") values. The former were accepted, the latter rejected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7218
Summary: Right now emails don't include bookmark info (wasn't added in D2897). Lets include it so it's consistent with the web UI.
Test Plan: Inspected code, made sure it matched web UI code. Verified that web UI with these changes was consistent with rendering before refactoring.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7215
Summary:
Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'.
- There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant.
- The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally.
Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3829
Differential Revision: https://secure.phabricator.com/D7214
Summary:
Ref T603. I had to partially revert this earlier because it accidentally blocked access to Conduit and File data for installs without "policy.allow-public", since the applications are available to "all users" but some endpoints actually need to be available even when not logged in.
This readjusts the gating in the controller to properly apply application visibility restrictions, and then adds a giant pile of unit test coverage to make sure it sticks and all the weird cases are covered.
Test Plan:
- Added and executed unit tests.
- Executed most of the tests manually, by using logged in / admin / public / disabled users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7211
Summary:
Fixes T1461.
Adds
- FIELD_ALWAYS - now you could add this to a content type to always get notified
- FIELD_REPOSITORY_AUTOCLOSE_BRANCH - solves T1461
- CONDITION_UNCONDITIONALLY - used by these two fields to not show any value for the user to select
Test Plan: made a herald rule where diffs on autoclose branches would get flagged blue. made a diff on an autoclose branch and committed it. commit was flagged!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1461
Differential Revision: https://secure.phabricator.com/D7210
Summary:
In most cases this just makes the URIs more consistent, but it's funky/breakish for SVN repositories which are only partially tracked.
See also T3915, and IRC.
Test Plan:
- Browsed some repositories, verified URIs generated as expected, with trailing slashes for directories.
- Verified nothing goofy happened in the extremes (like double slashes on the first crumb).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7209
Summary: Ref T603. If you get in trouble, `bin/policy unlock PHID-APPS-PhabricatorApplicationDifferential` and such can get you out now.
Test Plan: Unlocked an application.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7206
Summary:
Ref T603. Enables:
- Application policies can be edited.
- Applications can define custom policies (this will be used for setting defaults, like "what is the default visibiltiy of new tasks", and meta-policies, like "who can create a task?").
Test Plan: Edited application policies. A future diff does more with custom policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7205
Summary:
Ref T603. Broadly, this allows you to implement a policy like "Only users in Engineering can use Differential."
This isn't complete, and there will be a long tail of special cases to deal with. Some examples:
- If you can't use Differential, should you still be able to attach/detach revisions from tasks?
- You currently will be able to.
- This actually seems pretty reasonable.
- But in other cases it might not be: the "send user a message" action should probably require access to Conpherence.
- If you can't use Differential, should you still be able to see feed stories about it?
- You currently will be able to, if you can see the revisions.
- This seems not-so-reasonable and we should probably lock it down.
- If you can't use Differential, can users CC you on revisions?
- Currently, they can, and you can't do anything about it.
- Probably they shouldn't be able to? This seems challenging to explain in the UI.
- If you can't use Differential, can you write a Herald rule against it?
- You currently will be able to.
- Seems like you obviously shouldn't be able to.
- I think this is a general issue right now (you can still write Differential herald rules even if you uninstall the application, I believe).
There are probably a few more things I haven't thought of. However, there are a finite number of these things and I suspect there aren't //too/ many more than this -- I can't come up with like 100 of them, and half of the ones above have easy fixes.
Despite the rough edges, I think this accomplishes 95% of what installs expect from it.
Test Plan: Restricted Differential and saw it vanish from the home page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7203
Summary:
I'm just going to store application policy settings (like view/edit policy, and default policies for content) in config, because:
1) We'll need access to it on every page, and Config is "free" since we already pull it.
2) Building separate storage and transactions seems like overkill, we get less-nice but pretty-reasonable transactions for free with config.
3) We could easily move it later if this is a bad call.
Also fix some formatting.
Test Plan: See future revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7202
Summary: Make the application query a little more flexible, and formalize the PHID type.
Test Plan: See next diffs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7201
Test Plan: Confirm the API returns a single flat result with a unified git diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, charles
Differential Revision: https://secure.phabricator.com/D7199
Test Plan:
Enable inline patches:
```
bin/config set metamta.differential.patch-format 'unified'
bin/config set metamta.differential.inline-patches 100000000
```
Create a new diff and confirm it renders correctly via email.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7198
Summary: Cleans up jump nav so it doesn't hard code a bunch of application behaviors. It still hard-codes a few, but few//er//?
Test Plan: Jumped to stuff like `D12`, `d`, `@dog`, `p admins only`, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7196
Summary: Ref T603. This didn't impact policies anyway, but using PhabricatorObjectQuery is far simpler and more general.
Test Plan: Used "Attach" dialog to find mocks, tasks, and revisions by "Dxx", "Mxx", etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7195
Summary: I removed the only callsite in D7179, but forgot to remove this code.
Test Plan: Grepped for callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7194
