Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary:
Ref T7585. This implements everything specified, with a few caveats
- since rooms you have yet to join can't be viewed in the column yet, the column view has some bugs and isn't expected to work.
- the room you're looking at is just pre-pending to the top of the "recent" list
Test Plan: made a room that no one could join. verified when viewing that there was no comment ui. made a room that others could join. verified folks who had yet to join had a "join" button with an area for text. tried joining with / without message text and it worked in both cases
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7585
Differential Revision: https://secure.phabricator.com/D12149
Summary:
Ref T1460. Ref T6403. Replace `Diffusion::INLINEDONE` with `Transactions::INLINESTATE` and generalize things enough that we can lift it into core.
The next change will lift Differential's similar implementation into the core.
Also start implementing a fix for T6403, providing an alternate hook for optional builtin transactions.
Test Plan: Changed inline state in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6403, T1460
Differential Revision: https://secure.phabricator.com/D12129
Summary:
Ref T2009. Ref T1460. The way Diffusion and Differential load inlines is horrible garbage right now:
- Differential does an ad-hoc query to get the PHIDs, then does a real load to policy check.
- Diffusion completely fakes things. In practice this is not a policy violation, but it's dangerous.
Make TransactionCommentQuery extensible so we can subclass it and get the query building correctly in the right Query layer.
Specifically, the Diffusion and Differential subclasses of this Query will add appropriate `withX()` methods to let us express the query in SQL.
Test Plan: Loaded, previewed, edited, and submitted inlines in Differential and Diffusion
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009, T1460
Differential Revision: https://secure.phabricator.com/D12026
Summary: This ended up having a different signature; the discrepancy can cause a warning.
Test Plan: No more warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11971
Summary: The participant list can sometimes be `null`, which fails when we try to `array_fuse()` it.
Test Plan: Created a new thread cleanly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11969
Summary:
Ref T6516. Although this behavior is somewhat-arguable as desirable, I think it's less surprising and more consistent to disable mentions when a repository is publishing.
In particular, if you import a repository developed on another Phabricator install, this stops all the `T123` in commit messages from creating mentions on your unrelated `T123` tasks.
We already disable autoclose, so `Closes T123` and `Ref T123` already have no effect, but a bare `T123` would generate a mention. Likewise, `@epriestley` would generate a mention.
If you import such a repository and then update it periodically, updates will activate autoclose and publishing (if you didn't disable them), but presumably this will hit a couple of tasks and you'll go change the settings if you forgot.
At some point, we may have some kind of use case for separating the "publish" setting into a "publish" setting and a "this is a local repository" setting. For example, if you work at Widget Corp, want to import Phabricator locally, //and// want to write Herald rules against it, you can't currently configure the repository to let you do all of this. But we haven't actually seen a use case for this yet.
Test Plan:
- Pushed some commits with bare `T11`, saw mentions.
- Disabled publishing for the repository, pushed some commits with
- Imported a bunch of commits without seeing pipeline failures.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6516
Differential Revision: https://secure.phabricator.com/D11966
Summary: Fixes T7232.
Test Plan: Saw "View Raw" gone for removed comment, still present and functional for non-removed comment.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7232
Differential Revision: https://secure.phabricator.com/D11926
Summary: Ref T7094. I am not sure when this text is legitimately exposed to users - they should be getting an error about not being able to see the object before they get an error about not being able to see a given transaction... That said, I think this text is logically correct at least.
Test Plan: read the text
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11632
Summary:
Ref T7094. We should do a policy query on the files IMO because there exists a scenario where the file gets locked down directly. This requires being a bit more disciplined about setting user, which in turn requires deciding whether or not to show edit / reply links as a separate piece of logic, not conditional on user presence.
This is not the best code but I don't think it gets worse with this and is just some other nuance in any larger cleanup we take on someday.
Test Plan: looked at a revision and noted inline comments rendered correctly with reply / edit actions. looked at a diff standalone and noted no reply / edit actions as expected. looked at a "details" link on a transaction and it rendered correctly. looked at a diff in phriction of page edits and it looked good. grepped around and verified the remaining callsite in diffusion already has the setUser call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11579
Summary: Fixes T6819. This isn't as useful as you might think and has one horribly buggy behavior - if you edit an object which has a description and a projects field, you can be unable to remove the associated project as the automagic association from the description kicks in. Further, since we've added the ability for applications to create multiple email addresses AND herald can react to those emails - say by programmatically adding projects - the known needs for this feature are basically 0. If this proves to be false we can maybe add some other syntax for these mentions - see T6819 for ideas / discussion.
Test Plan: removed a project from a maniphest task while still mentioning it in the description and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6819
Differential Revision: https://secure.phabricator.com/D11573
Summary: due to typehints, passing null is going to barf here. Ref D11564, ref T5039.
Test Plan: made an edit to a task from the web ui and it didnt fatal
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11571
Summary:
Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???"
Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective.
Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11564
Summary: Removes the 1x application icons, and uses the fonticons instead. Feed was only known location.
Test Plan:
feed, dashboards, grep for use
{F275636}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11496
Summary: Fixes T6989. Basically return a nice dialogue like we do for "NoEffect" transactions. This is a little prettier than the other dialogue was. Also, stop adding TYPE_EDGE as a transaction type as we end up having it 2x, which then makes the error get validated 2x.
Test Plan: tried to add myself as a reviewer and got a nice error message.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6989
Differential Revision: https://secure.phabricator.com/D11448
Summary: Ref T6962. Mainly accomplished by re-factoring the base editor `buildMailBody` function and then using it differently in the `DifferentialTransactionEditor`.
Test Plan: commented on a revision leaving inline feedback. inspected via bin/mail and it looked good! also made a maniphest comment and checked that email, which still looked good.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6962
Differential Revision: https://secure.phabricator.com/D11402
Summary: Fixes T6895, When viewing comment edit history, user should not see a dropdown for each comment edit transaction.
Test Plan: Edit task comment, view comment edit history, comment transactions should not provide a dropdown with action items.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6895
Differential Revision: https://secure.phabricator.com/D11355
Summary: Fixes T6594, Logged out users should be able to "View Raw" comments in public objects.
Test Plan: Log out, open maniphest task with comments, open dropdown associated with comment, click "View Raw", should be able to see raw comment.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6594
Differential Revision: https://secure.phabricator.com/D11295
Summary:
Ref T3165. Builds a dedicated index for Conpherence to avoid scale/policy filtering concerns.
- This is pretty one-off but I think it's generally OK.
- There's no UI for it.
- `ConpherenceFulltextQuery` is very low-level. You would need to do another query on the PHIDs it returns to actually show anything to the user.
- The `previousTransactionPHID` is so you can load chat context efficiently. Specifically, if you want to show results like this:
> previous line of context
> **line of chat that matches the query**
> next line of context
...you can read the previous lines out of `previousTransactionPHID` directly, and the next lines by issuing one query with `WHERE previousTransactionPHID IN (...)`.
I'm not 100% sure this is useful, but it seemed like a reasonable thing to provide, since there's no way to query this efficiently otherwise and I figure a lot of chat might make way more sense with a couple of lines of context.
Test Plan:
- Indexed a thread manually (whole thing indexed).
- Indexed a thread by updating it (just the new comment indexed).
- Wrote a hacky test script and got reasonable-looking query results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D11234
Summary: Modernize remaining edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Browsed around and performed various actions include subscribing, unsubscribing and watching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11116
Summary:
Removes an unused PhabricatorFeedStory Parameter from all getTitleForFeed() and getApplicationTransactionTitleForFeed() functions.
Ref D11088 Ref T6545
Test Plan: ran all unit tests and viewed some dashboard feeds
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11146
Summary:
Ref T4411
I'm not quite sure if this is the right place for this as it will be difficult to provide proper user feedback of why we removed a particular subscriber.
Is the ApplicationTransactionEditor generally the right place to extract mentioned phids in comments?
On the other hand in some cases we cannot really give user feedback why a user was not subscribed (e.g.: commits & diffs)
Adding a diff to a repo where the user mentioned has no view permissions the subscriber is currently still added. Still would have to find where this is donet...
Any other places?
Unrelated: Is there any way to remove a subscriber from a commit/audit ?
Test Plan:
- Edited tasks with the mentioned user having view permissions to this specific task and without
- Raised concern with a commit and commented on the audit with the user having view permissions to the repo and without
- Added a commit to a repo with and without the mentioned user having permissions
- Mention a user in a task & commit comment with and without permissions
- Mentioning a user in a diff description & comments with and without permissions to the specific diff
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T4411
Differential Revision: https://secure.phabricator.com/D11049
Summary:
Shows a timeline of all modified settings Fixes T6545
Will show all settings (no pagination, should be not so difficult to add if needed but most installs won't have hundreds of settings changes)
I'm not happy by how the PhabricatorConfigTransaction object is instructed to render the config keys but i don't see any other reasonable way.
We could always show the keys though.
Test Plan: Changed settings and called the history page
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11088
Summary: These classes are named differently from other `PhabricatorEdgeType` subclasses. Rename them for consistency.
Test Plan: I would expect the linter to complain if I missed anything.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11112
Summary:
...except the transaction class itself, which still needs some knowledge of these transactions for older installs.
Ref T5245. T5604 and T5245 are now in a similar place -- there's an unknown set of bugs introduced from my changes and there's still old display code lying around with some old transactions in the database. I'll stomp out the bugs if / when they surface and data migration is up next.
This revision also adds a "TransactionPreviewString" method to the edge objects so that we can have a prettier "Bob edited associated projects." preview of this transaction.
Test Plan: added a project from task detail and saw correct preview throughout process with correct project added. bulk removed a project from some tasks. added a project from the edit details pane.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11013
Summary:
There's a comma to the lower-left of my profile picture here:
{F248962}
This is on a page like https://secure.phabricator.com/F248948
What's happening is that some `render()` method is returning a valid result like `array($stuff, null)`. This is getting passed to JS as an array, which is implicitly `join()`'ing it into a string, adding a comma.
Instead, make sure we render these to strings on the server side before shipping them to the client.
Test Plan: No more comma on file previews.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10974
Summary: Fixes T6731. I don't really understand the intent behind the two view classes here, but to get this to work I need to pass yet more data to the lower-level class.
Test Plan: Viewed a task with many comments. Clicked "show older". Quoted everything I could. Verified for each quote that it quoted correctly, inlcuding linking to the prior transaction.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6731
Differential Revision: https://secure.phabricator.com/D10973
Summary: Fixes T6727. Repro is: mention a task on another task, in a comment.
The inverse edge editor applying the "alincoln mentioned this in <other task>" transaction doesn't have enough data to execute Herald rules.
Just don't try to execute the rules, since they don't make much sesne from a product perspective and are tricky from a technical perspective.
Test Plan: Commented on `T1` with `T2` in comment body and a Herald rule that examines subscribers.
Reviewers: btrahan
NOTE: Cowboy committing this since any task mention fatals.
Summary: Fixes T6694. Ref T4712. Turns out the logic here was slightly incorrect; we don't want to use the id of the last thing we hid but rather the first thing we show. I had garbage test data ("asdsadsadsa", etc) I guess so I didn't notice this.
Test Plan: made a new task where user a and user b alternated 3 comments each, cooperatively numbering them from 1 - 20. as both users, showed older transactions. pre-patch the issue described in T6694 occurred and post patch I saw the entire counting sequence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712, T6694
Differential Revision: https://secure.phabricator.com/D10933
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: Only necessary for edits, only bother if the comment version is greater than 1. Ref T6690. This is another way to fix T6690 -- this check will never run since you can't edit a conpherence comment -- **but** the fix already applied should happen too to future proof Conpherence.
Test Plan: made a comment on a diff - success. edited the comment and mentions were generated.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6690
Differential Revision: https://secure.phabricator.com/D10928
Summary: Fixes T6648. We do some automagical hotness based on the text you enter in remarkup textareas - e.g. adding projects or mentioning other objects. Refine the code here so that even when just editing a comment we build these transactions and apply them.
Test Plan: edited a comment and noted new mentions and projects showed up appropriately...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6648
Differential Revision: https://secure.phabricator.com/D10922
Summary:
Ref T4712. Thus far, it seems that most "non-standard" things can be done pretty easily in the controller. Aside from deploying, this diff had to fix a few bugs / missing implementations of stuff.
(Notably, PhabricatorAuthProviderConfig, HeraldRule, PhabricatorSlowvotePoll, and AlmanacNetwork needed to implement PhabricatorApplicationTransactionInterface, PhabricatorAuthAuthProviderPHIDType had to be added, and a rendering bug in transactions of type PhabricatorOAuth2AuthProvider had to be fixed.)
Test Plan: Almanac - looked at binding, device, network, and service view controllers and verified timeline displayed properly. Herald - looked at a rule and verified timeline. Slowvote - looked at a vote and verified timeline. Auth - looked at an auth provider (Facebook) and verified proper display of transactions within timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10921
Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.
Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10887
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Ref T1217, Add link to email preferences to email template
Test Plan: Add comment to object like Maniphest task, check that email has a footer with a link to email preferences.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T1217
Differential Revision: https://secure.phabricator.com/D10883
Summary: Ref T6343, adding HTMLMailMode to remarkup, and most objects should now be processed and appear pretty in emails.
Test Plan: Add a comment to a Maniphest task containing a mention of an object like '{T1}' or 'T1'. Emails should show a styled version of the object similar to how the object looks in the context of the Maniphest task in the UI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6343, T2617
Differential Revision: https://secure.phabricator.com/D10859
Summary:
Ref T5833. Currently, we have an `AlmanacDeviceProperty`, but it doesn't use CustomFields and is specific to devices. Make this more generic:
- Reuse most of the CustomField infrastructure (so we can eventually get easy support for nice editor UIs, etc).
- Make properties more generic so Services, Bindings and Devices can all have them.
The major difference between this implementation and existing CustomField implementations is that all other implementations are application-authoritative: the application code determines what the available list of fields is.
I want Almanac to be a bit more freeform (basically: you can write whatever properties you want, and we'll put nice UIs on them if we have a nice UI available). For example, we might have some sort of "ServiceTemplate" that says "a database binding should usually have the fields 'writable', 'active', 'credential'", which would do things like offer these as options and put a nice UI on them, but you should also be able to write whatever other properties you want and add services without building a specific service template for them.
This involves a little bit of rule bending, but ends up pretty clean. We can adjust CustomField to accommodate this a bit more gracefully later on if it makes sense.
Test Plan: {F229172}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10777
Summary: Fixes T6427.
Test Plan: Log out of sandbox, navigate to public task, click 'See Details' in a transaction. Get Dialog.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6427
Differential Revision: https://secure.phabricator.com/D10759
Summary: missing a setHandles on this codepath I think...? Fixes T6300.
Test Plan: not actually tested - I just think this is the fix since the other renderX methods all do this setHandles thing and I can't figure out how handles get set otherwise...
Reviewers: epriestley, avivey
Reviewed By: epriestley, avivey
Subscribers: avivey, Korvin, epriestley
Maniphest Tasks: T6300
Differential Revision: https://secure.phabricator.com/D10699
Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit.
Test Plan:
- Published a Differential feed story into Asana with comment text.
- Pulbished a Diffusion feed story into Asana with comment text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10584
Summary:
Ref T1191.
- Adds support for custom fields.
- Adds support for partial indexes (indexes on a prefix of a column).
- Drops old auxiliary storage table: this was moved to custom field storage about a year ago.
- Drops old project table: this was moved to edges about two months ago.
Test Plan:
- Viewed web UI, saw fewer issues.
- Used `grep` to verify no readers/writers for storage or project table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10526
Summary: Ref T1191. This fills in some more features and gets audit and auth nearly generating reasonable expected schemata.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10500
Summary: ...also re-jiggers all the anchor stuff to use $xaction ID. This seemed like the simplest way once I got in the code, as well as having nice properties for if / when we want to re-add some ajax stuff since the ID is a pretty solid piece of data to key off. Fixes T6083.
Test Plan: mentioned DX in private DX+1. Could see on DX the mention as me and not as the other user. For transactions, I left a comment on Paste and it worked, and I edited an existing transaction and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6083
Differential Revision: https://secure.phabricator.com/D10488
Summary: Fixes T6059.
Test Plan: Made a comment on TX mentioning TX and TX+1. TX did not get a "mentioned" transaction while TX+1 did.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6059
Differential Revision: https://secure.phabricator.com/D10464
Summary: Some versions of PHP aren't very happy about both interfaces and superclasses defining a method. Just remove it from MentionableInterface and leave it as implicit.
Auditors: btrahan
Summary: Fixes T4036. Now if you say something on diff X like "This reminds me of Tx and Dy and commitHashFoo and Px." each of those objects gets a little visible transaction that the mention occurred. No feed, email, or notifications.
Test Plan: made a comment like above and verified transactions. also submitted a diff that "Fixes Tx" and Tx did not get the transaction as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T4036
Differential Revision: https://secure.phabricator.com/D10451
Summary: Fixes T6037. We don't currently write the "this file is attached to such-and-such object" edge on comment edits.
Test Plan: Edited a comment, adding `{Fnnn}`. Verified file was not attached before the edit, but was afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6037
Differential Revision: https://secure.phabricator.com/D10423
Summary:
Fixes T4057. This sort of sidesteps the trickiest (but very rare) case of things like embedded slowvotes. We might be able to refine that later.
In the common bad case (macros, large images) it gets reasonable results by using `overflow: hidden` with `max-height`.
We use `PhabriatorMarkupEngine::summarize()` to try to just render the first paragraph.
Test Plan: {F195093}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4057
Differential Revision: https://secure.phabricator.com/D10355
Summary: its not necessary. Fixes T5906
Test Plan: clicked "Login to Comment" and went straight to the login form, sans intermediary dialogue
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5906
Differential Revision: https://secure.phabricator.com/D10295
Summary:
Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Test Plan:
Been running this in our deployment for a few months now.
====Well behaved clients====
- Gmail
- Mail.app
====Bad clients====
- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.
====Need testing====
- Outlook (Windows + Mac)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: webframp, taoqiping, chad, epriestley, Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D9375
Summary: Should help with debugging. In the web UI there is now a link to "View Email". This uses the same debugging output that `./bin/mail show-outbound` uses. Fixes T5768. Code is very defensive as I think these tables may be truncated eventually?
Test Plan: viewed some emails and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10244
Summary: Fixes T4973. For `PhabricatorProjectInterface` objects, add a header to let clients do mail filtering.
Test Plan: Saw `X-Phabricator-Projects: <#goat_farm>` in outbound mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: rush898, epriestley
Maniphest Tasks: T4973
Differential Revision: https://secure.phabricator.com/D10256
Summary:
Ref T5861. Ref T5769. If users don't care at all about something, allow them to ignore it.
We have some higher-volume notifications either built now (column changes) or coming (mentions) which users might reasonably want to ignore completely.
Test Plan:
Ignored some notifications, then took appropraite actions. Saw my user culled from the notification subscriber list.
{F189531}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5769, T5861
Differential Revision: https://secure.phabricator.com/D10240
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.
This has zero impact on the UI or behavior.
Test Plan:
- Checked/unchecked some options, saved form.
- Swapped back to `master` and saw exactly the same values.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10238
Summary:
Fixes T5456. We lost this logic in the transition to applicationtransactions.
When publishing a feed story, mark all of the object's projects as related, so the project filter in feed works.
Test Plan: Made a comment on a task associated with a project, saw the story in filtered feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: timor, epriestley
Maniphest Tasks: T5456
Differential Revision: https://secure.phabricator.com/D10233
Summary:
- Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
- Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
- Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.
Test Plan:
- Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
- Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
- Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5771, T4864, T5851
Differential Revision: https://secure.phabricator.com/D10221
Summary: Sets layout as flush when rendering diff table or timeline in a Dialog
Test Plan: Tested each
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10194
Summary:
Ref T5245. This removes some hacks and activates two meaningful interactions:
- The "projects" field goes through shared code now.
- Mentioning projects in tasks using hashtags now tags them.
Test Plan:
- Viewed a task with projects.
- Viewed a task with no projects.
- Viewed a task with projects and board positions.
- Viewed a revision with projects.
- Made a `#hashtag` comment in Maniphest and got a project association.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D10177
Summary: Use cutlery icon for hilarity. Ref T5768.
Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10183
Summary: Ref T4896. Instead of using custom stuff, use standard stuff.
Test Plan: Viewed a bunch of feed stories and published some over the Asana bridge.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10114
Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:
- No more fake proxy writes;
- no more fake detection of `@mentions`.
For now, the old code still applies most of the effects and handles feed and email.
Test Plan:
- Added comments.
- Added comments with inline comments.
- Added just inline comments.
- Added comments with Conduit.
- Previewed comments.
- Added CCs explicitly and with `@mentions`.
- Added auditors.
- Accepted a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10109
Summary: Ref T4896. Depends on D10055. This uses core rendering stuff for audit comments, and fixes all the wonkiness with inlines so we can actually land the migration.
Test Plan: Viewed, previewed and edited various types of comments in Diffusion.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10056
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Provide an implementation for the `getName` method rather than automagically determining the application name.
Test Plan: Saw reasonable application names in the launcher.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10027
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: Instead of implementing the `getTypeConstant` method in all subclasses of `PhabricatorPHIDType`, provide a `final` implementation in the base class which uses reflection. See D9837 for a similar implementation.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9985
Summary: Ref T5655. The `PhabricatorDestructibleInterface` interface is misspelled as `PhabricatorDestructableInterface`. Fix the spelling mistake.
Test Plan: `grep`. Seeing as this interface is fairly recent, I don't expect that this would cause any widespread breakages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9988
Summary: These got removed recently but I missed one callsite.
Test Plan: Used `git grep` to double check all other callsites.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9973
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary:
Ref T5245. This hooks up the translation/rendering methods added previously.
These are messy, but now extractable/translatable.
Test Plan: Viewed edge transactions and stories, saw correct strings.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9841
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary:
Ref T5245. See some discussion in D9838.
When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once.
To do this, allow edge types to `shouldWriteInverseTransactions()`. When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log.
Test Plan: Attached and detached revisions from tasks, saw transactions appear on both sides of the operation.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9839
Summary:
This was significantly easier than expected. Here's an example of what an extension class might look like:
```
<?php
final class AddRiskReviewHeraldCustomAction extends HeraldCustomAction {
public function appliesToAdapter(HeraldAdapter $adapter) {
return $adapter instanceof HeraldDifferentialRevisionAdapter;
}
public function appliesToRuleType($rule_type) {
return $rule_type == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL ||
$rule_type == HeraldRuleTypeConfig::RULE_TYPE_OBJECT;
}
public function getActionKey() {
return 'custom:add-risk';
}
public function getActionName() {
return 'Add risk rating (JSON)';
}
public function getActionType() {
return HeraldAdapter::VALUE_TEXT;
}
public function applyEffect(
HeraldAdapter $adapter,
$object,
HeraldEffect $effect) {
$key = "phragile:risk-rating";
// Read existing value.
$field_list = PhabricatorCustomField::getObjectFields(
$object,
PhabricatorCustomField::ROLE_VIEW);
$field_list->readFieldsFromStorage($object);
$field_list = mpull($field_list->getFields(), null, 'getFieldKey');
$field = $field_list[$key];
$field->setObject($object);
$field->setViewer(PhabricatorUser::getOmnipotentUser());
$risk = $field->getValue();
$old_risk = $risk; // PHP copies arrays by default!
// Add new value to array.
$herald_args = phutil_json_decode($effect->getTarget());
$risk[$herald_args['key']] = array(
'value' => $herald_args['value'],
'reason' => $herald_args['reason']);
$risk_key = $herald_args['key'];
// Set new value.
$adapter->queueTransaction(
id(new DifferentialTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD)
->setMetadataValue('customfield:key', $key)
->setOldValue($old_risk)
->setNewValue($risk));
return new HeraldApplyTranscript(
$effect,
true,
pht(
'Modifying automatic risk ratings (key: %s)!',
$risk_key));
}
}
```
Test Plan: Created a custom action for differential revisions, set up a Herald rule to match and trigger the custom action, did 'arc diff' and saw the action trigger in the transcripts.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: locutus, edutibau, ite-klass, epriestley, Korvin
Maniphest Tasks: T4884
Differential Revision: https://secure.phabricator.com/D8784
Summary:
Fixes T5489. Currently, if you make a `#proj` comment on an object already tagged with `#proj`, you get a "no effect" dialog.
Instead, continue if these transactions produce no effect (this is normal/expected, and consistent with `@user`).
Test Plan: Made two `#proj` comments in a row on a revision.
Reviewers: joshuaspence, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5489
Differential Revision: https://secure.phabricator.com/D9745
Summary:
Fixes T5424.
- One concrete issue: drafts were not being cleared properly because `__draft__` was not set on submission. This (mostly) fixes phantom drafts.
- This ajax comment magic feels weird and floaty and generally has problems. For example, if you add subscribers or inlines, all the stuff on the page which represents those won't update automatically. Instead, just reload. Maybe we'll ajax this stuff some day, but it feels like a net negative for now.
- Also remove it from other applications where it's currently used.
- Fix an issue with inline previews.
Test Plan: Made some comments on a mock, everything worked normally like I expected it to.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5424
Differential Revision: https://secure.phabricator.com/D9649
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: Ref T2628. This makes Transactions understand objects that can have project relationships, extract project mentions, and handle watching.
Test Plan: See next diff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2628
Differential Revision: https://secure.phabricator.com/D9340
Summary:
This does some backend cleanup of the tile stuff, and some general cleanup of other application things:
- Users who haven't customized preferences get a small, specific set of pinned applications: Differential, Maniphest, Diffusion, Audit, Phriction, Projects (and, for administrators, Auth, Config and People).
- Old tile size methods are replaced with `isPinnnedByDefault()`.
- Shortened some short descriptions.
- `shouldAppearInLaunchView()` replaced by less ambiguous `isLaunchable()`.
- Added a marker for third-party / extension applications.
Test Plan: Faked away my preferences and viewed the home page, saw a smaller set of default pins.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9358
Summary:
Ref T4967. Adds a "Watch" relationship to projects, which is stronger than member/subscribed.
Specifically, when a task is tagged with a project, we'll include all project watchers in the email/notifications. Normally we don't include projects unless they're explicitly CC'd, or have some other active role in the object (like being a reviewer or auditor).
This allows you to closely follow a project without needing to write a Herald rule for every project you care about.
Test Plan:
- Watched/unwatched a project.
- Tested the watch/subscribe/member relationships:
- Watching implies subscribe.
- Joining implies subscribe.
- Leaving implies unsubscribe + unwatch.
- You can't unsubscribe until you unwatch (slightly better would be unsubscribe implies unwatch, but this is a bit tricky).
- Watched a project, then recevied email about a tagged task without otherwise being involved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4967
Differential Revision: https://secure.phabricator.com/D9185
Summary: Fixes T4930. We currently show the edit/quote menu if a transaction group has //inline// comments, but this doesn't make sense and doesn't work properly. Only show this menu if the group has a normal comment.
Test Plan:
Viewed these groups:
- Normal comment (edits fine).
- Just inlines (no more edit menu).
- Inline + comment (edits fine, affects the normal comment properly).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: bitglue, epriestley
Maniphest Tasks: T4930
Differential Revision: https://secure.phabricator.com/D9180
Summary:
Ref T4968. If you add a comment to revision you aren't currently subscribed to, the email currently chooses "[Changed Subscribers]" as the action title. This is less interesting than "[Commented]", provided the affected subscriber is you (adding other people //is// usually interesting).
In this case, reduce the strength of this action below the strength of "comment".
Test Plan: Made several comments in conjunction with implicit and explicit subscriptions. Saw "[Commented]" for stuff affecting me, and "[Changed Subscribers]" for stuff affecting others.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4968
Differential Revision: https://secure.phabricator.com/D9168
Summary:
Create transaction, editor, etc, and move command generation over to editor.
Show in a timeline in the buildable page.
Also prevent Engine from creating an empty transaction when build starts (Fixes T4885).
Fixes T4886.
Test Plan: Restart builds and buildables, look at timeline.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4885, T4886
Differential Revision: https://secure.phabricator.com/D9110
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary:
Ref T4119.
- Trim newlines off the quoted text before quoting it; otherwise we can end up with a staircase of ">" at the end of a quote.
- Allow image macros to have leading whitespace, so multiple consecuitive quoted macros work properly.
Test Plan: I QUOTED MACROS A LOT OF TIMES
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8983
Summary:
man I sure hate Javascript
I removed the ajax-edit and ajax-remove interactions, becuase they were prohibitively complex to get working given that the entire menu has to change too. Instead, the page just reloads. This works perfectly fine in practice.
If we want to restore these in the future, we should have the server re-render the entire transaction group or something. I think very little is lost here, though.
Test Plan:
- Took all the actions.
- Used existing dropdown menus.
{F150196}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8966
Summary:
Ref T4119. This is ugly for now, but technically works.
The comment area and transaction log don't realy know about each other, so for the moment the linking is a bit manual. Differential/Maniphest are special cases anyway.
Test Plan: {F149992}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8957
Summary:
Fixes T4909. Adds a "remove" link next to the edit link, which permanently hides a comment. Addresses two use cases:
- Allowing administrators to clean up spam.
- Allowing users to try to put the genie back in the bottle if they post passwords or sensitive links, etc.
The user who removed the comment is named in the removal text to enforce some level of administrative accountability.
No data is deleted, but there's currently no method to restore these comments. We'll see if we need one.
This is cheating a little bit by storing "removed" as "2" in the isDeleted field. This doesn't seem tooooo bad for now.
Test Plan:
- Removed some of my comments.
- As an administrator, removed other users' comments.
- Failed to view history of a removed comment.
- Failed to edit a removed comment.
- Failed to remove a removed comment.
- Verified feed doesn't show the old comment after comment removal.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: qgil, chad, epriestley
Maniphest Tasks: T4909
Differential Revision: https://secure.phabricator.com/D8945
Summary:
tracing this a bit, in core/behavior-form.js we end up doing form.submit() on this form. Before this patch, the form had no submitURI set so it ended up doing a page load. Now, we set the submitURI and (at least) the form is actually submitted and data is saved. Ref T4669.
Not sure how come the syntheticSubmit even isn't getting picked up though?
Test Plan: hit apple + enter to submit comment edit. page reloaded and my edit persisted!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4669
Differential Revision: https://secure.phabricator.com/D8952
Summary: These stories/notifications aren't too useful, just turn them off at least for now.
Test Plan: Will vet this in a sec...
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8899
Summary: Fixes T4919. There's some special casing in Diffusion for CAN_PUSH right now, just accommodate that until things get more general.
Test Plan: Viewed a repository edit screen with a custom policy transaction. Clicked the link to view it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4919
Differential Revision: https://secure.phabricator.com/D8898
Summary: 'cuz those can be complicated. Fixes T4738. I needed to do a fair amount of heavy lifting to get the policy stuff rendering correctly. For now, I made this end point very one purpose and tried to make that clear.
Test Plan: looked at some custom policies. see screenshots.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4738
Differential Revision: https://secure.phabricator.com/D8890
Summary:
In some applications, using `{V2}` syntax to embed a vote throws. The chain of causality looks like this:
- We try to render a `phabricator_form()`.
- This requires a CSRF token.
- We look for a CSRF token on the user.
- It's an omnipotent user with no token, so everything fails.
To resolve this, make sure we always pass the real user in.
Test Plan:
- Lots of `grep`.
- Made a Differential comment with `{V2}`.
- Made a Diffusion comment with `{V2}`.
- Made a Maniphest comment with `{V2}`.
- Replied to a Conpherence thread with `{V2}`.
- Created a Conpherence thread with `{V2}`.
- Used Conduit to update a Conpherence thread with `{V2}`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lkassianik
Differential Revision: https://secure.phabricator.com/D8849
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.
Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8827
Summary: The token transactions can publish empty transaction feed stories.
Stop them from doing that, and make notifications fail more quietly.
Auditors: btrahan
Summary:
Ref T1049. When Harbormaster tests pass, don't bother sending an email about it.
(I tried to implement this earlier but didn't test it entirely properly, and we needed a little more code.)
Test Plan: Used `bin/harbormaster build` to build some junk, got no email about passes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D8813
Summary:
Fixes T4810. When a buildable completes, make an effort to update the corresponding object with a success or failure message. Commits don't support this yet, but revisions do.
{F144614}
Test Plan:
- Used `bin/harbormaster build` and `bin/harbormaster update` to run a pile of builds.
- Tried good/bad builds.
- Sent some normal mail to make sure the mail reentrancy change didn't break stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8803
Summary:
Ref T4810. Ultimate goal is to let Harbormaster post a "build passed/failed" transaction. To prepare for that, implement `PhabricatorApplicationTransactionInterface` in Differential.
To allow Harbormaster to take action on //diffs// but have the transactions apply to //revisions//, I added a new method so that objects can redirect transactions to some other object.
Test Plan:
- Subscribed/unsubscribed/attached/detached from Differential, saw transactions appear properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8802
Summary: See IRC. Some users are having difficulty figuring out why Herald is taking some actions. Make it easier to get to the transcript.
Test Plan: {F144622}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: dctrwatson, epriestley
Differential Revision: https://secure.phabricator.com/D8804
Summary:
Couple of minor cleanup things here:
- Pass handles to ApplicationTransactions when rendering their stories; this happened implicitly before but doesn't now.
- Add `?text=1` to do ad-hoc rendering of a story in text mode.
- Make Conduit skip unrenderable stories.
- Fix/modernize some text in the Commit story.
Test Plan: Rendered text versions of stories via Conduit and `?text=1`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: zeeg, spicyj, epriestley
Differential Revision: https://secure.phabricator.com/D8793
Summary: ...add a "renderingTarget" to FeedStory and use it as appropos. Overall, not a ton of changes was necessary to make this work. I think this could be made to be even cleaner by going through each and every feed story and re-implementing as necessary with the full toolset available. But this is good enough for now I think, and just something to keep cleaning up when we're in here. Fixes T4630.
Test Plan: made a task. gave it a token. viewed my feed - saw stories. used conduit.feed.query with mode == 'text' and got good readable results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: spicyj, epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8750
Summary: I also changed PhabricatorApplicationTransactionFeedStory and the TokenGivenFeedStory to include only the title/first line of the feed story, which is more convenient (previously, strip_tags gave a multi-line story without even any linebreaks) and more consistent with the other story types.
Test Plan: Added a requestbin URL to feed.http-hooks, commented on a Differential, and saw storyText equal to "alpert added a comment to D2: c." in the POST data it received.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8710
Summary: ...the key is to move a layer lower and beam down the updated comment. There is a wee bit of Javascript gymnastics going on here. Fixes T4608.
Test Plan: made a comment + resolve. clicked edit and made changes. noted transaction updated correctly and "history" link worked. edited again to a deletion and noted the "this is deleted" looked right and history link still worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T4608
Differential Revision: https://secure.phabricator.com/D8702
Summary: This "Reply to comment, etc., etc." section got lost along the way at some point. Restore it for transaction mail.
Test Plan: Received mail from Maniphest with reply instructions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8700
Summary: Ref T418. Fixes T4642. The "changes since last update" and "branch" fields got dropped; restore them in a general, field-driven way.
Test Plan:
- Created a revision, got relevant sections in mail.
- Commented on a revision, got relevant sections in mail.
- Updated a revision, got relevant sections in mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: spicyj, epriestley
Maniphest Tasks: T418, T4642
Differential Revision: https://secure.phabricator.com/D8657
Summary: Fixes T4629. CCs added by Herald don't get added to the cached subscriber list. Just reload subscribers before sending mail to pick up effects.
Test Plan: Created an "always add X as CC" Herald rule for revisions, created a revision, saw them get initial mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: spicyj, epriestley
Maniphest Tasks: T4629
Differential Revision: https://secure.phabricator.com/D8565
Summary: Fixes T4430. Basically does a little code massage from the new stuff in D8525 and application transactions to get this working. Adds a new controller to the subscriptions app to make rendering these pretty easy peasy.
Test Plan: Used my test task in D8525 to verify both add and rem versions of these dialogs worked correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, chad, Korvin
Maniphest Tasks: T4430
Differential Revision: https://secure.phabricator.com/D8540
Summary:
Fixes T4609. Steps are:
- Make a comment.
- Edit it.
- Delete all the text.
We expect to see "This comment has been deleted." -- instead, things currently render goofy.
Root cause is that `hasComment()` means both "comment object exists" //and// "comment object is nonempty".
Test Plan: {F128862}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4609
Differential Revision: https://secure.phabricator.com/D8533
Summary: End-cap for timeline. Fixes T4438
Test Plan: Tested on a timeline with and without endcap.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin, chad, btrahan
Maniphest Tasks: T4438
Differential Revision: https://secure.phabricator.com/D8530
Summary:
Fixes T4362. If you have a default edit + view policy of "no one" things get weird when you try to create a task - basically its impossible.
Ergo, re-jigger how we do policy checks just a bit.
- if its a new object, don't bother with the "can the $actor edit this thing by virtue of having can see / can edit priveleges?" That makes no sense on create.
- add a hook so when doing the "will $actor still be able to edit this thing after all the edits" checks the object can be updated to its ultimate state. This matters for Maniphest as being the owner lets you do all sorts of stuff.
Test Plan:
- made a task with no one policy and assigned to no one - exception
- made a task with no one policy and assigned to me - success
- made a comment on the task - success
- reassigned the task to another user - exception
- reassigned the task to another user and updated policies to "users" - success
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Maniphest Tasks: T4362
Differential Revision: https://secure.phabricator.com/D8508
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
