1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 00:02:41 +01:00
Commit graph

1163 commits

Author SHA1 Message Date
epriestley
9292433dae Implement "Resign" action against ApplicationTransactions
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
2014-02-25 12:36:02 -08:00
epriestley
4eadcf975a Implement most Differential actions against ApplicationTransactions
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
2014-02-24 15:57:49 -08:00
epriestley
d48a88d4cd Add inline comment support to "Pro" comment save controller
Summary: Ref T2222. Makes the "pro" controller work with inlines.

Test Plan: Added a bunch of inlines and saved them with the "pro" controller.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8306
2014-02-24 15:57:35 -08:00
epriestley
c7d208fda1 Add a "Pro" version of the Differential comment save controller
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
2014-02-24 15:57:26 -08:00
epriestley
3a8eb28a07 Send basic email from new ApplicationTransaction editor
Summary: Ref T2222. Adds basic support for email.

Test Plan: Received an email via `/editpro/`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8303
2014-02-24 15:57:16 -08:00
epriestley
d94c33b61a Use a more modern design for "no reviewers" warning in Differential
Summary: Ref T2222. Currently this is a giant header box thing. Move it into the ObjectBox.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8301
2014-02-21 15:10:58 -08:00
epriestley
b62420e6e4 Separate reading object values out of didSetObject() in CustomField
Summary:
Ref T3886. Broadly, fields break down into two types right now: fields which store data on the object (like `DifferentialTitleField`) and fields which store data in custom field storage.

The former type generally reads data from the object into local storage prior to editing, then writes it back afterward. Currently, this happens in `didSetObject()`.

However, now that we load and set objects from ApplicationTransactionQuery, we'll do this extra read-field-values on view interfaces too. There, it's unnecessary and sometimes throws data-attached exceptions.

Instead, separate these concepts, and do all the read-from-object / read-from-storage in one logical chunk, separate from `didSetObject()`.

Test Plan:
  - Edited Differential revision.
  - Edited Maniphest task.
  - Edited Project.
  - Edited user profile.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8299
2014-02-21 14:44:01 -08:00
epriestley
a84cc1ab73 Fix (?) an issue with CCs not being written into commit messages properly
Summary: Report from @zeeg, I think this is the root issue. Currently, if a project is CC'd we'll write "CC: projectname", but should write "CC: #projectname".

Test Plan: Verified that we now write "CC: #projectname".

Reviewers: btrahan, zeeg

Reviewed By: zeeg

CC: zeeg, aran

Differential Revision: https://secure.phabricator.com/D8296
2014-02-21 12:56:38 -08:00
epriestley
a24605432f Use standard rendering and controller for Differential subscriptions
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
2014-02-21 11:55:35 -08:00
epriestley
c5ba75ee9e Implement a "Reviewers" CustomField
Summary:
Ref T3886:

  - Adds a "Reviewers" field as a modern CustomField.

Ref T418:

  - Allows CustomFields to emit transaction metadata.

Test Plan: {F116254}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418, T3886

Differential Revision: https://secure.phabricator.com/D8291
2014-02-21 11:54:32 -08:00
epriestley
aa7ba4c6e6 Implement Differential subscribers as a CustomField
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
2014-02-21 11:54:08 -08:00
epriestley
f91e94eb90 Implement view and edit policies in Differential CustomFields
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
2014-02-21 11:53:48 -08:00
epriestley
01572d9d93 Implement "Repository" as a new-style CustomField in Differential
Summary:
Ref T3886. Ref T418.

  - Adds new capabilities for CustomField:
    - Controls can now bulk-load PHIDs (e.g., for tokenizers).
    - Transactions can now bulk-load PHIDs (e.g., for relationship changes).
  - Implements "Repository" control.
  - Improves tokenizer StandardCustomField controls.

Test Plan:
{F115942}

{F115943}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418, T3886

Differential Revision: https://secure.phabricator.com/D8286
2014-02-21 11:53:37 -08:00
epriestley
05f2ab32d8 Implement "Test Plan" as a CustomField
Summary: Ref T3886. Moves some of the "required" logic to the base class ("DifferentialCoreField") so Title and Test Plan can share it.

