Summary: Fixes T6732. Fix is to stop trying to catch the error in the controller and let the editor do its job.
Test Plan: tried to add an existing subscriber and got an error message about how that wouldn't do anything
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6732
Differential Revision: https://secure.phabricator.com/D11303
Summary: Fixes T6179. This makes the interaction where users remove a task from a workboard much more pleasant.
Test Plan: Loaded up workboard for "A Project". Edited tasks and if / when I removed "A Project" they disappeared on save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6179
Differential Revision: https://secure.phabricator.com/D11259
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplicationSearchEngine` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11242
Summary: Fixes T6555, The following should 404: /maniphest/task/create/?parent=asdf, /maniphest/task/create/?parent=0, /maniphest/task/create/?parent=999999 (where T999999 does not exist)
Test Plan: Navigate to /maniphest/task/create/?parent=asdf or /maniphest/task/create/?parent=0 or /maniphest/task/create/?parent=999999 (where T999999 does not exist). See 404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6555
Differential Revision: https://secure.phabricator.com/D11258
Summary: When updating the status of a task via commit, transaction should show responsible commit and status update if it was changed.
Test Plan: Push a commit "Fixes Txx", transaction should include status update and commit number.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6637
Differential Revision: https://secure.phabricator.com/D11230
Summary: CLosed is a pretty important state and black tends to blend in a bit. This bumps to an alternate color to improve ability to scan and know state of objects.
Test Plan:
Review a number of closed objects. I will follow up with another diff on 'Archived' colors.
{F261895}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11222
Summary: This class is no longer used after D6673.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11167
Summary: This class is no longer used after D10965.
Test Plan: `grep`
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11133
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: Modernize Pholio edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Attached a mock to a task, observed the expected comment in the transaction view (both ways).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11113
Summary: This method has been deprecated for a long time (see D2887 and D6336).
Test Plan:
```lang=bash
> echo '{}' | arc --conduit-uri=http://phabricator.local call-conduit 'maniphest.find'
Waiting for JSON parameters on stdin...
{"error":"ERR-CONDUIT-CALL","errorMessage":"ERR-CONDUIT-CALL: ","response":null}
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11117
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: Modernize Differential edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Krenair, epriestley
Differential Revision: https://secure.phabricator.com/D11074
Summary: This is a typo from D11045.
Test Plan: I haven't actually tested this, but the tests from D11045 should apply here.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11108
Summary: Fixes T6838. We use a special transaction type for merging, but don't handle it when figuring out mail tags.
Test Plan: Verified merge mail picks up the `maniphest-status` tag.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6838
Differential Revision: https://secure.phabricator.com/D11101
Summary: database migration + drop old view code. Fixes T5604.
Test Plan: grepped src/ for TYPE_CCS (no hits); viewed some tasks with old cc transactions and noted they still rendered correctly post data conversion
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D11015
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: Ref T5245. This is some of the associated cleanup there.
Test Plan:
foreach ManiphestTaskQuery site, I made the change (or not) and tested as follows:
=== Call sites where added needProjectPHIDs ===
- PhabricatorHomeMainController - loaded the home page
- ManiphestBatchEditController - batch edited some tasks (added a project)
- ManiphestConduitAPIMethod - tested implicitly when tested ManiphestUpdateConduitAPIMethod
- ManiphestInfoConduitAPIMethod - used the method via conduit console with input id : 1
- ManiphestQueryConduitAPIMethod - used the method via conduit console with input ids : [1, 2]
- ManiphestUpdateConduitAPIMethod - used the method via conduit with input id : 1 and comment : “asdasds"
- ManiphestReportController - viewed “By User” and “By Project”
- ManiphestSubpriorityController - changed the priority of a task via a drag on manphest home
- ManiphestTaskMailReceiver - updated Task 1 via bin/mail receive-test with a comment that is the README
- ManiphestTaskSearchEngine - loaded Manifest home page
- ManiphestTaskEditController - edited a task
- ManiphestTransactionEditor - closed a blocking task
- ManiphestTransactionSaveController - commented on a task
- PhabricatorProjectProfileController - viewed project with id of 1 that has a few tasks in it
- PhabricatorSearchAttachController - merged tasks together
- DifferentialTransactionEditor - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
- PhabricatorRepositoryCommitMessageParserWorker - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated
=== Calls sites where *did not* add needProjectPHIDs (they do not appear in this revision) ===
- PhabricatorManiphestApplication - loaded the home page
- ManiphestGetTaskTransactionsConduitAPIMethod - used the method via conduit console with input ids : [1, 2] ManiphestTaskDetailController - viewed a task with and without associated projects; finished workflow creating a task with a parent
- ManiphestTransactionPreviewController - verified transaction preview showed up properly
- PhabricatorProjectBoardViewController - viewed a board
- PhabricatorProjectMoveController - moved a task around
- ManiphestRemarkupRule - made a task reference like {T123}
- ManiphestTaskQuery - executed a custom query for all tasks with page size of 2 and paginated through some tasks
- ManiphestTaskPHIDType - nothing random seems broken? =D
=== Call sites where had to do something funky ===
- ManiphestHovercardEventListener - loaded hover cards from task mentions
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D11004
Summary: Fixes T6748. This just didn't get aligned when CCs became a modern transaction.
Test Plan: Added a CC to a task, used `bin/mail show-outbound` to verify it showed up as a CC tag.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6748
Differential Revision: https://secure.phabricator.com/D10991
Summary: Fixes T6595. This diff has two issues as is... 1) the differential data fetching is pretty cheesey, but it looks like we can't just issue three separate databases to get the right data? 2) the translations break, since I am turning this into a string (and not an int) so the whole pluralization bit fails. I think 1 is okay as is and 2 needs to be fixed though I am not sure how to best do that...
Test Plan: loaded home page and it looked nice...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6595
Differential Revision: https://secure.phabricator.com/D10979
Summary: Ref T5604. Found this trying to open T5604 live. Basically this internal query needs the needSubscriberPHID set to true.
Test Plan: doing it live
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D10975
Summary: (Needed a clean branch). Moves the field up and renames to Query
Test Plan: Visit Maniphest Search, see new field, test a query
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10971
Summary: Fixes T5604. This should fix some random bugs, lets us move forward more easily, and all that good stuff about killing code debt.
Test Plan:
- Conduit method maniphest.createtask
- verified creating user subscribed
- verified subscription transaction
- Conduit method maniphest.update
- verified subscribers set as specified to ccPHIDs parameter
- verified subscription transaction
- Herald
- verified herald rule to add subscriber worked
- verified no subscribers removed accidentally
- edit controller
- test create and verify author gets added IFF they put themselves in subscribers control box
- test update gets set to exactly what user enters
- lipsum generator'd tasks work
- bulk add subscribers works
- bulk remove subscriber works
- detail controller
- added myself by leaving a comment
- added another user via explicit action
- added another user via implicit mention
- task merge via search attach controller
- mail reply handler
- add subscriber via ./bin/mail receive-test
- unsubscribe via ./bin/mail receive-test
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5604
Differential Revision: https://secure.phabricator.com/D10965
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: 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: Fixes T4652, adding workboard link to emails
Test Plan: Move a task in a workboard from one column to another. Email notification should contain "WORKBOARD" section with link to that workboard
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4652
Differential Revision: https://secure.phabricator.com/D10889
Summary: Uses the check icon for closed, which is the primary action taken.
Test Plan:
Close as a duplicate, seen new icon.
{F236048}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10882
Summary: Finishes up work on PHUIInsetForm, cleans up the UI a bit when editing multiple tasks
Test Plan:
Edit multiple tasks, apply some new projects, still works as expected.
{F236044}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10881
Summary: Fixes T3189. Now if you say #projects in a commit message they will associate nicely with the commit. Also we record transactions about all this project editing fun.
Test Plan: tested migration by associating some projects with commits and verifying they still showed up post migration. tested adding / removing projects by doing so from the UI, noting transactions written nicely as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Projects: #projects
Maniphest Tasks: T3189
Differential Revision: https://secure.phabricator.com/D10877
Summary:
When a logged-out user views the home page, we currently compute a meaningless "assigned tasks" count which just counts every open task. Don't do this.
Ideally `ManiphestTaskQuery` should get some tightening up too (that is, `withOwners(array(null))` should not select every task), but that might affect other stuff and the performance implications of counting every open task are affecting WMF, so just fix the immediate issue for now.
Test Plan:
- Viewed homepage as a logged out user, no assigned task count.
- Viewed homepage as a logged-in user, still saw assigned task count.
Reviewers: btrahan, chad, chasemp
Reviewed By: chasemp
Subscribers: 20after4, epriestley
Differential Revision: https://secure.phabricator.com/D10876
Summary: ...way way down in PhabricatorController. Use it on ManiphestTaskDetailController to test it. Ref T4712. I think the pager logic to be added as part of T4712 can safely reside entirely within this method. As I said earlier, 5 parameters is a lot, so I don't really want to add more. Next diff would do the pagination logic and the diff after that would deploy it everywhere. If while deploying it everywhere I find something off, that will be a different diff.
Test Plan: viewed maniphest tasks and they looked as spiffy as ever.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10844
Summary: Fixes T5015, Allow Herald rules for Maniphest to act on task status changes.
Test Plan: Create Herald rule for Maniphest tasks to flag a task with status "wontfix". Change status of Maniphest task to "wontfix". Task should be flagged.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5015
Differential Revision: https://secure.phabricator.com/D10842
Summary: When merging tasks, the corresponding transaction on the merged task should be black, and the transaction on the ultimate task should be green.
Test Plan: Create two tasks, merge one into the other, merged task transaction is black, the surviving task should show a green transaction.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6500
Differential Revision: https://secure.phabricator.com/D10827
Summary: Missed this in previous pass. Send these as links in HTML emails.
Test Plan: Register a new user that nees approval.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10815
Summary: Fixes T6343. Grepped for all callsites and added addLinkSection where needed.
Test Plan: Tested Differential, Maniphest, Conpherence, Ponder and Macro. Inspect HTML mail for anchor tags. Inspect text mails for non-disruption.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: talshiri, Korvin, epriestley
Maniphest Tasks: T6343
Differential Revision: https://secure.phabricator.com/D10762
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary: Fixes T6254 and renames status as string. Though maybe this should go through `formatStringConstants`?
Test Plan: Reload Conduit page, see new text.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6254
Differential Revision: https://secure.phabricator.com/D10637
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T1191. For most text columns, we either don't care if "a" and "A" are the same, or we expect them to be different (for example: keys, domains, secrets, etc). Default text columns to the `_bin` collation so they are compared by strict character value. This is safer in cases where we aren't sure.
For some text columns, we allow the user to sort by the column in the UI (like Maniphest task titles) or we do care that "A" and "a" are the same (for example: project names). Introduce a new class of virtual data types, the "sort..." types, to cover these columns. These are like the "text..." types but use sorting collations which treat "A" and "a" the same.
Test Plan:
- Made an effort to identify all columns where the UI relies on database collation.
- Ran `bin/storage adjust` and cleared all warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: beng, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10602
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary:
Ref T1191. Notes:
- Drops the project affiliation table. This is a very old membership table which was migrated to edges.
- Drops the subproject table. This is a very old table for a removed feature.
Test Plan:
- Grepped for dropped tables.
- Saw ~100 fewer setup issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10581
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: 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 T6056. This documentation is out of date and not very useful.
We could probably fold this option into `maniphest.priorities` at some point.
Test Plan: Read documentation, clicked link.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6056
Differential Revision: https://secure.phabricator.com/D10450
Summary: see title. Ref T5875.
Test Plan: Merged one task into another task - verified transactions on both tasks. Merged two tasks into another task - verified transactions on all three tasks. Checked out my feed and saw MERGE_INTO stories and MERGE_FROM stories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5875
Differential Revision: https://secure.phabricator.com/D10427
Summary: Ref T6031. I figure its totally cool to include the user creating the task as a subscriber, even if from the template case, so just do that there too. Code is written such that if the user wasn't already in the subscriber case they end up being the last person in the tokenizer. Theoretically this should make any users who didn't want to be automagically subscribed via the create from template case to remove themselves.
Test Plan: made a template from a task that didn't have me as a subscriber initially and observed i was a subscriber.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6031
Differential Revision: https://secure.phabricator.com/D10408
Summary: Fixes T6029. We should append custom fields last so they show up after things like projects, tokens, etc that render via UI events.
Test Plan: viewed a task with custom fields and projects was last
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6029
Differential Revision: https://secure.phabricator.com/D10407
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary:
Fixes T5140. When you ajax-edit a task and we send back a full-size card, we currently always put a drag grip on it.
If you clicked the "edit" thing from a priority-ordered list, this is appropriate. However, if you clicked it from some other type of list, it is not.
Pass the expected grippableness through the call.
Test Plan:
- Edited a task from a reorderable (priority-ordered) view, got grip.
- Edited a task from a nonreorderable (author-ordered) view, got no grip.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5140
Differential Revision: https://secure.phabricator.com/D10282
Summary: Fixes T5769. Fixes T5861. Add mail tags for "unblock" and "column change".
Test Plan: Did unblocks and column changes, verified the mail got the right mailtags and recipient nondelivery flags.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861, T5769
Differential Revision: https://secure.phabricator.com/D10241
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 T5859. This doesn't change much, but makes the transaction record a little more accurate and activates stuff like `#hashtags` and `{F123}` causing policy associations.
Test Plan: Used `bin/mail receive-test` and mail receiver script to send bug mail, saw hashtags imply projects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5859
Differential Revision: https://secure.phabricator.com/D10229
Summary:
Fixes T5184. Fixes T5008. Three issues with stories/notifications about changing the status of tasks which block other tasks:
**Bad Feed Stories**
- Problem: Feed story rendering was confusing (T5184).
- Solution: fix it to provide context.
**Too Many Feed Stories**
- Problem: Feed gets a story for the original task's close ("a closed x"), and a story for each blocked task ("a closed x, a task blocking y").
- "Solution": Punt. These are redundant in the full feed but not in filtered feeds. Right solution is display-time aggregation. No users have really complained about this.
**Too Many Notifications**
- Problem: Users subscribed to both tasks get notified about the clsoe, and also about the unblocked task. These notifications are redundant.
- "Solution": Punt. This is easy to fix by silencing notifications for the sub-editor, but I'm worried it would be confusing. Users haven't complained. Display-time aggregation might be a better fix.
Test Plan: {F189463}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5008, T5184
Differential Revision: https://secure.phabricator.com/D10235
Summary: To assist with {T5245}, I have added projects back into the lipsum maniphest generator with the edge infrastructure.
Test Plan: Run the lipsum script for PhabricatorManiphestTaskTestDataGenerator and make sure it generates project data.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10202
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: Fixes T5829. This stuff is old and busted, but keep it working for now.
Test Plan: No more fatal when there are recently closed tasks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5829
Differential Revision: https://secure.phabricator.com/D10201
Summary:
Via the UI adding a mailinglist for CC works, but via
the API currently it shows:
>One or more PHIDs were invalid for ccPHIDS
This removes the user validation check for ccPHIDs.
(I left it in for other things like owner since that seems
still appropriate?)
Test Plan:
used arc locally to add a mailinglist to cc
```echo '{"id": 2, "ccPHIDs": ["PHID-MLST-ohduchbv4dfimk7opt3r"]}' | arc call-conduit maniphest.update```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10193
Summary:
Ref T4807. This is probably a complete fix, but I'd be surprised if there isn't a little cleanup I missed.
When users drag tasks on a "natural"-ordered workboard, leave things where they put them.
This isn't //too// bad since a lot of the existing work is completely reusable (e.g., we don't need any new JS).
Test Plan:
- Dragged a bunch of stuff around, it stayed where I put it after dropped and when reloaded.
- Dragged stuff across priorities, no zany priority changes (in "natural" mode).
- Created new tasks, they show up at the top.
- Tagged new tasks, they show up at the top of backlog.
- Swapped to "priority" mode and got sorting and the old priority-altering reordering.
- Added tasks in priority mode.
- Viewed task transactions for correctness/sanity.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10182
Summary:
Ref T4807. This is an alternative to D10179. The problem these diffs solve is that I want to be able to reorder a column's positions without having to load the actual objects, but that's difficutl because two positions may have the same sequence number (and I think it's good that we allow that, since it makes a bunch of other stuff way easier).
Instead of using the object ID (e.g., the task ID) to reorder positions with the same sequence, use the position itself. This is a little easier, is less ambiguous if columns eventually have several types of objects, and produces a better behavior when old objects are freshly added to a board. For example, if you tag `T300` with `#project`, this new rule will push it to the top of "Backlog" while the old rule might have buried it deep. I think this behavior is desirable and more "natural".
When creating a group of new rows, we do order the batch by ID, so a group of freshly-tagged objects float to the top togehter in ID order. This seems like the most natural rule, too.
Test Plan:
- Loaded some boards with implicit objects on them (freshly tagged tasks) and saw rows create.
- Verified new rows created in the right order.
- Dragged some tasks around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10180
Summary:
Ref T4807. This doesn't actually do anything yet, but adds a dropdown menu for choosing an ordering and gets all the UI working correctly.
This also fixes a bug where column hidden state wouldn't persist across filter changes.
(I won't land this until it does something, but the next diff will probably be a mess so this seemed like a clean place to sever things.)
Test Plan:
{F187114}
- Altered sort ordering.
- Altered hidden state and filters, verified all states persisted correctly.
- Added `phlog()` to edit/create and move controllers and verified they receive sort information.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: swisspol, chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10178
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:
Fixes T5476. Using edges to store which objects are on which board columns ends up being pretty awkward. In particular, it makes T4807 very difficult to implement.
Introduce a dedicated `BoardColumnPosition` storage.
This doesn't affect ordering rules (T4807) yet: boards are still arranged by priority. We just read which tasks are on which columns out of a new table.
Test Plan:
- Migrated data, then viewed some boards. Saw exactly the same data.
- Dragged tasks from column to column.
- Created a task directly into a column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5476
Differential Revision: https://secure.phabricator.com/D10160
Summary:
Ref T4896. Now that we have a transaction editor, we can delete a giant block of hacks.
I believe this also resolves the commit/task attachment issues @joshuaspence and @mbishopim3 mentioned.
Test Plan: Attached and detached commits and tasks.
Reviewers: btrahan, joshuaspence, mbishopim3
Reviewed By: mbishopim3
Subscribers: mbishopim3, epriestley, joshuaspence
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10138
Summary:
Ref T4589. We don't recognize task descriptions as remarkup blocks, so `{F...}` references in them do not get attached to the objects, and thus no policy exemption is created.
Recognize them, which activates `{F...}` and `@mentions`.
We probably have a few more of these in other applications, but it's not a big deal to clean them up as they arise.
Test Plan: Uploaded a file to a task in the description field, saw it attach and get a policy exemption.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10139
Summary:
Resolves T4659. This implements support for sorting tasks by custom fields.
Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query.
Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4659
Differential Revision: https://secure.phabricator.com/D10106
Summary: Fixes T5751. Currently, we incorrectly overwrite `$xactions`.
Test Plan: Closed a blocking task, got an email about the correct transaction set ("closed task; added comment") instead of an overwritten transaction set ("closed blocking task").
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5751
Differential Revision: https://secure.phabricator.com/D10088
Summary: At least on my install, sorting was pretty borked from a type issue. (e.g. "unbreak now" of 100 sorting as less than "High" of 90). Fix this with some parseInt action. Also support adding new cards with the new colsort stuff. The clever bit here is to include the task ID in the sorting vector because the task ID wins ties at the moment I think / new tasks need to show up before older tasks when they are initially created. Fixes T5716.
Test Plan: added many "normal" priority cards and saw them fly in correctly. changed priority and moved correctly. made no edits and no moves were made correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5716
Differential Revision: https://secure.phabricator.com/D10081
Summary: Instead of implementing the `getCapabilityKey` method in all subclasses of `PhabricatorPolicyCapability`, provide a `final` implementation in the base class which uses reflection. See D9837 and D9985 for similar implementations.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D10039
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: Fixes T5661. We may now pick up a conflicting `dateCreated` field from an edge table join.
Test Plan: Ran a project + dateCreated filtering query, no longer got an exception.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5661
Differential Revision: https://secure.phabricator.com/D9997
Summary: Ref T4420. This one is users plus "upforgrabs". I renamed that to "none" and gave it a special visual style to make it more discoverable. Future diffs will improve this.
Test Plan:
- Used it in global search.
- Used it in batch editor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9891
Summary: Ref T4420. This is just "project", plus the special "no project" token, but that doesn't actually work. Replace it with a normal project typeahead. This is only used in Maniphest's reports.
Test Plan: Searched for a couple of projects in reports.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9890
Summary:
Ref T5245. We'll still display the old ones, but write real edge transactions now -- not TYPE_PROJECTS transactions.
Some code remains to show the existing transactions. The next diff will modernize the old transactions so we can remove this code.
Test Plan:
- Previewed a project-editing comment.
- Submitted a project-editing comment.
- Edited a task's projects.
- Batch edited a task's projects.
Reviewers: joshuaspence, chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9852
Summary: Ref T5245. This property predates edges and is unusual in modern applications. Stop writes to it and populate it implicitly from edges when querying.
Test Plan:
- Viewed task list.
- Created a task.
- Added and removed projects from tasks.
Reviewers: joshuaspence, chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9851
Summary: Ref T5245. This moves the actual storage over and stops reads and writes to the old table.
Test Plan:
- Verified tasks retained projects across the migration.
- Added and removed projects from tasks.
- Searched for: all, any, users' projects, not-in-projects, no-projects.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9850
Summary: Ref T5245. With work elsewhere (notably, D9839) we can remove this TODO and use real transactions.
Test Plan: Pushed a `closes Txxx` commit and got a close + transaction.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9848
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:
Ref T5245. A very long time ago I had this terrible idea that we'd let objects react to edges being added and insert transactions in response.
This turned out to be a clearly bad idea very quickly, for like 15 different reasons. A big issue is that it inverts the responsibilities of editors. It's also just clumsy and messy.
We now have `PhabricatorApplicationTransactionInterface` instead, which mostly provides a cleaner way to deal with this.
Implement `PhabricatorApplicationTransactionInterface`, implicitly moving all the attach actions (task/task, task/revision, task/commit, task/mock) to proper edge transactions.
The cost of this is that the inverse edges don't write transactions -- if you attach an object to another object, only the object you were acting on posts a transaction record. This is sort of buggy anyway already. I'll fix this in the next diff.
Test Plan: Attached tasks, revisions and mocks to a task, then detached them.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9838
Summary: Ref T5245. Currently, task/project links rely on side effects in `save()`. Make them more transaction-oriented, with the goal of moving fully to edges a few diffs down the line.
Test Plan:
- Added and removed projects using "Edit Task", "Associate Projects" comment action, and Herald.
- Verified database ended up in the expected state.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9833
Summary: Ref T4420. Update "projects" source.
Test Plan:
- Edited projects on a Differential revision.
- Edited projects on a commit.
- Edited projects on a repository.
- Edited projects in feed search.
- Edited projects in a Herald rule field.
- Edited projects in a Herald rule action.
- Edited projects in Maniphest batch editor.
- Edited projects on Maniphest task.
- Edited projects in "Associate Projects..." action in Maniphest.
- Edited projects on Maniphest search in "all projects", "any project" and "not projects" fields.
- Edited projects on a Paste.
- Edited projects on a Pholio mock.
- Edited projects on a custom policy rule.
- Edited projects on a Ponder question.
- Edited projects on a Diffusion search query.
- Edited projects on a global search query.
- Edited projects on a slowvote.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9884
Summary: change typeahead placeholder to include 'or "upforgrabs" to unassign...' Fixes T2267. Well, makes it as good as its going to be until we get some new space age UI.
Test Plan: new text looks okay-ish
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2267
Differential Revision: https://secure.phabricator.com/D9882
Summary:
Ref T5476. Currently, the task edit code assumes it knows what the UI looks like and sends back where on the column an item should be inserted.
This is buggy after adding filters, and relatively complex. Instead, send down the ordering on the whole column and sort it in the UI. This is a bit simpler overall and more general. It makes it easier to further generalize this code for T5476.
Test Plan:
- Edited a task on a board, changing priority. Saw it reorder properly.
- Edited a task on a board in a field of other tasks at the same top-level priority. Saw it refresh without reordering.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5476
Differential Revision: https://secure.phabricator.com/D9832
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: Toss the hard-codes and use slim tags.
Test Plan: Scoped out task list.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9748
Summary:
Standardizes tag rendering in Maniphest and Maniphest/Diffusion list views.
(This might need some size/spacing tweaks, I tried to make it look reasonable.)
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9741
Summary:
Fixes T5467.
- Let search engines figure out if they're rendering for a panel or not.
- If Maniphest is rendering a panel, turn off the grips and batch selection.
Test Plan:
- Viewed task panels (no grips).
- Viewed non-panel query results (grips).
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5467
Differential Revision: https://secure.phabricator.com/D9714
Summary:
Ref T5446.
- For all callsites which do not specify a value, set `false` explicitly.
- Make `true` the default.
Test Plan: Used `grep`, then manually went through everything.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5446
Differential Revision: https://secure.phabricator.com/D9687
Summary: Not sure if this is the correct fix, but I think it's where you intend to go?
Test Plan: Click on link in Task, get the the correct board. Click lots of links of boards and make sure everything still works.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5361
Differential Revision: https://secure.phabricator.com/D9520
Summary: Ref T4986. Instead of requiring users to know the name of an application search engine class, let them select from a list.
Test Plan:
Created a new panel.
{F165468}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9500
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: This went smoother than expeced. Makes the rounded Card the default, also tweaked selected state a little.
Test Plan:
Test UIExamples, Maniphest, Home, Differential, Harbormaster, Audit. Everything seems normal
{F163971}
{F163973}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9408
Summary: Replace PanelView with ObjBox. Make burnup chart look less hated.
Test Plan:
Test a project, non project, various layouts on Maniphest Reports
{F163644}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9397
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: Fixes T5215. This mentions an old article name.
Test Plan: Read config option.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5215
Differential Revision: https://secure.phabricator.com/D9331
Summary: Fixes T4818. Clarify that this does not search for arbitrary text substrings.
Test Plan: `grep`
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4818
Differential Revision: https://secure.phabricator.com/D9278
Summary: Ref T5021. This specific label is a little more clear as "Blocks". See also IRC.
Test Plan: eyeballed it
Reviewers: btrahan, lpriestley, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5021
Differential Revision: https://secure.phabricator.com/D9279
Summary: Fixes T5021, UI labels for the fields, "Edit Dependencies" in the action list, transaction strings ("added dependent tasks", etc), UI strings in the dependencies dialog (title/submit/etc)
Test Plan: Open task, edit blocks, dialog should have new term, task history should show "blocks" instead of "dependencies"
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5021
Differential Revision: https://secure.phabricator.com/D9270
Summary:
Ref T4673.
IMPORTANT: I had to break one thing (see TODO) to get this working. Not sure how you want to deal with that. I might be able to put the element //inside// the workboard, or I could write some JS. But I figured I'd get feedback first.
General areas for improvement:
- It would be nice to give you some feedback that you have a filter applied.
- It would be nice to let you save and quickly select common filters.
- These would probably both be covered by a dropdown menu instead of a button, but that's more JS than I want to sign up for right now.
- Managing custom filters is also a significant amount of extra UI to build.
- Also, maybe these filters should be sticky per-board? Or across all boards? Or have a "make this my default view"? I tend to dislike implicit stickiness.
Test Plan:
Before:
{F157543}
Apply Filter:
{F157544}
Filtered:
{F157545}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: qgil, swisspol, epriestley
Maniphest Tasks: T4673
Differential Revision: https://secure.phabricator.com/D9211
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:
Ref T5008. Three notes:
- I'm not hiding these even if the status change is open -> open or closed -> closed. I think these are OK, but might be a little spammy.
- These show in feed, but shouldn't, since they're very redundant with stories which will almost always appear adjacently. Probably a bit spammy, see TODO. We can't hide them from feed without also squelching the notifications right now, which I //don't// want to do.
- You get a notification even if you're on the original task which changed status. This is definitely spammy, see other TODO.
Test Plan: {F156217}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5008
Differential Revision: https://secure.phabricator.com/D9166
Summary:
Updates policy, headers, typeaheads to FA over policy icons
Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late
Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names
Browsed numerous places, checked new dropdowns, see pudgy people.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4739
Differential Revision: https://secure.phabricator.com/D9179
Summary: This doesn't have any impact on anything right now (we don't persist the query) but could in the future, so I just left it as-is but fixed the typo.
Test Plan: looked at it carefully
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: WikiChad, epriestley
Differential Revision: https://secure.phabricator.com/D9155
Summary:
Fixes T5081. This block enforces policies and prevents you from seeing groups associated with projects you can't see. However, it incorrectly removes tasks which end up with no group key. This can happen in two cases:
- The task isn't in a project.
- The task is in a project, but the query includes an "In All Projects: <that project>" constraint. In that case, we don't show the group becuase it would always contain every task.
Test Plan:
Replicated the setup in T5081, saw an "Ungrouped" group with "Task A":
{F155766}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5081
Differential Revision: https://secure.phabricator.com/D9152
Summary: Ref T5058. The use of "enum" is confusing; we mean "choose one of these specific string constants". Make this more clear.
Test Plan: Viewed each call from the web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5058
Differential Revision: https://secure.phabricator.com/D9127
Summary: Remove white app icons, no longer in use as far as grep/memory serve. These were for list hover states.
Test Plan: Rebuild sprites, celerity. Grep for appIcon use (only feed). Verify all action lists are driven by FontAwesome.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9078
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: See <http://fab.wmflabs.org/T88>. While this issue is on MW's side, these links are kind of ugly and have more readable alternate forms now. Update them to use proper modern forms.
Test Plan: See inlines.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9042
Summary: Fixes T4995. This uses commas so it shouldn't have `[]`. I think I just derped this a while ago.
Test Plan: fiddled
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4995
Differential Revision: https://secure.phabricator.com/D9009
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: Ref T4938. `arc close` needs to know about custom statuses and this conduit method is step 1 of letting it know
Test Plan: See next diff, which works!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4938
Differential Revision: https://secure.phabricator.com/D8937
Summary:
Fixes T4917. Currently, if a user doesn't have access to, e.g., Phriction, they still get a checkbox in the search results to search for Wiki Documents. Those results will be filtered anyway, so this is confusing at best.
Instead, bind PHID types to applications. This is a relatively tailored fix; some areas for potential future work:
- Go through every PHID type and bind them all to applications. Vaguely nice to have, but doesn't get us anything for now.
- If no searchable application is installed, we don't show you an error state. This isn't currently possible ("People" is always installed) but in the interest of generality we could throw an exception or something at least.
- The elasticserach thing could probably constrain types to visible types, but we don't have a viewer there easily right now.
Test Plan: Uninstalled Phriction, saw the checkbox vanish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4917
Differential Revision: https://secure.phabricator.com/D8904
Summary: Fixes T4819, remove status "duplicate" from dropdown in edit task unless task is already in duplicate status
Test Plan: Edit task, not in duplicate status, verify dropdown does not have "duplicate" option. Edit task already in "duplicate" status, verify that dropdown shows "duplicate" status option.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4819
Differential Revision: https://secure.phabricator.com/D8902
Summary: See IRC. We construct this a little bit wrong if there are multiple "open" statuses. Use a more modern construction.
Test Plan: Hit `?statuses=wontfix,invalid`, etc. Clicked "view all" from projects.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8854
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: center aligns the icons in the fill area, removes some of the positioning jank. Also set new icons for maniphest custom.
Test Plan: test desktop and mobile layouts, tested thin pins for proper centering.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4866
Differential Revision: https://secure.phabricator.com/D8839
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: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.
Test Plan: Looked at a revision, task, paste, macro, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8812
Summary: I recently made this better about accepting project names, but we use it in some cases with PHIDs. Make that work properly again.
Test Plan: Clicked "New Task" from a project page.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8778
Summary: Fixes T4655. Basically leaves the display code intact for legacy installs but removes the option from the UI and removes "create" code.
Test Plan:
tried to attach file and the action was not in the dropdown!
made a new task and it worked!
commented on an old task and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4655
Differential Revision: https://secure.phabricator.com/D8777
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 added a getTaskPriorityColor function to the ManiphestTaskPriority class which returns the color set in the maniphest config for the given priority.
This is in preparation for a change to arcanist which will allow it to display the priority color (if it is a supported color) upon running `arc tasks`.
Fixed some linting issues
Test Plan:
Invoke the maniphest.info method from conduit and ensure that:
* The priorityColor property is given in the json
* the priorityColor property is set correctly
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8734
Summary: Fixes T4777. We technically support `?projects=...` already, but parse it in an unusual way and apply old, awkward, excessively strict lookups to it.
Test Plan: Used reasonable, standard, human-readable strings to prefill `?projects=` and got the results I expected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4777
Differential Revision: https://secure.phabricator.com/D8733
Summary:
Arcanist is currently displaying all tasks as closed when invoking `arc tasks`.
This is because arcanist is setting the display to closed if there is anything in the `status` property. Adding an isClosed property will allow arcanist to properly display open/closed status on tasks by checking against the isClosed property. The isClosed property will be set according to the closed property that is set on each status in maniphest.
Test Plan:
Invoke the conduit maniphest.info method on any task and insure that:
# The isClosed property is included in the properties
# that it is set properly according to the statuses set for maniphest.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4744
Differential Revision: https://secure.phabricator.com/D8731
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: The "burnup chart" relies on these to determine when tasks opened and we recently stopped writing them. Keep writing them for now. They're fluff and don't show up in the UI, but draw the right chart.
Test Plan: Saw chart go up when I made tasks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8682
Summary: the quotes are 'cuz "create" is inferred. Previously, we inferred on "status", but since we set that on "initializeNewTask" instead infer off "title" (aka "name") like most other apps do. Only hairy tweak was to elevate TYPE_TITLE to the most important of all maniphest transactions, which doesn't actually seem too unreasonable if not correct even? Fixes T4686.
Test Plan: made a new task, used bin/mail, got the right headers (mail vary prefix == created)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4686
Differential Revision: https://secure.phabricator.com/D8639
Summary: 0 => imploded string of hotness. Fixes T4689
Test Plan: for each spot i fixed, clicked link and it worked! (I did a grep for "/maniphest" to find these spots; 98% confident I got them all.)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4689
Differential Revision: https://secure.phabricator.com/D8626
Summary: Fixes T4641.
Test Plan: Dragged a "normal" task between "high" and "low" tasks and it stayed as "normal". Generally seems correct when playing around.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: mbishopim3, Beltran-rubo, epriestley, Korvin
Maniphest Tasks: T4641
Differential Revision: https://secure.phabricator.com/D8622
Summary:
Ref T1049. Fixes T4602. Moves all the funky field stuff to CustomField. Uses ApplicationTransactions to apply and record edits.
This makes "artifact" fields a little less nice (but still perfectly usable). With D8599, I think they're reasonable overall. We can improve this in the future.
All other field types are better (e.g., fixes weird bugs with "bool", fixes lots of weird behavior around required fields), and this gives us access to many new field types.
Test Plan:
Made a bunch of step edits. Here's an example:
{F133694}
Note that:
- "Required" fields work correctly.
- the transaction record is shown at the bottom of the page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4602, T1049
Differential Revision: https://secure.phabricator.com/D8600
Summary: Fixes T1812. Moves the internal configuration into public space and documents it.
Test Plan:
- Tried to set it to some invalid stuff.
- Set it to various valid things.
- Browsed around, changed statuses, filtered statuses, viewed statuses, merged duplictes, examined transaction record, created tasks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8585
Summary: Ref T1812. This still doesn't expose configuration to the user, but adds validation for it.
Test Plan: Added a pile of unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8584
Summary:
Ref T1812. I think integer constants are going to be confusing and error prone for users to interact with. For example, because we use 0-5, adding a second "open" status like "needs verification" without disrupting the existing statuses would require users to define a status with, e.g., constant `6`, but order it between constants `0` and `1`. And if they later remove statuses, they need to avoid reusing existing constants.
Instead, use more manageable string constants like "open", "resolved", etc.
We must migrate three tables:
- The task table itself, to update task status.
- The transaction table, to update historic status changes.
- The saved query table, to update saved queries which specify status sets.
Test Plan:
- Saved a query with complicated status filters.
- Ran migrations.
- Looked at the query, at existing tasks, and at task transactions.
- Forced migrations to run again to verify idempotentcy/safety.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8583
Summary: Ref T1812. Without actually exposing configuration, this moves all status information into a config-like chunk of data which can later be exposed to human editors.
Test Plan:
- Made a bunch of status changes.
- Merged duplicates.
- Created task.
- Viewed feed, transaction record, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8582
Summary: Ref T1812. These are a bit fluff and don't make too much sense to make configurable, at least for now.
Test Plan: Grepped for external callers.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8581
Summary: Ref T1812. This is mega gross but Facts is too far away to do this right for now.
Test Plan:
bleh gross
Looked at reports, saw same data as before.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8580
Summary: Ref T1812. Moves most specialized status handling into `ManiphestTaskStatus`. The only real missing case is reports.
Test Plan:
Browsed most of the affected interfaces. Changed task status:
{F132697}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8579
Summary:
Fixes T4637.
- We already allow you to order by this column but don't have a key on it. Add one.
- Expose UI for querying on ranges.
Test Plan:
- Ran some queries, got reasonable-looking results and no table scans.
Reviewers: btrahan, bigo
Reviewed By: bigo
Subscribers: bigo, epriestley
Maniphest Tasks: T4637
Differential Revision: https://secure.phabricator.com/D8557
Summary: Fixes T912. This was very nearly working, it just needed a little tweaking on the last mile.
Test Plan:
Made updates with no effect, and updates with an effect. Made a no-effect update and posted just the comment part.
{F129037}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T912
Differential Revision: https://secure.phabricator.com/D8543
Summary: Fixes T4403. Supports the "send an email" action in Maniphest.
Test Plan: Wrote a "email duck" rule, then commented on a task and saw "duck" get an email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4403
Differential Revision: https://secure.phabricator.com/D8529
Summary: Ref T4430. This just deploys it on the property lists. (Help on how to do translations better? I tried a more traditional pht('%s, %s, %s, and %d other(s)') but I think the string lookup assumes the %d comes as the second param or something?)
Test Plan: Made a Maniphest Task with a hojillion subscribers and noted the working dialogue. Also made a Pholio Mock with lots of subscribers and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin, chad
Maniphest Tasks: T4430
Differential Revision: https://secure.phabricator.com/D8525
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: Fixes T4553, T4407.
Test Plan: created tasks and they showed up in the proper column. edited task priority and they moved about sensically.
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4553, T4407
Differential Revision: https://secure.phabricator.com/D8420
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: looks better, more useful
Test Plan: looked better, was more useful when I observed my feed
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8394
Summary: This wasn't working. Create a little JS handler and server-side support for returning the Task in the "project card" format.
Test Plan: Edited tasks from the board - they worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D8392
Summary:
If the first non-null entry in the params array is falsey, the request bombs.
Something like {"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-on3xxsnaljmfn36d4b7a"}
Test Plan:
Before:
echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
{"error":"ERR-NO-EFFECT","errorMessage":"ERR-NO-EFFECT: Update has no effect.","response":null}
After:
echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
{"error":null,"errorMessage":null,"response":{"id":"279","phid":"PHID-TASK-lbwcq3pmur2c5fuqqhlx"...
Reviewers: garoevans, epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8391
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:
...this was nice to do for boards, since this diff also starts calling this code in the board move case. The big trick is to use the new expandTransactions hook to expand the subpriority transaction into a priority transaction if pertinent. The other stuff is just about hiding these new subpriority extractions.
...also removes the "edit" UI from the default board since we can't actually edit anything and it thus is buggy.
Ref T4422. Next step is to move board edits into the editor with their own little transaction.
Test Plan: re-orded tasks on a maniphest query, reloaded, and noted re-order stuck.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4422
Differential Revision: https://secure.phabricator.com/D8358
Summary:
Ref T2222. Ref T3886. Ref T418. A few changes:
- CustomField can now index into global search.
- Use CustomField fields instead of older custom fields for Differential global search. (This slightly breaks any custom fields which exist, but they are presumably very rare, and probably do not exist; this break is also very mild.)
- Automatically perform CustomField and Subscribable indexing on applicable object types.
Test Plan: Used `bin/search index` to reindex a bunch of stuff, then searched for it. Debug-dumped abstract documents to inspect them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886, T2222
Differential Revision: https://secure.phabricator.com/D8346
Summary:
When you click the pencil icon in the Maniphest task list, we currently fatal:
Argument 1 passed to PhabricatorCustomFieldList::appendFieldsToForm() must be an instance of AphrontFormView, instance of PHUIFormLayoutView given, called in /core/lib/phabricator/src/applications/maniphest/controller/ManiphestTaskEditController.php on line 576 and defined
This is because we build an `AphrontFormView` noramlly, but a `PHUIFormLayoutView` for dialogs, since they don't take a full form (they render their own form tag).
Instead, always build an `AphrontFormView` and just pull the `PHUIFormLayoutView` out of it when we're ready to put it in a dialog. This means `$form` is always the same type of object, and is generally better and makes more sense.
Test Plan: Clicked pencil edit icon in Maniphest task list.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, carl
Differential Revision: https://secure.phabricator.com/D8324
Summary:
Ref T3886. Broadly, fields break down into two types right now: fields which store data on the object (like `DifferentialTitleField`) and fields which store data in custom field storage.
The former type generally reads data from the object into local storage prior to editing, then writes it back afterward. Currently, this happens in `didSetObject()`.
However, now that we load and set objects from ApplicationTransactionQuery, we'll do this extra read-field-values on view interfaces too. There, it's unnecessary and sometimes throws data-attached exceptions.
Instead, separate these concepts, and do all the read-from-object / read-from-storage in one logical chunk, separate from `didSetObject()`.
Test Plan:
- Edited Differential revision.
- Edited Maniphest task.
- Edited Project.
- Edited user profile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8299
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:
Ref T1812. This cleans up most of the easy hard-coded references to specific statuses:
- The "fixes" language moves into ManiphestTaskStatus.
- Add a method to list open statuses.
- Add a method to test if a status is open.
- Add a method to get default status for new tasks.
Test Plan: Browsed around, lint, grep, created, filtered and updated tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8264
Summary: Fixes T3872. Ref T1812. Ref T3886. Modernize the "closes x as y" string parser, and use all the new parsers instead of the old ones.
Test Plan: Made a commit full of a pile of these trigger strings, then used `scripts/repository/reparse.php --message` to reparse it. Verified that parses came back as expected using a bunch of `var_dump()`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1812, T3872, T3886
Differential Revision: https://secure.phabricator.com/D8263
Summary:
Ref T4420. Tokenizers currently operate in "preload" or "ondemand" modes. In the former mode, which is default, they'll try to load the entire result list when a page loads.
The theory here was that this would slightly improve the experience for small installs, and once they got big enough they could switch to "ondemand". In practice, several issues have arisen:
- We generally don't have a good mechanism for telling installs that they should tweak perf config -- `metamta.send-immediately` is the canonical example here. Some large installs are probably affected negatively by not knowing to change this setting, and having settings like this is generally annoying.
- We have way way too much config now.
- With the advent of ApplicationSearch, pages like Maniphest make many redundant loads to prefill sources like projects. Most of the time, this data is not used. It's far simpler to switch everything to ondemand than try to deal with this, and dealing with this would mean creating two very complex divergent pathways in the codebase for a mostly theoretical performance benefit which only impacts tiny installs.
- We've been using `tokenizer.ondemand` forever on `secure.phabricator.com` since we have many thousands of user accounts, and it doesn't seem sluggish and works properly.
Removing this config is an easy fix which makes the codebase simpler.
I've retained the ability to use preloaded sources, since they may make sense in some cases (in at least one case -- task priorities -- adding a static source pathway might make sense), and they're part of Javelin itself. However, the code will no longer ever go down that pathway.
Test Plan: Used `secure.phabricator.com` for years with this setting enabled.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D8232
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: Fixes T4429. Shows and allows you to click through on board links when a task appears on workboards.
Test Plan:
{F112837}
Clicked the links.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4429
Differential Revision: https://secure.phabricator.com/D8226
Summary: Simplified header, added Workboard button and icon, moved Maniphest actions to "Open Tasks" Object Box. Reduced actions by 3.
Test Plan: Test a number of project pages, looks better, cleaner.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8219
Summary: Ref T3574. Since this list just clips in a totally reasonable way on mobile and we got another user request for it, let's bump this to 4 for now and we can refine mobile later.
Test Plan: Looked at list on desktop; saw 4 tags before truncation. Looked at list on mobile, saw reasonable clipping behavior which didn't mar usability.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3574
Differential Revision: https://secure.phabricator.com/D8213
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: Lets you type any mailable into the "Subscribers" field.
Test Plan: Typed a list, got relevant tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8161
Summary: Fixes T4378. I just removed the `<em>` since this element is unusual and it's not convenient to switch it to translatable remarkup.
Test Plan: stared at it real good
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chrisbolt
Maniphest Tasks: T4378
Differential Revision: https://secure.phabricator.com/D8155
Summary: Ref T4365. Two diffs from now, I'm changing the UI a bit to let you search for closed and unowned documents more explcitly. To support this in ElasticSearch and more easily in MySQL search, make these explicit, positive relationships.
Test Plan: `bin/search index --all`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8122
Summary:
Ref T4365. Primary search currently uses `PhabricatorSearchQuery` for storage, which is pretty much the same as `PhabricatorSavedQuery`, except that it's old and not used anywhere else anymore.
Maniphest used to also use this table, but no longer does after Septmeber, 2013. We need to retain the class so the migration can work.
This introduces `PhabricatorSearchApplicationSearchEngine` and `PhabricatorSearchDocumentQuery`, but they're both stubs that I just needed for technical reasons and/or to pass lint. The next couple patches will move logic into them and use ApplicationSearch properly.
Test Plan:
- Searched for stuff.
- Searched for stuff with filters.
- Searched for fulltext in Maniphest.
- Grepped for `PhabricatorSearchQuery`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8120
Summary: This adds the app icons, cleans up css Ref T3623
Test Plan: see new icons in dropdown menu
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8124
Summary: Ref T3623. These are obsoleted by the global quick-create menu, so we can simplify the app launcher.
Test Plan: Looked at app launcher, grepped for everything.
Reviewers: chad
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8104
Summary: Ref T3623. Also dropped "New" from everything, since the "Create a new:" label contextualizes that.
Test Plan: Clicked all the stuff in the menu.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8103
Summary: See IRC. This will be obsoleted by ApplicationTransactions eventually, but fix issues with stauts change previews for now. Specifically, if you select "Reopen Task", it incorrectly previews as "x created this task" because the old state is not set correctly.
Test Plan: Selected "Reopen Task", saw a preview with the correct language.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8094
Summary:
Ref T3623. This is like a pre-v0, in that it doesn't have a dropdown yet.
Clicking the button takes you to a page which can serve as a right click / mobile / edit target in the long run, but is obviously not great for desktop use. I'll add the dropdown in the next iteration.
Test Plan: {F105631}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8088
