Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.
This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.
Test Plan: Used `arc diff --create` to create several revisions with different data.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8452
Summary: Ref T2222. Ref T3886. This is a little early for general use, but the message parse/generate stuff requires CustomFields and FieldSpecifications to be closely aligned, so this provides at least a plausbile approach for any installs that run into trouble.
Test Plan: Viewed config; reordered fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222, T3886
Differential Revision: https://secure.phabricator.com/D8450
Summary:
Ref T2222.
- Restore mail tags for ApplicationTransactions mail.
- Restore subject line verbs.
- Denormalize line count and repository PHID.
- Fix an issue with the mailgun adapter where headers weren't attached properly.
Test Plan: Sent some mail, verified it had correct subjects and tags.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8378
Summary:
Ref T2222. Make the "EditPro" controller accommodate diff updates, and support the transaction type. This one is pretty straightforward.
Also make `revisionPHID` in the comments table nullable to fix the "Edit" action.
Test Plan:
- Created new revision.
- Updated revision.
- Tried to do some invalid stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8376
Summary: Ref T2222. This probably doesn't get everything, but should improve many of the newer transactions.
Test Plan: Looked at feed after making some edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8368
Summary: Ref T2222. This will probably have some rough edges for a bit (e.g., weird cases I didn't remember or think of), but there's no change to the underlying data and we can easily revert if things get too messy.
Test Plan: Looked at a variety of revisions and saw sensible output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8361
Summary:
Ref T2222. Ref T3886. Gets the storage-based fields working.
This requires future changes to actually do anything, all this code is inactive.
Test Plan: {F118863}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8357
Summary: Ref T2222. Ref T3886. Differential has a legacy storage table for auxiliary fields; move the data to modern storage.
Test Plan:
- Ran migration.
- Verified fields still worked properly afterward (view, edit, etc).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8355
Summary: Ref T2222. This enriches mail a little bit to get these rendering pretty much like they do now.
Test Plan: {F118255}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8343
Summary:
Ref T2222. This doesn't feel super clean, but doesn't feel too bad either.
Basically, Differential transactions can have secondary state-based effects (changing the overall revision status) when reviewers resign, are removed, accept, or reject revisions.
To deal with this in ApplicationTransactions, I did this:
- `applyFinalEffects()` can now alter the transaction set (notably, add new ones). This mostly matters for email, notifications and feed.
- In Differential, check for an overall revision state transition in `applyFinalEffects()` (e.g., your reject moving the revision to a rejected state).
- I'm only writing the transaction if the transition is implied and indirect.
- For example, if you "Plan Changes", that action changes the state on its own so there's no implicit state change transaction added.
The transactions themselves are kind of fluff, but it seems useful to keep a record of when state changes occurred in the transaction log. If people complain we can hide/remove them.
Test Plan: {F118143}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8339
Summary: Ref T2222. This mostly makes Accept/Reject work. The big missing piece is that overall revision status does not yet update properly. I need to think about how I want that to work a little bit more.
Test Plan: Accepted and rejected some stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8333
Summary: Ref T2222. This is obsolete and no longer used. We could deduce it from transactions or commits in modern Phabricator if we wanted it. We may implement a more general mechanism for T4434.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8330
Summary:
Ref T2222. This introduces two small new concepts:
- `expandTransactions()`: allows a transaction to expand into several transactions. For example, "resign" adds a "remove reviewers" transaction.
- We have some other cases which could use this, but currently hard-code things outside of the `Editor`.
- One example is that in Maniphest, closing a task implies claiming it if it is unowned.
- `setIgnoreOnNoEffect()`: The whole Editor can be set to continue or stop if any transactions have no effect, but this allows the behavior to be refined at the individual transaction level. This is primarily to make the UX less confusing, so the user gets only a single relevant error instead of one for each expanded transaction.
Otherwise, this is pretty straightforward.
Test Plan:
Rigged comment form to use SavePro controller, enabled resign action, then tried to resign from a bunch of stuff.
{F117743}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8328
Summary: Ref T2222. Implements the simpler actions (abandon, reclaim, close, reopen, plan changes, request review) in a transactional way with validation and effect checks.
Test Plan:
- Rigged submissions to point at the Pro controller.
- Rigged dropdown to have all these options all the time.
- Tried to apply about 20-30 of these operations to various revisions and always got the expected result (success, error, or no-op).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8307
Summary: Ref T2222. Adds a mostly-functional "Pro" comment controller. This does the core stuff, but does not yet do actions (accept, reject, etc.) or inline comments.
Test Plan: Changed the `if (false)` to an `if (true)`, then made some comments, etc. This is normally unreachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8304
Summary:
Ref T2222. Differential has custom code for managing subscriptions, but no longer requires it.
The one trick is that we don't have a hook for loading related data on the subscriptions workflow right now. Just glue that in for the moment; it's relatively harmless, and once Diffusion converts we'll have more context on how to best surface it properly.
Test Plan: Subscribed and unsubscribed from a revision. Viewed different revisions and saw correct subscription state.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8293
Summary: Ref T3886. Now that a custom field can emit a core transaction, just emit a subscribers transaction.
Test Plan: {F116014}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8289
Summary:
Ref T3886. Ref T418.
- Adds "View Policy" and "Edit Policy" fields.
- Allows CustomFields to produce arbitrary types of transactions, so these fields can produce standard view/edit policy transactions and get all the strings and validation associated with them.
Test Plan: {F116001}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886
Differential Revision: https://secure.phabricator.com/D8287
Summary:
Ref T3886.
- Adds "Summary" field.
- Adds "CoreField" for fields stored on the actual object, to reduce code duplication a bit for the main fields.
Test Plan: {F115902}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8283
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8294
Summary:
Ref T3496. Currently, we call loadAssets() on each revision table, which invokes a new revision query and a pile of subqueries.
Instead, add `needFlags()` and `needDrafts()` to `RevisionQuery`. Some day these could perhaps be more generic.
Test Plan:
- Viewed home, differential, etc., no longer saw 9203809238 queries being run for no reason.
- Drafts and flags still appear properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8277
Summary:
Ref T3886. I spent a few hours trying to make `DifferentialFieldSpecification` extend `PhabricatorCustomField` so I could be more blunt in my approach here and just swap the whole thing over in one go (more or less like I did with Maniphest) but we have a ton of custom fields and things felt really shaky and the change was enormous and hard to keep track of.
Instead, I'm going to do this more gradually and go field-by-field. This implements a CustomField version of the "Title" field.
(There are no links to this in the UI.)
Test Plan:
{F115353}
{F115354}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8276
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.
Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8275
Summary:
Ref T2222. I want to stage a "later" patch to drop this column, but get rid of the last few references to it.
One of these methods has no callers, and the other stuff I've updated to use the modern fields.
Test Plan: Created some inlines, hit "edit", submitted them, `grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8240
Summary: Ref T2222. Once these are live, yell if any of them seem off. I tried to mostly stay consistent-ish with what we had before.
Test Plan: Looked at a bunch of revisions and saw more detailed, colorful transactions.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8223
Summary:
Ref T2222. This gets rid of Differential's custom view and uses a standard view instead.
This also mostly fixes the rendering logic for inlines.
This is headed to the `tmp.differential` branch.
Test Plan: {F112696}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8215
Summary:
Ref T2222. This is the big one.
This migrates each `DifferentialComment` to one or more ApplicationTransactions (action, cc, reviewers, update, comment, inlines), and makes `DifferentialComment` a double-reader for ApplicationTransactions.
The migration is pretty straightforward:
- If a comment took an action not otherwise covered, it gets an "action" transaction. This is something like "epriestley abandoned this revision.".
- If a comment updated the diff, it gets an "updated diff" transaction. Very old transactions of this type may not have a diff ID (probably only at Facebook).
- If a comment added or removed reviewers, it gets a "changed reviewers" transaction.
- If a comment added CCs, it gets a "subscribers" transaction.
- If a comment added comment text, it gets a "comment" transaction.
- For each inline attached to a comment, we generate an "inline" transaction.
Most comments generate a small number of transactions, but a few generate a significant number.
At HEAD, the code is basically already doing this, so comments in the last day or two already obey these rules, roughly, and will all generate only one transaction (except inlines).
Because we've already preallocated PHIDs in the comment text table, we only need to write to the transaction table.
NOTE: This significantly degrades Differential, making inline comments pretty much useless (they each get their own transaction, and don't show line numbers or files). The data is all fine, but the UI is garbage now. This needs to be fixed before we can deploy this to users, but it's easily separable since it's all just display code.
Specifically, they look like this:
{F112270}
Test Plan:
I've migrated locally and put things through their paces, but it's hard to catch sketchy stuff locally because most of my test data is nonsense and bad migrations wouldn't necessarily look out of place.
IMPORTANT: I'm planning to push this to a branch and then shift production over to the branch, and run it for a day or two before bringing it to master.
I generally feel good about this change: it's not that big since we were able to separate a lot of pieces out of it, and it's pretty straightforward. That said, it's still one of the most scary/dangerous changes we've ever made.
Reviewers: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8210
Summary: Ref T2222. We need this `clone` when constructing the new multi-comments in Differential, or we get double-comments internally. This shows up as emails with double comment text.
Test Plan: Sent some "Accept + comment" emails, only one comment in the body.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8206
Summary:
Ref T2222. Ref T4415. We're still writing Differential subscription stuff into this weird legacy `differential_relationship` table, which is like an edge table but extremely ancient.
Move it into a proper table.
I've removed `withSubscriptions()` from `DifferentialRevisionQuery`. It was weird, doesn't work consistently with other similar filters, and was only used by the API. Now it means "ccs", which is consistent with the ApplicationSearch UI and with Maniphest.
Test Plan:
Without migrating, added and removed subscribers via various workflows. Queried for subscribers. Everything worked as expected.
Ran the migration, verified data survived.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222, T4415
Differential Revision: https://secure.phabricator.com/D8202
Summary:
See D8200. Ref T2222. Instead of writing one comment which can have a ton of different effects, write a series of one-effect comments. These will be easier to convert into ApplicationTransactions.
This has a minor user-facing effect of making these multiple-action comments render separately:
{F111919}
Once the migration completes, they should automatically merge together nicely again.
Test Plan: Made a bunch of comments and took a bunch of actions, all of which worked normally except that they rendered as several things instead of just one.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, FacebookPOC
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8201
Summary:
Ref T2222. Instead of writing one comment which performs both a diff update and adds a comment, write two comments, one for each action. These will translate directly into ApplicationTransactions writes.
This has a small impact on the UX: these updates now render in two rows, instead of one. After T2222, they'll automerge back together.
{F111909}
Test Plan: Updated a revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8200
Summary:
Ref T2222. Currently, `DifferentialComment` stores both (a) the text of comments and (b) various other transaction details. This data needs to map to both Transactions and TransactionComments in the long run. This diff separates out all the data which is bound for the TransactionComment table, so that when we migrate `DifferentialComment` itself it will //only// need to migrate into the Transactions table. This is a much simpler migration than the inline comment one was, partly because it set up infrastructure and partly because the data is less complex.
Basically, I'm just proxying the read/write for the comment text into the other table. All readers already go through the Query class, and there are only three writers (preview, comment, implicit comment on diff update) which are all highly regular and straightforward to test.
We can also back out of this diff very easily: doing double writes cost only one line of code (`$this->content = $content;`) so we have proper double writes and a trivial revert path.
Test Plan:
- Without migrating, added comments and saw them show up.
- Migrated.
- Saw all the old comments, and no damage to the new ones.
- Added new comments.
- Used comment preview.
- Updated a revision to implicitly create an update comment and verified it looked OK.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8196
Summary:
Moves away from ArcanistProjects:
- Adds storage for diffs to be directly associated with a repository (instead of indirectly, through arcanist projects). Not really populated yet.
- Drops `parentRevisionID`, which is obsoleted by the "Depends On" edge. This is not exposed in the UI anywhere and doesn't do anything. Resolves TODO.
Test Plan: Ran storage upgrades, browsed around, lots of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8072
Summary:
Ref T1049. Adds `bin/harbormaster` and `bin/harbormaster build` for applying plans from the console. Since this gets `--trace`, it's much easier to debug what's going on.
This doesn't work properly with some of the Drydock steps yet, I need to look at those. I think `setRunAllTasksInProcess` probably obsoletes some of the mechanisms. It might also not work with "Wait for Builds" but I didn't check.
Test Plan: Used `bin/harbormaster` to run a bunch of builds. Ran builds from web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7825
Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.
The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.
To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.
Also delete the very old COMMITTED status, which has been obsolete for over a year.
Test Plan: Browsed around most/all of the affected interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7653
Summary:
Ref T1049. Ref T2222. `DifferentialDiff` does not currently have a PHID, but we need it for Harbormaster and ApplicationTransactions. See some discussion in D7501.
(I split the SQL into two sections so we can't fail in the middle. At some point, I'd like to do a pass on the migration stuff and get this happening automatically, and also simplify the PatchList.)
Test Plan:
- Ran `bin/storage upgrade`.
- Checked for valid PHIDs in the database.
- Used `phid.query` to look up a diff by PHID.
- Created a new diff and verified it got a PHID.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2222, T1049
Differential Revision: https://secure.phabricator.com/D7513
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
Summary:
Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions.
Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc.
Test Plan: Browsed several revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3415, T2222
Differential Revision: https://secure.phabricator.com/D7375
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.
The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.
Two risks here:
- I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
- This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.
I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.
Test Plan:
- Before migrating, then after migrating:
- Made a bunch of inlines (drafts, submitted).
- Edited and deleted inlines.
- Verified inlines showed up in preview.
- Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
- Verified that inlines ARE indexed when they're not drafts.
- Verified that drafts inlines make revisions appear as "with draft" in the revision list.
- Made left, right, and draft inlines.
- Migrated (`bin/storage upgrade`).
- Verified that my inlines from before the migration still showed up.
- (Repeated all the stuff above.)
- Manually inspected the inline comment table.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7139
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
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.
I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.
Test Plan: Created new revisions from the CLI and conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7269
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.
Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.
That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.
Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.
Also provide more context, and more useful exception messages.
Test Plan:
- See screenshots.
- Also triggered a policy exception and verified it was dramatically more useful than it used to be.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7260
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 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