Test Plan: Edited revisions using "pro" editor, saw test plan transactions.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8285
2014-02-21 11:53:27 -08:00
epriestley
88227d26bc Allow CustomField to provide ApplicationTransaction change details
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.

Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.

Test Plan:
{F115918}

  - Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T418

Differential Revision: https://secure.phabricator.com/D8284
2014-02-21 11:53:04 -08:00
epriestley
be262f4b0c Implement new-style "Summary" field
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
2014-02-21 11:52:52 -08:00
epriestley
92ac8b5ad9 Make Differential inline comment rendering more consistent and somewhat more modern
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
2014-02-21 11:52:03 -08:00
epriestley
50ed42761c Don't issue a bazillion queries to load Differential object lists
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
2014-02-18 17:57:45 -08:00
epriestley
65bc2b1ac5 Implement "Pro" version of revision editor, with one field
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
2014-02-18 16:32:55 -08:00
Bob Trahan
dcd7a316d2 Differential - add DifferentialDraft to track whether revisions have draft feedback or not
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
2014-02-18 16:25:16 -08:00
epriestley
f3cbc0e006 Move many task status hardcodes into ManiphestTaskStatus
Summary:
Ref T1812. This cleans up most of the easy hard-coded references to specific statuses:

  - The "fixes" language moves into ManiphestTaskStatus.
  - Add a method to list open statuses.
  - Add a method to test if a status is open.
  - Add a method to get default status for new tasks.

Test Plan: Browsed around, lint, grep, created, filtered and updated tasks.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1812

Differential Revision: https://secure.phabricator.com/D8264
2014-02-17 15:59:31 -08:00
epriestley
8a725ece0f Bring "fixes x as y" parser forward and use new parsers instead of old ones
Summary: Fixes T3872. Ref T1812. Ref T3886. Modernize the "closes x as y" string parser, and use all the new parsers instead of the old ones.

Test Plan: Made a commit full of a pile of these trigger strings, then used `scripts/repository/reparse.php --message` to reparse it. Verified that parses came back as expected using a bunch of `var_dump()`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1812, T3872, T3886

Differential Revision: https://secure.phabricator.com/D8263
2014-02-17 15:59:13 -08:00
epriestley
5afddb6703 Bring "Reverts X" to more general infrastructure and port unit tests
Summary:
Ref T3886. See D8261. This brings the "reverts x" phrase to modern infrastructure. It isn't actually called by the real parser yet, I'm going to do that in one go at the end so I can test everything more easily.

This had unit tests; port most of them forward.

Test Plan: Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

Differential Revision: https://secure.phabricator.com/D8262
2014-02-17 15:58:59 -08:00
epriestley
d016cac915 Modularize parser for "Closes task X as Y"
Summary:
Ref T3886. Ref T3872. Ref T1812. We have several parsers which look for textual references to other objects, like:

  Closes Tx.
  Depends on Dy.
  Reverts Dz.

Currently, these are pretty hard coded, don't get all the edge cases right, and don't generalize well. They're also implemented in the middle of Differential's field code. So I want to:

  - Share more code so that, e.g., "Tx, Ty" always works (only some rules support it right now);
  - fix bugs in the parser, like T3872;
  - make this a modular, extensible process which runs against custom fields, not a builtin part of fields;
  - make the internals more flexible to accommodate custom stuff like T1812.

This implements the "Verbs optional-noun Object, Optional Other Objects optional-as-something." grammar in a general way so subclasses can just plug in their keywords. Runtime code doesn't touch this yet.

Test Plan: Ran unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3872, T1812, T3886

Differential Revision: https://secure.phabricator.com/D8261
2014-02-17 15:53:47 -08:00
epriestley
04ba976a04 Remove references to legacy comment IDs
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
2014-02-14 15:56:25 -08:00
epriestley
23146f2bb9 Render Differential previews through ApplicationTransactions
Summary: Ref T2222. Use new code for rendering. Delete `DifferentialRevisionCommentView`, which has no remaining callsites.

Test Plan: Went through all the different actions and verified the previews rendered correctly. Reloaded page to test draft behavior.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8236
2014-02-14 13:16:58 -08:00
epriestley
a0262c0b4f Remove tokenizer.ondemand, and always load on demand
Summary:
Ref T4420. Tokenizers currently operate in "preload" or "ondemand" modes. In the former mode, which is default, they'll try to load the entire result list when a page loads.

