Summary:
Ref T2222. Brings the major mail features (affected files, patches) forward.
This drops some of the minor integrations which just show object state (like "Maniphest Tasks") since I think they're not very important. I'll put them back if users miss them.
Test Plan: Sent mail with inline/attached patches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8459
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 T3794. Medium term goal is to remove `DifferentialRevisionEditor`. This removes one of two callsites.
Test Plan: Used `arc diff --edit` to repeatedly update a revision, making changes to various fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3794, T2222
Differential Revision: https://secure.phabricator.com/D8451
Summary: Ref T2222. Update this callsite; pretty straightforward.
Test Plan: Used Conduit to take actions and saw their effects in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8442
Summary: Ref T2222. Straightforward update to new stuff.
Test Plan:
- Tried to close an uncloseable revision.
- Closed a closeable revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8425
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
Summary:
Ref T2222. Ref T4484. This is a stepping stone to getting Herald supported in the new Differental code. Generally:
- Instead of an Editor either supporting or not supporting Herald, let it choose based on transactions. Specifically, Differential only runs rules on revision creation and diff updates.
- Optionally, allow an Editor to return some transactions to apply instead of having to apply everything itself. This lets us make it clear why changes happend in the transaction log, and share more code.
- I updated only one transaction type (owners in Maniphest) since it was the easiest and cleanest to update and test. Everything else still works like it used to, it just won't generate a transaction record yet.
- The transaction records are a touch rough, but we can clean them up later.
Test Plan: {F122282}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4484, T2222
Differential Revision: https://secure.phabricator.com/D8404
Summary:
Ref T2222. Five very small improvements:
- I hit this exception and it took a bit to understand which transaction was causing problems. Add an `Exception` subclass which does a better job of making the message debuggable.
- The `oldValue` of a transaction may be `null`, legitimately (for example, changing the `repositoryPHID` for a revision from `null` to some valid PHID). Do a check to see if `setOldValue()` has been called, instead of a check for a `null` value.
- Add an additional check for the other case (shouldn't have a value, but does).
- When we're not generating a value, don't bother calling the code to generate it. The best case scenario is that it has no effect; any effect it might have (changing the value) is always wrong.
- Maniphest didn't fall back to the parent correctly when computing this flag, so it got the wrong result for `CustomField` transactions.
Test Plan: Resolved the issue I was hitting more easily, made updates to a `null`-valued custom field, and applied other normal sorts of transactions successfully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4557, T2222
Differential Revision: https://secure.phabricator.com/D8401
Summary:
Fixes T4550 by changing supportsFeed to shouldPublishFeedStory, so things can be more granular like that are with mail. Attempts to fix things generally too, filtering out xactions that have no business in feed, etc.
Also return an updated Task HTML representation on drag and drop moves, etc. This is important so if the priority changes you can see it reflected in the UI.
Test Plan: dragged tasks around. observed no feed stories on subpriority drags. observed feed stories and updated color bars on stories that changed priority
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4550
Differential Revision: https://secure.phabricator.com/D8399
Summary:
adds ManiphestTransaction::TYPE_PROJECT_COLUMN and makes it work. Had to clean up the Javascript ever so slightly as it was sending up the string "null" when it should just omit the data in these cases. Ref T4422.
NOTE: this is overall a bit buggy - e.g. move a task Foo from column A to top of column B; refresh; task Foo is at bottom of column B and should be at top of column B - but I plan on additional diff or three to clean this up.
Test Plan: dragged tasks around columns. clicked on those tasks and saw some nice transactions.
Reviewers: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4422
Differential Revision: https://secure.phabricator.com/D8366
Summary:
Ref T2222. Differential has certain "words of power" (like `Ref T123` or `Depends on D345`) which should expand into a separate transaction when they appear anywhere in text.
Currently, they're respected in only some fields. I'm expanding them to work in any remarkup field, including comments and inline comments.
This partially generalizes transaction expansion/extraction in comments. Eventually, I'll probably implement some very soft sort of reference edge for T4036, maybe.
Test Plan: {F119368}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8369
Summary: Ref T2222. This updates the new JIRA field to be editable.
Test Plan: Used `/editpro/` to edit associated JIRA issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8364
Summary:
The intent of getOrBuildEngine() is to save some boilerplate in cases where we're just using a standard engine, but it didn't get cached so we'd rebuilt it over and over again.
This was especially bad in Differential with a large number of inlines.
Test Plan: "Query" tab of services is no longer quite so ridiculous in Differential.
Reviewers: btrahan, dctrwatson
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8352
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 requires one new trick:
- When merging edge transactions which both add/update an edge, the Editor gets to control how the edge data is merged.
Specifically, we pick the "strongest" state to keep, so "accept + comment" leaves you with an accept instead of a comment.
Test Plan: Accepted, commented on, and comment + accepted revisions. Added some debugging dumps to verify that the merging was getting hit and working correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8340
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 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. 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
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 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. 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
Summary: so it was said in IRC and so it is true
Test Plan: saving maniphest tasks with custom fields no longer barfs
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8279
Summary:
Ref T3886. Fixes the removed TODO. This also implements the generally reasonable policy "you have to be able to see an object in order to see its transactions". That was implicit before (we never load transactions without loading an object first) but is now explicit.
This fixes bad (nonspecialized) rendering of custom field transactions in Projects, and shortly in Differential, where stories would read "alincoln edited this object." instead of a more specific string.
Test Plan: Viewed a project edit, saw a more specific string. Browed ApplicationTransaction applications.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8273
Summary:
Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions.
This might need some #design help.
Test Plan:
The tricky part here is the anchor rule, which deals with the case where you follow a link to `T123#4`, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest:
{F112891}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8229
Summary: Fixes T4423. This works better and is simpler and more flexible.
Test Plan: Clicked "(Show Details)", got a popup with the details.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4423
Differential Revision: https://secure.phabricator.com/D8227
Summary:
Updates PhabricatorTimeline to PHUITimeline. Uses standard colors and spacing, softens up the actors, and reduces visual spacing of action-only events.
- Also updated some 2x sprite images.
Test Plan: Tested Tasks Paste and Pholio in my sandbox.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8205
Summary:
Ref T4379. Fixes T4359. Currently, `bin/search index` does not rebuild CustomField indexes. This is because they aren't really part of the main search index. However, from a user's point of view this is by far the most logical place to look for index rebuilds, and it's straightforward for us to write into this secondary store.
At some point, it might be nice to let you specify fields as "fulltext" too, although no one has asked for that yet. We could then dump the text of those fields into the fulltext index. Ref T418.
Test Plan: Used `bin/search index --type proj --trace`, etc., and examination of the database to verify that indexes rebuilt. Reindexed users, tasks, projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4359, T418, T4379
Differential Revision: https://secure.phabricator.com/D8185
Summary:
Ref T4379. Currently, you can edit away your edit capability in Projects. Prevent this in a general way.
Since some objects have complex edit policies (like "the owner can always edit"), we can't just check the value itself. We also can't fairly assume that every object has a `setEditPolicy()` method, even though almost all do right now. Instead, provide a way to pretend we've completed the edit and changed the policy.
Test Plan: Unit tests, tried to edit away my edit capability.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4379
Differential Revision: https://secure.phabricator.com/D8179
Summary:
Ref T4379. Perform all editing with modern transaction infrastructure. A few practical changes here:
- Message for "project name required" should be a little nicer. I'll deal with this once more stuff gets straightened out. You get a reasonable message now, it's just not nicely handled as part of the form.
- Message for "project name is not unique" should be a little nicer. Same as above.
- Previously, we would automatically archive a project when the last member left or was removed. I'll probably restore this in a bit but am omitting it for the moment for simplicity.
- Previously, we would create projects with goofy nonsensical permissions. Now we create them with reasonable permissions.
Test Plan:
- Created project.
- Edited project.
- Ran unit tests.
- Viewed project edit history.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4379
Differential Revision: https://secure.phabricator.com/D8168
Summary:
Ref T4379. When you add a redundant edge, we currently compare the values strictly, using `===`. However, the old and new versions of the edge have slightly different member data, because one has been synthetically constructed and one has been read from the database.
Instead, compare only the things we actually care about:
# Were any destintations added or removed?
# Was any edge data changed?
If the answer to both questions is "no", consider the update a no-op.
Test Plan: In the next diff, I'm making project members use the EDGE transaction type. Before this change, adding an existing project member would generate a transaction with no changes. Now, it is correctly detected as a no-op, while normal transactions continue to work properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4379
Differential Revision: https://secure.phabricator.com/D8166
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
Summary: I've only seen this be an issue with PhabricatorBot.
Test Plan: Comment on something with characters that are automatically converted like "<>", and see what the irc bot reports
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D8140
Summary:
Ref T4175.
- Add a configurable name for the clone-as directory, so you can have "Bits & Pieces" clone as "bits~n~pieces/" or simliar.
- By default, use "reasonable" heruistics to choose such a name.
- Generate a copy/pasteable clone commmand with this directory name.
Test Plan: Looked at some repositories.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8097
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.
Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7966
Summary: Ref T4266. This implements rules similar to the old rules. With D7842, maybe this is reasonable? I think it's not like grotesquely bad, at least.
Test Plan: See screenshot.
Reviewers: chad, wrotte
Reviewed By: chad
CC: aran
Maniphest Tasks: T4266
Differential Revision: https://secure.phabricator.com/D7843
Summary: We were getting a weird double box here, missed it my first pass
Test Plan: Review logged in Maniphest and Paste, as well as logged out versions. Test Login flow.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7716
Summary: Simplifies the code a bit and fixes all the wonky previews. Fixes T4053
Test Plan: Test all pages, logged in and logged out.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4053
Differential Revision: https://secure.phabricator.com/D7622
Summary:
Ref T4122. Implements a credential management application for the uses described in T4122.
@chad, this needs an icon, HA HA HAHA HA BWW HA HA HA
bwahaha
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7608
Summary:
Fixes T4060. The logic here is:
- When you take several actions at once, we show a single feed story for all of them.
- We choose the "most interesting" title for the feed story. For example, "close task" is more interesting than "add CC".
Currently, the issue with this is:
- "Add comment" is the //least interesting// title. I think this is correct: all other actions are more interesting than the fact that you added a comment.
- We try to conserve the number of objects we need to load by rendering only the most interesting transaction.
To fix this:
- Stop being so conservative; load all of the transactions and all of their PHIDs.
- Add bodies from any transactions which render bodies. In all cases (I think?) this is a maximum of one comment adding a body.
The end result is a story like this:
epriestley closed T123: the building is on fire.
"Okay guys I put the fire out"
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran, asherkin
Maniphest Tasks: T4060
Differential Revision: https://secure.phabricator.com/D7504
Summary: Ref T4010. Adds a history page and restores the transaction title strings, which previously sort-of existed in the defunct feed story class.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7371
Summary:
Ref T4010. Projects have a weird proto-version of ApplicationTransactions which is very similar but not quite the same.
Move the storage to a modern format, but keep all the other code for now.
Test Plan: Migrated project transactions; edited projects.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7370
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
Summary:
This is primarily a client request, and a little bit use-case specific, but policies seem to be holding up well and I'm getting more comfortable about maintaining this. Much if it can run through ApplicationTransactions.
Allow the ability to edit status, policies, priorities, assignees and projects of a task to be restricted to some subset of users. Also allow bulk edit to be locked. This affects the editor itself and the edit, view and list interfaces.
Test Plan: As a restricted user, created, edited and commented on tasks. Tried to drag them around.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7357
Summary:
We have this commented-out chunk of code now which was originally buggy and is now just nonfunctional.
For now, the core edit types don't always require CAN_EDIT (e.g., subscribe, comment, add edges), except for editing the edit policy itself, which always does. Add a supplemental capability check there and let everything else go through with CAN_VIEW. We can buff the policy checks on application editors over time, they all require appropriate capabilities to get to in the first place anyway.
Test Plan: Created and edited some tasks without getting overwhelmed with policy exceptions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7351
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. When a user comments on an object with an embedded file, write an "attached" edge.
Test Plan: Made a comment on a task with an embedded file, verified the edge was written in Files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7191
Summary:
Currently, if you attach a revision to a task and the revision has a title with quotes or angle brackets in it, they are over-escaped in the email.
Instead, don't do that.
Test Plan: Attached `"QUOTES" MATH: 1 < 2` to a task, got a reasonable looking email.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7186
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
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
Summary: right now you get sent an email with a broken link 'cuz the email is plain text if you edit something with the edit policy being a project.
Test Plan: edited a legalpad document edit policy repeatedly to various projects. observed good emails via bin/mail debug tool. object page still looked good too
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7149
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
Summary:
We currently try to send Maniphest email "To" the owner and actor, but for unassigned tasks there is no owner.
Just filter the PHIDs in the parent, since it's reasonable for subclasses to be liberal about construction here.
Test Plan: Commented on an unassigned task, got an email without a bogus "To".
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7129
Summary:
Ref T603. Disable things the user can't use, allow logged-out users to get a reasonable version of the page.
Also allow logged-out users to view edit history of comments if they're able to see the object.
Test Plan: Viewed Maniphest detail as a logged-out user, got a largely sensible page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7124
Summary:
Ref T2217. Use `getLinkName()` instead of `getName()` so that we get, e.g.,
alincoln attached a revision: D123 Chop some logs
...instead of:
alincoln attached a revision: D123
Test Plan: Attached stuff, looked at the email, saw full object name.
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7127
Summary: Ref T2217. Render feed stories like "alincoln updated T123" instead of "alincoln updated this task.". Fix up some more translations.
Test Plan: Looked at feed, saw something a bit more reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7108
Summary: Ref T2217. Pro is the new standard.
Test Plan: Lots of `grep`, made a pile of Maniphest views/edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7093
Summary: Ref T2217. This is essentially the last writer, should be able to start deleting code now.
Test Plan: Used "Edit Task" to make a bunch of task edits.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7090
Summary: Ref T2217. Nothing too surprising here. This transaction type is weird and should be replaced with the mainstream EDGE type at some point after things clear up more.
Test Plan: Attached and detached revisions and mocks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7085
Summary:
Ref T2217. Ship "Merge in Duplicates" through the new editor. The only notable thing here is `setContinueOnMissingFields()`.
The problem this solves is that if you add a custom field and mark it as required, all existing tasks are "invalid" since they don't have a value, and trying to edit them will raise an error like "Some Custom Field is required!". This is fine for normal edits via the UI, since the user can just select/provide a value, but surgical edits to specific fields should just ignore these errors. Add the ability to ignore these errors and use it on all the field-speific editors.
Test Plan: Merged duplicates, including "invalid" duplicates with missing fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7084
Summary:
Ref T2217. Swaps batch edits to modern editor.
Also, fix some issues with required fields and viewers being required to render certain standard fields (notably, date).
Test Plan: Made various batch edits, verified they went through properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7083
Summary: Ref T2217. All the reads route through new code already, start swapping writes over. This is the simplest writer, used when the user drag-and-drops stuff on the task list.
Test Plan: Dragged and dropped stuff across priorities. Got a transaction and some email. Verified the email and transaction looked OK, threaded properly, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7080
Summary: Ref T2217. Get rid of this rendering pathway's internals and move them to the modern stuff.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7072
Summary: Ref T2217. Route transaction rendering through modern code. This just affects the detail page. Some rough edges but nothing significant.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7070
Summary:
Ref T418. This is fairly messy, but basically:
- Add a validation phase to TransactionEditor.
- Add a validation phase to CustomField.
- Bring it to StandardField.
- Add validation logic for the int field.
- Provide support in related classes.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418
Differential Revision: https://secure.phabricator.com/D7028
Summary: Also some random cleanup now and again. Note reply handler stuff is kind of bojangles bad right now. It didn't work before though either so hey.
Test Plan: asked questions, answered questions, edited answers... the feed pleased my eye
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3653
Differential Revision: https://secure.phabricator.com/D7027
Summary:
Ref T2625. Ref T3794. Ref T418. Ref T1703.
This is a more general version of D5278. It expands CustomField support to include real integration with ApplicationSearch.
Broadly, custom fields may elect to:
- build indicies when objects are updated;
- populate ApplicationSearch forms with new controls;
- read inputs entered into those controls out of the request; and
- apply constraints to search queries.
Some utility/helper stuff is provided to make this easier. This part could be cleaner, but seems reasonable for a first cut. In particular, the Query and SearchEngine must manually call all the hooks right now instead of everything happening magically. I think that's fine for the moment; they're pretty easy to get right.
Test Plan:
I added a new searchable "Company" field to People:
{F58229}
This also cleaned up the disable/reorder view a little bit:
{F58230}
As it did before, this field appears on the edit screen:
{F58231}
However, because it has `search`, it also appears on the search screen:
{F58232}
When queried, it returns the expected results:
{F58233}
And the actually good bit of all this is that the query can take advantage of indexes:
mysql> explain SELECT * FROM `user` user JOIN `user_customfieldstringindex` `appsearch_0` ON `appsearch_0`.objectPHID = user.phid AND `appsearch_0`.indexKey = 'mk3Ndy476ge6' AND `appsearch_0`.indexValue IN ('phacility') ORDER BY user.id DESC LIMIT 101;
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| 1 | SIMPLE | appsearch_0 | ref | key_join,key_find | key_find | 232 | const,const | 1 | Using where; Using temporary; Using filesort |
| 1 | SIMPLE | user | eq_ref | phid | phid | 194 | phabricator2_user.appsearch_0.objectPHID | 1 | |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
2 rows in set (0.00 sec)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T1703, T2625, T3794
Differential Revision: https://secure.phabricator.com/D6992
Summary: Deploy on paste and macro for create stories, 'cuz those are boring emails. Fixes T3808.
Test Plan: made a paste and a macro. commented on 'em. verified i got mail on comments only.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3808
Differential Revision: https://secure.phabricator.com/D6988
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
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
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
Summary: Fixes T2213
Test Plan: Updated a pholio mock description. Observed that when I first showed details there was a round trip made. Toggled show / hide noting no more trips made to server.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2213
Differential Revision: https://secure.phabricator.com/D6801
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
Summary: Ref T3663. There's no data recorded in this table yet, but add the UI and controller for it. Edits and such will eventually go here.
Test Plan: Clicked "View History" on a project, got an empty but non-broken page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3663
Differential Revision: https://secure.phabricator.com/D6785
Summary:
This diff accomplishes this task by adding an arbitrary metadata store to PhabricatorObjectHandle. This seemed like it would be "necessary eventually"; for example if / when we decide we want to show images in these stories we'd need to add some more arbitrary data. A point of debate is this technique will yield the _current_ data and not the data at the time the transaction was originally made. I can see this being both desirable and non-desirable.
Otherwise, the best way to do this is to make a new transaction type specifically for create and store exactly what data we think we would need.
(and there's probably many other ways but they require much more work...)
Test Plan: viewed some pholio create stories and yes, they had the description showing.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3685
Differential Revision: https://secure.phabricator.com/D6767
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
Summary: Currently, we check that the user can view and edit their own transaction, which is always true. Instead, check that they can view the object. I'll fix this with a more tailored check against the EDIT capability that's per-transaction later.
Test Plan: Applying no transactions no longer fatals with undefined `$xaction`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6754
Summary:
Ref T1703. Ref T3718. The `PhabricatorCustomFieldList` seems like a pretty good idea. Move more code into it to make it harder to get wrong.
Also the sequencing on old/new values for these transactions was a bit off; fix that up.
Test Plan: Edited standard and custom profile fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1703, T3718
Differential Revision: https://secure.phabricator.com/D6751
Summary: Ref T2852. Token given stories currently try to `strip_tags()` a `PHUIFeedView` or similar, which doesn't work. Cast it to a string before stripping. This is super gross but I don't want to clean it up until after ApplicationTransactions so we can really clean up all of Feed.
Test Plan: Ran `bin/feed republish <id>` on a feed story about giving a token to a revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6733
Summary: This puts back the 'one line' story we previously had with the updated design.
Test Plan: Review my feed.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6666
Summary: Ref T3373. Most edits aren't too interesting, put them on a separate history page.
Test Plan: Viewed question page; viewed history page for question and answer.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3373
Differential Revision: https://secure.phabricator.com/D6612
Summary:
Ref T3373. The submit listener doesn't properly scope the form it listens to right now, so several forms on the page mean that comments post to one of them more or less at random.
Scope it properly by telling it which object PHID it is associated with.
Test Plan: Made Question comments, saw comments Ajax in on the question itself rather than on an arbitrary answer.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3373
Differential Revision: https://secure.phabricator.com/D6611
Summary:
Ref T3373. This is still pretty messy:
- The JS bugs out a bit with multiple primary object PHIDs on a single page. I'll fix this in a followup.
- The comment form itself is enormous, I'll restore some show/hide stuff in a followup.
Test Plan: Added answer comments in Ponder.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3373
Differential Revision: https://secure.phabricator.com/D6608
Summary: Fixes T3621. We had a fatal from a non-existent function. Also changes form to full width.
Test Plan: edited and it worked! looked better too!
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T3621
Differential Revision: https://secure.phabricator.com/D6571
Summary: Fixes T2654.
Test Plan: attached lots of mocks and tasks to one another from both maniphest and pholio. verified transactions rendered okay in both applications
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2654
Differential Revision: https://secure.phabricator.com/D6501
Summary:
Nice title. We add three new transactions - IMAGE_FILE, IMAGE_NAME, and IMAGE_DESCRIPTION. The first is a bit like subscribers as it is a list of file phids. The latter have values of the form ($file_phid => $data), where $data is $name or $description respectively. This is because we need to collate transactions based on $file_phid...
Overall, this uses the _underyling files_ and not the "PholioImage" to determine if things are unique or not. That said, simply mark PholioImages as obsolete so inline comments about no-longer applicable PholioImages don't break.
Does a reasonable job implementing the mock. Note you can't "update" an image at this time, though you can delete and add at will.
Test Plan: played with pholio a ton.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3489
Differential Revision: https://secure.phabricator.com/D6441
Summary:
Ref T2852. Asana has one bug which I'm having a little trouble figuring out. I want to get more information to debug it, but I'll need them to run `bin/feed republish <story_id>` to get that data.
Right now, it's incredibly hard to figure out the story ID for feed stories. So mostly this is to make that easier (click permalink; pull it out of the URL), but it also adds a little functionality and cleans the code up a bit.
The page itself could be prettier and maybe some day we'll add comments or whatever, but it seems reasonably functionalish.
Test Plan:
{F49962}
- Also loaded many pages of feed history to check that nothing broke.
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6440
Summary: ...also make it so in Pholio when you add an inline comment the preview refreshes. Fixes T2649.
Test Plan: played around in pholio leaving commentary. noted that a new inline comment would refresh the preview.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2649
Differential Revision: https://secure.phabricator.com/D6267
Summary: in applyExternalEffects, for subscriber transactions, we now re-load subscribers. also fixes a bug where a user can get emailed 2x when they take an action on a mock they created.
Test Plan: made some mocks. verified one copy sent to creator and one to each subscriber. (note having problems with email so I verified the phids mail was supposed to be sent to and did not get the actual email delivered)
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3315
Differential Revision: https://secure.phabricator.com/D6206
Summary:
Ref T1536. None of this code is reachable.
For the new web UI for auth edits, give providers more and better customization options for handling the form. Allow them to format transactions.
Also fix the "Auth" application icon.
Test Plan: {F46718}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6221
Summary:
Adds a profile edit controller (with just one field and on links to it) that uses ApplicationTransactions and CustomField.
{F45617}
My plan is to move the other profile fields to this interface and get rid of Settings -> Profile. Basically, these will be "settings":
- Sex
- Language
- Timezone
These will be "profile":
- Real Name
- Title
- Blurb
- Profile Image (but I'm going to put this on a separate UI)
- Other custom fields
Test Plan: Edited my realname using the new interface.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6152
Summary:
None of this code is reachable yet. See discussion in D6147. Ref T1703.
Provide tighter integration between ApplicationTransactions and CustomField. Basically, I'm just trying to get all the shared stuff into the base implementation.
Test Plan: Code not reachable.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6149
Summary:
Fixes T3139. See that task for discussion.
If all mentions are removed because they're already subscribed, we currently generate an empty transaction, which later gets picked up as having no effect and the user gets yelled at.
Instead, don't generate a transaction if no PHIDs remain after filtering already-subscribed PHIDs.
Test Plan: Followed plan in T3139.
Reviewers: garoevans, btrahan
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T3139
Differential Revision: https://secure.phabricator.com/D6048
Summary: Ref T3184. See discussion in D6042.
Test Plan: {F44265}
Reviewers: garoevans, btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3184
Differential Revision: https://secure.phabricator.com/D6047
Summary:
Fixes T3184
We make sure that we're not working with a global policy that just sets the title correctly. Otherwise we have a PHID to work with and set the handle as required.
Test Plan: Change policies on a mock and see the title render correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3184
Differential Revision: https://secure.phabricator.com/D6042
Summary:
Kind of just guessed what to do here. It seemed reasonable.
Take the transaction, use the author to get an actor image. Get the main object handle type, use that for the app icon. Use the transaction's created date as a time.
Ref T2988
@chad I think there's still no icon for mock?
Test Plan: Look at my feed. See some info.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: chad, aran, epriestley, Korvin
Maniphest Tasks: T2988
Differential Revision: https://secure.phabricator.com/D6044
Summary: I introduced this helper at some point, clean up all the code duplication around content sources.
Test Plan: Grepped; hit edit interfaces for most/all of these.
Reviewers: btrahan, chad, edward
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6030
Summary: this is D5750 but just the conpherence part. fixes a few random conpherence bugs / quirks as well. Also messes with ApplicationTransactionEditor to expose the xactions so Conpherence doesn't over-update participation rows. Fixes T2429.
Test Plan: set LIMIT to 3. verified I could scroll down all conpherences. next, picked a conpherence "in the middle" to load. verified I could page up and down. next, picked a conpherence in the middle then had another user update that conpherence. verified as I paged up the conpherence re-loaded properly selected
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin, vrana
Maniphest Tasks: T2429
Differential Revision: https://secure.phabricator.com/D5783
Summary: This moves Feed's rendering over to a PHUI class. I want to build it out and have it power Ponder, Phame, Feed, as well as Profiles and Projects in some fashion. It also provides some more data depth over ObjectItemView. Also updated Profile for mobile and fixed some other display issues there.
Test Plan: Tested Feed, Profile. Used iOS and Chrome.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5687
Summary: Refreshes feed's design a bit, adds app icons, works nicer on mobile.
Test Plan: Tested many feed stories, not sure I got them all, but seems fine.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5673
Summary:
Let applicationtransaction editors request locks when applying certain transactions. See D5567.
We should probably do this for all not-guaranteed-safe operations, but let's put our toe in the water with Conpherence first. I believe the cost of these locks is very small and my cautious stance toward acquiring them isn't really warranted, but I've also never seen, e.g., a race on a title edit.
Test Plan: Added comments with and without locks, verified both pathways worked correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5582
Summary:
Apparently I am crazy and didn't test D5537 propertly at all. In particular:
- Currently, the update sends back new "people" and "files" widgets. The "people" widget has a tokenizer, which fatals when the behavior initializes without the widget in the DOM. For now, disable widget updates on replies. I'll fix this in a future diff.
- Currently, we don't update the "last_transaction_id" in the form itself, so the first reply sends back 1 message, the next 2 messages, etc. Update the input.
- The transaction paging doesn't and has never worked, I am crazy. Make it actually work.
Test Plan:
computers are too hard
(also, this is why I hate Javascript)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5538
Summary: As we've moved more remarkup logic post-cache (e.g., to implement policy-aware rules), we're paying more of a price for it. Improve performance in Phabricator by sharing engine instances where possible.
Test Plan: With next diff, this drops 75% of the cost of markup handling in Conpherence.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5510
Summary:
Ref T2787. A product is the abstract representation of something you can buy or rent/subscribe to. Although the interface isn't locked down yet, this would ultimately be internal/administrative.
Products likely have some user-facing skin on top of them: plans would have a purchasing/comparison flow, physical goods would have a storefront, etc., so products don't have any information like descriptions or images, just the data that Phortune needs to correctly bill accounts.
Generally, this is very basic for the moment.
Test Plan:
{F37594}
{F37595}
{F37596}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5441
Summary:
- Grouping inline comments made us render `array(null)` for the content of a single comment. Detect that this corresponds to empty text.
- Don't try to mention-subscribe automatic subscribers, like the author.
Test Plan: Tried to mention-subscribe the author, saw no "x subscribed y" transaction generate. Added a real subscriber, saw no empty content render.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5315
Summary:
Fixes T2639 by grouping related transactions at display time, so all the inlines merge into a nice block.
(Note that this does not do anything about T2709 yet, so there's still no way to figure out where the inlines actually are.)
Test Plan: {F35262}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2639
Differential Revision: https://secure.phabricator.com/D5313
Summary: After D5305, this method does nothing since we automatically figure out what we need to do.
Test Plan:
- Viewed a page with the main menu on it (MainMenuView).
- Viewed a revision with transactions on it (TransactionView).
- Viewed timeline UIExample (TimelineView, TimelineEventView).
- Viewed a revision (PropertyListView).
- Viewed a profile (ProfileHeaderView).
- Viewed Pholio list (PinboardView, PinboardItemView).
- Viewed Config (ObjectItemView, ObjectItemListView).
- Viewed Home (MenuView).
- Viewed a revision (HeaderView, CrumbsView, ActionListView).
- Viewed a revision with an inline comment (anchorview).
- Viewed a Phriction diff page (AphrontCrumbsView).
- Filed T2721 to get rid of this.
- Looked at Pholio and made inlines and comments (mockimages, pholioinlinecomment/save/edit).
- Looked at conpherences.
- Browsed around.
Reviewers: chad, vrana
Reviewed By: chad
CC: edward, aran
Differential Revision: https://secure.phabricator.com/D5307
Summary: okay title. other apps can get this by implementing shouldAllowPublic and set(ting)RequestURI on TransactionsCommentView. note i put some css inline -- let me know if that belongs someplace else or needs better design.
Test Plan: viewed a mock logged out and saw new button. used new button and ended up on the mock logged in with a clean URI.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2653
Differential Revision: https://secure.phabricator.com/D5266
Summary: It's dumb to execute a query which we know will return an empty result.
Test Plan: Looked at comment preview with "11", didn't see "1 = 0" in DarkConsole.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5177
Summary: Fixes T2659. These didn't exist until recently.
Test Plan: {F34556}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2659
Differential Revision: https://secure.phabricator.com/D5221
Summary:
Provide a viewer to all remarkup engines.
This fixes commit summaries in Diffusion, which were failing to link because they didn't have a user and thus couldn't see/load `D123`, e.g.
Test Plan: Grepped for engine creation.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5152
Summary:
I missed this in review of D5155: `wordwrap()` returns a string, but `phutil_utf8_hard_wrap()` returns an array.
Implode the returned arrays so the stuff underneath it doesn't choke.
Test Plan: Clicked "show details" on a maniphest task description change
Reviewers: AnhNhan, kwadwon
Reviewed By: kwadwon
CC: aran
Differential Revision: https://secure.phabricator.com/D5195
Summary: using to phutil_utf8_hard_wrap instead of wordwrap
Test Plan: hard_wrap already unit tested
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5155
Summary: Fixes T2596. We currently omit this, so we don't get some styling (lists, e.g.)
Test Plan: Viewed a remarkup list in Macro, Pholio.
Reviewers: chad, vrana
Reviewed By: chad
CC: aran
Maniphest Tasks: T2596
Differential Revision: https://secure.phabricator.com/D5114
Summary:
Fixes T2587. Specifically:
- Don't try to implicitly subscribe the actor if they're already subscribed.
- Since there are like 5 things that need to interact with subscribers, just load them once upfront for Subscribable objects.
Test Plan: Made a comment on a mock I was CC'd on without an error.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2587
Differential Revision: https://secure.phabricator.com/D5091
Summary: When you make a comment on an object (or take certain other actions), we want to automatically CC you. Build this into ApplicationTransactions since it's a common behavior shared across multiple apps. Fixes T2215.
Test Plan: Made a comment on a macro, got cc'd.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2215
Differential Revision: https://secure.phabricator.com/D5019
Summary:
- Remove some redundant copies of translations after D4985.
- Make some %d more grammatical, "run this command" reads better than "run this 1 command". In context, these numbers are always very small, so counting them even in the >1 variants aren't useful.
- Fix subscriber(s).
Test Plan: Looked at an unsubscribe story, saw translation.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4989
Summary: Fixes T2214. For objects which support ApplicationTransaction, use ApplicationTransactions to apply subscription action changes. Principally, this makes clicking "Subscribe" / "Unsubscribe" appear correctly in the transaction log.
Test Plan: Clicked "Subscribe" and "Unsubscribe" a on Macros and Mocks.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2214
Differential Revision: https://secure.phabricator.com/D4986
Summary:
Some transactions (like editing configuration values, task descriptions, or Conpherence images) can't be simply explained and need an additional larger element to show them fully (like a text diff).
Support change details like this in ApplicationTransactions. Implements the element in Config, so you can see changes.
Test Plan: {F32974}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2213
Differential Revision: https://secure.phabricator.com/D4984
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
Summary: They are same because render() returns safe HTML and raw strings are automatically escaped.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4909
Summary:
This resolves lots of double escaping.
We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped.
Also `pht()` auto escapes if it gets `PhutilSafeHTML`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4889
Summary:
Done by searching for `AphrontDialogView` and then `appendChild()`.
Also added some `pht()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4882
Summary: Drafts are saved as inline comments for images when user comments mock.
Test Plan: Verified that drafts receive transactionphid when user comments mock.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D4850
Summary: this broke when I moved sorting to the editor. Turns out you can't have custom hooks for the comment transaction, and thus I couldn't update participation inside the editor. This fixes this by removing the empty switch statement for the transaction type inside the parent class editor. This should have no effect other than fixing Conpherence. Note that conpherences will need another message, etc for a given conphernece to fix itself.
Test Plan: Conpherence threads were updated properly!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4817
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 • 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
Summary: Created Applications application which allows uninstallation & installation of application.
Test Plan: In "Applications" application, clicked on uninstalled the application by cliking Uninstall and chekcing whether they are really uninstalled(Disabling URI & in appearance in the side pane). Then Clicked on the install button of the uninstalled application to check whether they are installed.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4715
Summary: Fixes various array_combine() warnings for PHP < 5.4
Test Plan: lint/unit/grep
Reviewers: btrahan, vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4660
Summary: Added a reply handler. A few problems -- first, I can't seem to get this to actually send me email so I haven't been able to reply (which I would have done by generating a reply, then copying the raw email into scripts/mail_handler.php). Second, the subject is often terrible on these emails -- unless the conpherence is named its something gross like "E4:" Third, on create I am noticing an error on array_combine() which I think is related to the need to write array_combine_not_broken or what have you I saw go by... (PhabricatorTransactionEditor does array_combine(xaction->getOldValue(), xaction->getOldValue()) and complains that the arrays are empty)
Test Plan: noted that /mail/ said mails were being sent
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4656
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
Summary: it's ugly. but it works. basically. See T2399 for a roughly prioritized list of what still needs to happen.
Test Plan:
- created a conpherence with myself from my profile
- created a conpherence with myself from "new conpherence"
- created a conphernece with another from "new conpherence"
- created a conpherence with several others
- created a conpherence with files in the initial post
- verified files via comment text ("{F232} is awesome!") and via traditional attach
- edited a conpherence image
- verified it showed up in the header and in the conpherence menu on the left
- edited a conpherence title
- verified it showed up in the header and in the conpherence menu on the right
- verified each widget showed up when clicked and displayed the proper data
- calendar being an exception since it sucks so hard right now.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, epriestley, chad, codeblock, Korvin
Maniphest Tasks: T2301
Differential Revision: https://secure.phabricator.com/D4620
Summary:
When configuration is set incorrectly (e.g., of the wrong type), detect and repair it by setting it to the default value. A setup warning will be raised separately.
Notably, this removes the need to hard-code all the class types.
This runs separately from the "invalid config" check because we need to run it on every page, but do setup checks only once per restart (some of them are slow).
Also dirty setup when we edit configuration.
Test Plan: Set config incorrectly on purpose, saw Phabricator correct it on restart and on every subsequent page load until it was fixed.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2292
Differential Revision: https://secure.phabricator.com/D4492
Summary: Use ApplicationTransactions in Config to create an edit history. Resolves T2256.
Test Plan: {F28477}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2256
Differential Revision: https://secure.phabricator.com/D4314
Summary:
When previewing, save drafts. When loading objects, restore drafts if they are available.
Depends on: D665
Test Plan:
- Viewed a Mock.
- Typed text into the comment box.
- Reloaded the page.
- Text still there.
- Hit submit, got my comment.
- Reloaded the page.
- Draft correctly deleted.
- Repeated for Macros.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4252
Summary:
Implements previews for Macros and Pholio.
(Design is nonfinal -- kind of split the difference between `diff_full_view.png`, laziness, and space concerns. Next couple diffs will add more stuff here.)
Test Plan: {F28055}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4246
Summary:
When a user submits an action with no effect (like an empty comment, an "abandon" on an already-accepted revision, or a "close, resolved" on a closed task) we want to alert them that their action isn't effective. These warnings fall into two general buckets:
- User is submitting two or more actions, and some aren't effective but some are. Prompt them to apply the effective actions only.
- A special case of this is where the only effective action is a comment. We provide tailored text ("Post Comment") in this case.
- User is submitting one action, which isn't effective. Tell them they're out of luck.
- A special case of this is an empty comment. We provide tailored text in this case.
By default, the transaction editor throws when transactions have no effect. The caller can then deal with this, or use `PhabricatorApplicationTransactionNoEffectResponse` to provide a standard dialog that gives the user information as above. For cases where we expect transactions to have no effect (notably, "edit" forms) we just continue on no-effect unconditionally.
Also fix an issue where new, combined or filtered transactions would not be represented properly in the Ajax response (i.e., return final transactions from `applyTransactions()`), and translate some strings.
Test Plan:
- Submitted empty and nonempy comments in Macro and Pholio.
- Submitted comments with new and existing "@mentions".
- Submitted edits in both applications.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T912, T2104
Differential Revision: https://secure.phabricator.com/D4160
Summary: Currently, AphrontProxyResponse is expected to build a string. This prevents some response types (like Dialog) from being proxied, because they have special rules. Instead, make proxy responses reduce into a non-proxied response so it's possible to proxy any type of response and hit all the normal rules for it.
Test Plan: Built a proxied DialogResponse on top of this.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104, T912
Differential Revision: https://secure.phabricator.com/D4159
Summary:
When possible, render application transactions via Ajax. Instead of reloading the page when the response returns, append new transactions to the transaction view.
Scroll the window to the new transactions, animate them in, and clear the form to make this interaction feel reasonable.
When editing transactions, fade them in but do not scroll to them (i.e., don't disrupt the user's position).
Test Plan: Edited and appended transactions via Ajax. Observed fade in animations and scroll behavior. Clicked anchors to verify proper anchor accounting.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4151
Summary: When possible, replace the edited or deleted transaction inline using Ajax.
Test Plan: Repeatedly edited and deleted transactions. Clicked their anchors to verify anchors were correctly preserved.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4150
Summary:
Allows you to edit or delete comments in appplications which support ApplicationTransactions.
UI/UX stuff:
- The dialogs are rough but I want to do a dialog design pass more generally, @chad has some mocks.
- When you add new mentions via edit, they don't currently count as mentions. I'm not sure what I want to do about this.
- When you edit or delete a comment, we do not publish any notifications about it. I think this is reasonable.
- I didn't separate "delete" out versus "edit"; I assume it will be reasonably intuitive that deleting all the text deletes effectively deletes the comment. I also want to discourage deletion somewhat (we still show the transaction, just show that the comment has been deleted).
Test Plan:
Transaction view, note "Edit" and "Edited" links:
{F26914}
Edit view, has some design issues but I want to do a pass on dialogs in general:
{F26915}
History view:
{F26913}
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4149
Summary: Publish feed stories, including from Pholio. Actual stories are somewhat garbage but it's all display-time; I'm going to do a pass on feed in general.
Test Plan: {F26832}
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4140
Summary:
- Adds mail support to the generic transaction construct.
- Restores mail support to Pholio (now much improved; the mails are actually useful).
Test Plan: Updated a Pholio mock, got mail.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4139
Summary:
Split Pholio's transaction implementation into generic and application-specific parts. Moves us toward generic transactions, with support for:
- Editing and deleting comments.
- Setting visibility of individual comments (I'm not a fan of this feature but we'll see).
I want to move everything to a more generic piece of infrastructure but there's very little they can share right now so adding transactions to, e.g., Paste or Macros (T2157) means massive amounts of similar code.
Tons of work left to do here, but I think it basically works. Here's a screenshot:
{F26820}
Test Plan: Made transactions in Pholio.
Reviewers: btrahan, vrana, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4136