The theory here was that this would slightly improve the experience for small installs, and once they got big enough they could switch to "ondemand". In practice, several issues have arisen:

  - We generally don't have a good mechanism for telling installs that they should tweak perf config -- `metamta.send-immediately` is the canonical example here. Some large installs are probably affected negatively by not knowing to change this setting, and having settings like this is generally annoying.
  - We have way way too much config now.
  - With the advent of ApplicationSearch, pages like Maniphest make many redundant loads to prefill sources like projects. Most of the time, this data is not used. It's far simpler to switch everything to ondemand than try to deal with this, and dealing with this would mean creating two very complex divergent pathways in the codebase for a mostly theoretical performance benefit which only impacts tiny installs.
  - We've been using `tokenizer.ondemand` forever on `secure.phabricator.com` since we have many thousands of user accounts, and it doesn't seem sluggish and works properly.

Removing this config is an easy fix which makes the codebase simpler.

I've retained the ability to use preloaded sources, since they may make sense in some cases (in at least one case -- task priorities -- adding a static source pathway might make sense), and they're part of Javelin itself. However, the code will no longer ever go down that pathway.

Test Plan: Used `secure.phabricator.com` for years with this setting enabled.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4420

Differential Revision: https://secure.phabricator.com/D8232
2014-02-14 10:24:40 -08:00
Chad Little
390a0f91b0 Remove spacing after timeline on diffs
Summary: I assume this box is always after timeline

Test Plan: test a diff or two

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8230
2014-02-13 21:02:19 -08:00
epriestley
7374a0a4d4 Fix inline comment links for non-visible comments
Summary: Ref T2222. Restore this funky is-visible / inline-is-elsewhere logic.

Test Plan: Updated a revision, saw all the inlines render properly when looking at various diffs and versus-diffs. Clicked inline links.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8224
2014-02-13 16:52:48 -08:00
epriestley
9afe52de51 Provide transaction strings, icons and colors for Differential
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
2014-02-13 15:51:26 -08:00
epriestley
cb20205aee Don't show "edit" links in Differential for now
Summary: Ref T2222. These don't work yet. We just have to copy a couple fields, but let's sort that out later since this is purely a new feature.

Test Plan: Looked at a revision, no edit links.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8222
2014-02-13 15:51:26 -08:00
epriestley
c7ca3cd4ab Pass $all_changesets, not $changesets, to inline rendering logic
Summary: `$all_changesets` has all the changesets.

Auditors: btrahan
2014-02-13 15:12:42 -08:00
epriestley
ee38e33496 Fix two issues with Differential Transactions
Summary: With symbols, the transactions fataled on `getID()`. Also my line
number sorting was a bit goofy.

Auditors: btrahan
2014-02-13 15:06:38 -08:00
epriestley
29918e1bd4 Fix double-rendering of inline comments in mail
Summary: Ref T2222. This is a `tmp.differential`-only issue. Inline comment transactions now have content, so we treat them like body text. We also render them separately as inline text. This produces mail where inlines are rendered twice.

Test Plan: Sent myself mail, saw only one copy of inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8216
2014-02-13 15:00:29 -08:00
epriestley
62cb584083 Use timeline view in Differential and make inlines somewhat usable again
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
2014-02-13 15:00:29 -08:00
epriestley
0dc73d5d93 Always provide a valid content source for DifferentialCommentEditor
Summary: Ref T2222. On the `tmp.differential` branch, we're currently having
issues parsing commits which reference Differential revisions, because the
"user closed this revision (closed by commit xyz)" message is fataling:

  [2014-02-13 14:12:36] EXCEPTION: (PhutilProxyException) Error while
  executing task ID 345358 from queue. {>} (AphrontQueryException)
  #1048: Column 'contentSource' cannot be null

Specifically, the MessageParser pathway for CommentEditor doesn't set a content
source. Make sure CommentEditor always sets a content source.

(This is also causing a buildup of diffs on D8212 and D8211.)

Auditors: btrahan
2014-02-13 06:19:55 -08:00
epriestley
18938b5310 Migrate Differential comments to ApplicationTransactions
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
2014-02-12 14:34:48 -08:00
epriestley
3092efdc65 Reduce reliance on getRevisionID() on DifferentialComment
Summary: Ref T2222. A few rendering interfaces rely on fishing the revision ID out of a DifferentialComment, but it will only have the PHID soon. Pass in the revision and use it to determine the ID instead.

Test Plan: Browsed, previewed, examined comments. Clicked anchors.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8209
2014-02-12 14:34:26 -08:00
epriestley
143b89286a Remove unreachable DifferentialRevisionStatsView
Summary:
Ref T2222. I wiped out the Differential-specific stats page a long time ago, but missed this. It turned up recently in `grep`.

Facts will eventually fill this role; this code is unreachable; it probably doesn't work now and definitely won't work in a day or two after ApplicationTransactions.

Test Plan: Used `grep` to look for callsites.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8208
2014-02-12 14:33:55 -08:00
epriestley
54ce4a719e Fix issue where new transaction comments can be spread across multiple transactions
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
2014-02-12 09:06:24 -08:00
epriestley
8f9b7f4196 Move Differential to proper subscriptions
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
2014-02-12 08:53:40 -08:00
epriestley
1dbfc56d35 Write one DifferentialComment per CommentEditor action
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
2014-02-11 15:21:21 -08:00
epriestley
c65fad3fca Separate revision updates into two separate comments
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
2014-02-11 15:21:14 -08:00
epriestley
8e232a8bf1 Allow Differential Mail to accept multiple comments for one mail
Summary:
Ref T2222. Currently, one `DifferentialComment` can do a lot of things (add ccs, reviewers, comments, inline comments, and perform state changes). In the future, each `ApplicationTransaction` does only one thing. This is the primary piece of complexity which makes the upcoming migration risky, because each comment needs to migrate into multiple transactions.

I want to mitigate this complexity as much as possible before the migration itself happens. One approach I'm going to use to do that is to start writing one comment per effect now, so the mapping is more direct when the migration itself happens and the write code can be straightforward (one row per save()) after the migration.

This tackles a small piece of that, which is the mail Differential sends. Currently, Differential mail acts on a single comment. Instead, allow it to act on a list of comments, but always give it one comment for now. In the future, we can hand it several comments instead and still get the expected behavior.

This change should have no impact on any application behaviors.

Test Plan:
  - Commented;
  - commented with inline;
  - added reviewers;
  - added CCs;
  - added CCs via mentions;
  - updated revision;
  - looked at all the mail, all of which seemed sane.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8199
2014-02-11 15:21:07 -08:00
epriestley
305fb3fbd9 Migrate all Differential comment text into new storage
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
2014-02-11 11:34:15 -08:00
Chad Little
ee44c6db53 Translate Differential Feed/Comment/Asana stories
Summary: I don't think this is too terrible, and makes the future easier? Maybe?

Test Plan: ALLCAPS translation, Viewed a diff, feed, and notifications.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8175
2014-02-09 10:07:29 -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
e7d671c8d3 Index "Closed" and "Unowned" relationships explicitly
Summary: Ref T4365. Two diffs from now, I'm changing the UI a bit to let you search for closed and unowned documents more explcitly. To support this in ElasticSearch and more easily in MySQL search, make these explicit, positive relationships.

Test Plan: `bin/search index --all`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4365

Differential Revision: https://secure.phabricator.com/D8122
2014-02-03 12:51:18 -08:00
Aviv Eyal
d0e30882a1 Add Disabled mode to landing revision ui
Summary:
Fixes T4066.

add `isActionDisabled()` to DifferentialLandingStrategy, which also explains why it is so.
Make an appropriate pop-up in the controller.

Also make the whole UI "workflow", and convert `createMenuItems()` to `createMenuItem()` (Singular).

Test Plan: Click "Land to..." button in all kinds of revisions.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4066

Differential Revision: https://secure.phabricator.com/D8105
2014-01-30 09:07:50 -08:00
epriestley
08225bd860 Remove policy caveat from Differential
Summary: Policies are now fully supported in Differential.

Test Plan: Grepped for other caveats, looks like I've already removed htem all.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8095
2014-01-29 11:44:10 -08:00