Summary:
Ref T7664. Currently, when spreading subpriorities we may recurse deeply in certain conditions. Make sure we never recurse more than one level.
To try to mitigate issues with floating point precision, be more aggressive about selecting tasks to reorder.
I wasn't really able to come up with a realistic test case here, and the test cases I found which sort of approximated the behavior took way too long to generate data to actually commit.
This approach is inherently somewhat fragile but hopefully this is approximately good enough. We don't have a durable storage engine which can meaningfully represent double-linked lists right now.
Test Plan:
- Wrote some (slow) tests which kind of approximately hit the issue.
- Verified they maxed out at stack depth 2 after the change.
- Unit tests still pass.
- Dragged some tasks around.
- Couldn't come up with any pathological issues here by thinking about it?
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7664
Differential Revision: https://secure.phabricator.com/D12511
Summary:
See M1433. Fixes T7266. Fixes T4475. Ref T7314.
Future work/notes/etc:
- Write the User Guide (see TODO).
- This might needs some design tweaks -- I think it's functionally almost-equivalent to the mock, but the UI isn't quite the same.
- (Mobile design is a touch off-looking I think?)
- When you use a custom query, the duplicate "magnifying glass" icons are a little weird. Maybe change one or the other.
- Maybe worth adding an "Open Documents in Current Application" option? Planning to wait for feedback on that.
- Need a Quicksand integration to change the current application at some point.
- Searching in "Current Application" from, e.g., the 404 page just searches all documents. Current plan is to just document this behavior, since the icon is a pretty good callout and it seems plausible that this is intuitive enough that users won't have a hard time with it.
Test Plan:
New dropdown:
{F379150}
Device-ish:
{F379151}
Normal search (current application, from maniphest, selects tasks):
{F379153}
Application search from non-application:
{F379154}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: johnny-bit, epriestley
Maniphest Tasks: T7266, T7314, T4475
Differential Revision: https://secure.phabricator.com/D12509
Summary: Ref T4100. This integration into the "Browse" dialog is probably a little more heavy-handed than we should shoot for.
Test Plan:
{F377131}
{F377132}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12482
Summary: Ref T4100. Support viewer(), members(), and add a new none().
Test Plan:
- Used all new functions.
- Batch edited tasks with unassign action.
- Saved a query from master, upgrade it to this patch, checkbox migrated cleanly into a "no one" token.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12470
Summary: Ref T4100. Let datasources specify a more meaningful title than the class name.
Test Plan: Browsed some sources.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12469
Summary:
Ref T4100. I can simplify the logic a bit here by moving some rendering into the datasources, but a few TokenizerControls currently don't have datasources.
Require datasources and always provide datasources.
Test Plan:
- Used previously-datasourceless controls (e.g., "Add Reviewers").
- Used normal controls.
- Manually verified that no other controls are missing datasources.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12456
Summary:
Ref T4100. Ref T5595. This allows PolicyAwareQuery to write all the logic for AND, OR, NOT, and NULL (i.e., "not in any projects") queries against any edge type.
It accepts an edge type and a list of constraints (which are basically just operator-value pairs, like `<NOT, PHID-X-Y>`, meaning the results must not have an edge connecting them to `PHID-X-Y`).
This doesn't actually do anything yet; see future diffs.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12455
Summary:
Ref T5523. Adds a new workflow to make some kinds of bulk workboard operations easier.
New dropdown action:
{F376848}
This brings you into the existing bulk edit flow:
{F376849}
When you save an edit, you're taken back to the board:
{F376850}
If you try to edit a column with nothing in it, you get an error:
{F376851}
Note that the selected workboard filter is applied before choosing tasks, so if your filter is set to "open tasks" we only batch edit the open (i.e., currently visible) tasks in the column. I think this is more powerful (it lets you use filtering to select task subsets) but might not be completely obvious in all cases (although I do think it's more obvious than the alternative rule -- just an issue of neither rule being completely obvious).
Test Plan:
- Batch edited tasks in a column.
- Used "Batch Edit Tasks..." to move tasks to a different workboard by removing + adding a project.
- Batch edited a column with filtered-out tasks, verified only visible tasks were edited.
- Batch edited a column with no visible tasks, received error.
- Used the batch editor normally (Maniphest -> Maniphest, no boards).
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: johnny-bit, cburroughs, epriestley
Projects: #prioritized
Maniphest Tasks: T5523
Differential Revision: https://secure.phabricator.com/D12475
Summary: Ref T5750. This makes browse work for all of the dynamic tokenizers in Herald, Policies, batch editor, etc.
Test Plan: Used tokenizers in Herald, Policies, Batch editor.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12442
Summary:
Ref T5750. These are a pain to modernize and most don't matter, so cheat:
- Mark a bunch nonbrowsable, including some that probably should be browsable but which I don't want to deal with for now.
- For static datasources, add an easy server-side filter (this isn't really cheating, and is appropriate for the status/priority/application datasources).
- Make composite sources browsable if their components are browsable.
Test Plan:
- Tried to browse an unbrowsable source, got a 404.
- Browsed a composite source.
- Browsed static sources (priority/status/applications).
- Browsed normal sources (people/projects).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12438
Summary:
Ref T7803. Prior to this change sequence, Query classes conflated paging values (the actual thing that goes in a "x > 3" clause) with cursor values (arbitrary identifiers which track where the user is in a result list).
Although the two can sometimes be the same, the vast majority of implementations are simpler and better when object IDs are used as cursors and paging values are derived from them.
The new stuff handles this in a consistent way, so we're free to separate getPagingValue() from paging. The new method is essentially getResultCursor().
This also implements getPageCursors(), which allows queries to return directional cursors. The inability to do this was a practical limitation blocking the implementation of T7803.
Test Plan:
- Browsed a bunch of results and paged through queries.
- Grepped for removed methods.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12383
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary:
Ref T7803. The ApplicationSearch integration is still a little rough here, but it seems to have the correct behavior.
The rest of this is now at least relatively sane, cohesive, and properly behaved.
Test Plan:
- Used all grouping and ordering queries in Maniphest. Pagingated results.
- Used custom field ordering in Maniphest. Paginated results.
- Paginated through the `null` section of "Assigned" and "Projects" group-by queries. Pagingation now works correctly (it does not work at HEAD).
- Ran unit tests covering priority changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12372
Summary:
Ref T7803. Removes some getReversePaging().
This also fixes `null` column handling, by adding an explicit `'null'` key with possible values "head" (put NULL before other values) or "tail" (put NULL after other values).
Maniphest has some glitchiness in paging through NULLs right now, but I believe it's all pre-existing and will be resolved when it fully converts. Diffusion is fully converted and pages through NULL correctly.
Test Plan:
- Failed to identify any reason for ChangesetQuery to reverse paging.
- Paged thorugh Diffusion.
- Paged through Maniphest.
- Maniphest has some issues when paging inside a NULL section, but these issues are preexisting and will be resolved later in this change sequence.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12371
Summary:
Ref T7803. Some Query subclasses implement getPagingColumn() in a trivial way, usually to provide a table alias.
Formalize the concept of a primary table alias, and remove obsoleted getPagingColumn() implementations.
Test Plan: Issued affected queries.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12356
Summary: Ref T7803. Reduce the amount of code we're trusting to build SQL queries.
Test Plan:
- Paged through results in Maniphest, Differential and Diffusion.
- Some of the NULLable groups in Maniphest are a bit funky but this was preexisting.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12353
Summary: Fixes T7778. This was likely caused by removing an `array_filter()` somewhere in the course of T7731, but I'd rather have the code be more correct.
Test Plan:
Sent mail on a task with no owner.
- Before patch: unknown recipient.
- After patch: expected recipients.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T7778
Differential Revision: https://secure.phabricator.com/D12320
Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.
In particular, this is stronger than these downgrades:
- Downgrades due to "self actions";
- downgrades due to "mail tags".
Test Plan:
- Wrote various Herald rules with "Send me an email" rules.
- Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
- Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12300
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.
- Reduce code duplication between existing receivers.
- Expose receiving objects in help menus where appropriate.
- Connect some "TODO" receivers.
Test Plan:
- Sent mail to every supported object type.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12249
Summary: Ref T7199. This makes the page look less janky and provides more context about how mail commands work and how to use them.
Test Plan: {F355959}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12245
Summary:
Ref T7199. Convert the single help menu item into a dropdown and allow applications to list multiple items there.
When an application has mail command objects, link them in the menu.
Test Plan:
{F355925}
{F355926}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12244
Summary: Ref T7199. This needs some polish and isn't reachable from the UI, but technically has all of the information.
Test Plan:
{F355899}
{F355900}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12241
Summary:
Ref T7199. This fully modularizes mail command handling in Maniphest.
I had to add a couple of minor not-totally-solid-feeling tricks to deal with the "create" case, but they feel not-too-bad, and a million times better than what came before.
Test Plan: Used all commands with `receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12240
Summary:
Ref T7199. Two notable changes:
- Process multiple commands.
- Process commands when creating //or// updating a task.
And generally clean things up a bit.
Test Plan:
- Used `receive-test` to execute all commands for new tasks.
- Used `receive-test` to execute all commands for existing tasks.
- Used a combination of commands to produce varied effects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12234
Summary:
Ref T7199. In the vein of D12231, these options were a bad idea.
- They once served a very narrow, Facebook-specific need (see T1992), except even Facebook only used the Differential setting AFAIK.
- Outside of that special case, they are unused and essentially unusable (generally speaking, they do not meaningfully implement anything modular or replaceable).
- I have no knowledge of any install ever changing these settings, and can imagine no reason why they would.
Moving forward:
- If they really need to, they can fork locally and chagne one line.
- I expect "!actions" to make mail at least somewhat more modular soon, anyway.
- Any derived handlers would break after T7199 and need to be rewritten anyway, so this is just taking advantage of a BC break to do cleanup.
Test Plan:
- Grepped for removed configuration.
- Sent some mail from applications, verified the reply handlers set proper reply addresses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12233
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.
- There is zero reason to ever set these to different things.
- Unsurprisingly, I don't know of any install which has them set to different things.
Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.
We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.
Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12231
Summary:
Ref T7199. This prepares for an exciting new world of more powerful "!action" commands. In particular:
- We parse multiple commands per mail.
- We parse command arguments (these are currently not used).
- We parse commands at the beginning or end of mail.
Additionally:
- Do a quick modernization pass on all handlers.
- Break legacy compatibility with really hacky Facebook stuff (see T1992). They've theoretically been on notice for a year and a half, and their setup relies on calling very old reply handler APIs directly.
- Some of these handlers had some copy/paste fluff.
- The Releeph handler is unreachable, but fix it //in theory//.
Test Plan:
- Sent mail to a file; used "!unsubscribe".
- Sent mail to a legalpad document; used "!unsubscribe".
- Sent mail to a task; used various "!close", "!claim", "!assign", etc.
- Sent mail to a paste.
- Sent mail to a revision; used various "!reject", "!claim", etc.
- Tried to send mail to a pull request but it's not actually reachable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12230
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.
Test Plan:
- Sent a new message; used "To".
- I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
- OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^
Did not test:
- Lint is nontrivial to test locally, I'll test it in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T7689
Differential Revision: https://secure.phabricator.com/D12224
Summary:
Ref T7689. This gives HandleLists `renderList()` and `renderHandle()` methods, which return views that can perform just-in-time data fetching and generally look and feel like other rendering code, instead of being odd pseudo-functional methods on `Controller`.
Also converts callsites on the Maniphest detail page to use these methods.
Next changes will wipe out more of the callsites.
Test Plan:
- Viewed Maniphest detail page with many relevant handles.
- Created a new subtask.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12205
Summary:
Fixes T7664. When there are a large number of tasks (400+) with the same subpriority (which can happen if the subpriority features are rarely used), it may take more than 30 seconds to rebalance them.
Make the algorithm more aggressive about rebalancing homogenous blocks of tasks.
This may need to get even fancier, but I'd guess it can process blocks 1-2 orders of magnitude larger, which should be ~all installs.
(If someone still hits issues with this, I'll make it fancier.)
Once a block is rebalanced, it doesn't need to be rebalanced again (at least, not as a whole block) so we basically just need to get over the initial hurdle here and then we're good.
In the worst case, we can provide `bin/maniphest rebalance` or similar and do the rebalance step offline.
And, in any case, we have more test coverage here now.
Test Plan:
- Existing tests.
- New tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7664
Differential Revision: https://secure.phabricator.com/D12166
Summary:
Fixes T7563. Fixes T5201. Reframe this as two separate operations:
- Move before or after a task.
- Move to the beginning or end of a priority.
Then:
- Make all the order queries unambiguous and properly reversible, with an explicit `id` order.
- Just reuse `ManiphestTask` to get results in the correct order.
- Simplify the actual transaction apply logic.
- Detect and recover from cases where tasks have identical or similar subpriorities.
Test Plan:
- Wrote and executed unit tests.
- Dragged and dropped tasks within priorities and between priorities in the main Maniphest view.
- Dragged and dropped tasks within priorities in the workboard view, when ordered by priority.
- Also poked at the "natural" order, but that shouldn't be affected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T5201, T7563
Differential Revision: https://secure.phabricator.com/D12121
Summary: Renames the method in PHUIObjectBoxView to match the new PHUIInfoView class.
Test Plan: grepped codebase. Went to Calendar and tried a new status.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12005
Summary: Fixes T7392. I kind of stink at SQL so my approach here was to "start over" conceptually and this way makes the most sense to me - we basically do one join on the dependency table and then a second join back from the dependency table to the main task table. In the where clause we filter the resulting rows, first checking the data from dependency join for existence as appropros and then checking the second join for main task table for the proper "open" task values.
Test Plan: made a task X be blocked by task Y. closed task y. search for "not blocked" tasks and saw task X.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7392
Differential Revision: https://secure.phabricator.com/D11962
Summary: Fixes T7434. We need to LEFT JOIN, not JOIN here, because we still want result rows where the value is `null`.
Test Plan: Issued blocked/not-blocked + blocking/not-blocking queries, got results in all cases.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7434
Differential Revision: https://secure.phabricator.com/D11939
Summary: Adding better CSS and set correct tag and examples.
Test Plan: Test UIExamples, creating and click on similar task, empty task in Maniphest.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7423
Differential Revision: https://secure.phabricator.com/D11932
Summary: Fixes T7421. Now that we join the task table again to ignore //closed// blockers, all the column names are ambiguous. Make them unambiguous.
Test Plan: Issued some searches with various different parameters.
Reviewers: btrahan, joshuaspence, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7421
Differential Revision: https://secure.phabricator.com/D11922
Summary: Removes AphrontContext bar and uses PHUIInfoView instead. This also attaches to the ObjectBox instead for cleaner UI. Also moved phui-error-view.css which was missed.
Test Plan: Test creating a subtask or a new task, see updated info bar and action buttons.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11920
Summary: Since this element isn't strictly about errors, re-label as info view instead.
Test Plan: Grepped for all callsites, tested UIExamples and a few other random pages.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11867
Summary: Fixes T7392. When filtering blocked/blocking Maniphest tasks, don't consider closed tasks.
Test Plan:
# Created `T1` and `T2` with `T2 depends on T1`.
# Marked `T1` as resolved.
# Searched for tasks "blocked by other tasks" and noted that `T2` wasn't in the result set.
Reviewers: btrahan, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7392
Differential Revision: https://secure.phabricator.com/D11911
Summary: Fixes T7367
Test Plan: I guess noone every used this? Click on mobile menu, get not a 404.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7367
Differential Revision: https://secure.phabricator.com/D11880
Summary:
Ref T7185. These settings shouldn't be unlocked anywhere. Specifically:
- `reply-handler`: These are on the way out.
- `reply-handler-domain`: Also hopefully on the way out; locked because a compromised administrator account can redirect replies.
- `phabricator.cookie-prefix`: Not dangerous per se, but an admin could have a hard time fixing this if they changed it by accident since their session would become invalid immediately.
Test Plan: Browsed Config.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7185
Differential Revision: https://secure.phabricator.com/D11764
Summary: Fixes T7088. Mainly this updates the documentation but I also snuck in tweaking how the domain reply handler is built. This does two main things -- makes the behavior consistent as some applications who didn't override this behavior would send out emails with reply tos AND makes it easier for us to deprecate the custom domain thing on a per application basis, which is just silly. On that note, the main documentation doesn't get into how this can be overridden, though I left in that mini blurb on the config setting itself. We could deprecate this harder and LOCK things if you want as well.
Test Plan: read docs, looked good. reasoned through re-factor
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7088
Differential Revision: https://secure.phabricator.com/D11725
Summary: I //think// Maniphest has switched to real edges now.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11716
Summary: Adds core and apps grouping to configuration options, makes it somewhat easier to browse config options.
Test Plan: Set each option, review list. Breakdown is nearly 50/50 apps/core.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11722
Summary: Fixes T7118. This does the basic "filter the list" thing, though it ends up being a little manual since I guess this hasn't come up before? There is also potential weird behavior if the user was using an app and lost access to it - they will have nothing selected on edit - but I think this is actually correct behavior in this circumstance.
Test Plan:
used a user who couldn't get access to the "quick create" apps and noted that the dropdown list on dashboard panel create was missing the expected engines
ran `arc unit --everything` to verify abstract method implemented everywhere
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7118
Differential Revision: https://secure.phabricator.com/D11687
Summary: Fixes T7164. Adds some details about how the statuses will show up in the UI.
Test Plan: Read the text
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T7164
Differential Revision: https://secure.phabricator.com/D11686
Summary: Fixes T7094 (last of many revisions). Its important to do this filtering ASAP so that users can't deduce the identify of an unknown / invisible project.
Test Plan: executed a query for tasks in project foo using user bar. using user foo, lock user bar out of project foo. reissued the query and saw "no data" as well as "restricted project" in the project typeahead.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11660
Summary: Ref T7094. This is a bit involved and should be tackled as a separate effort. The good news is policy still saves the day here but (back to the bad news) its a bad user experience.
Test Plan: NA, just a comment
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11658
Summary: Revamps Profile to be like Projects, a mini portal and side nav with icons.
Test Plan: Viewed my own profile, as well as others. Test seeing my commits, tasks, diffs, and upcoming events. Checked mobile navigation.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11547
Summary: This sets an icon for each config, makes it easier to scan.
Test Plan:
Reload Config page, see all new icons
{F281089}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11619
Summary: Clean up the error view styling.
Test Plan:
Tested as many as I could find, built additional tests in UIExamples
{F280452}
{F280453}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11605
Summary: Fixes T3404 (post D11565), fixes T5952. This infrastructure has been getting deployed against Maniphest and its time to get these other two applications going on it.
Test Plan: created an email address for paste and used `./bin/mail receive-test` ; a paste was successfully created
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952, T3404
Differential Revision: https://secure.phabricator.com/D11570
Summary: due to typehints, passing null is going to barf here. Ref D11564, ref T5039.
Test Plan: made an edit to a task from the web ui and it didnt fatal
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11571
Summary:
Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???"
Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective.
Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11564
Summary: Ref T5952. This adds support for a "default author" and deploys it on Maniphest.
Test Plan: used augmented (by this diff) bin/mail receive-test to test creation via an application email with a default author configured and no author specified. a task was created with the author as the default author i configured.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952
Differential Revision: https://secure.phabricator.com/D11446
Summary:
Ref T7055. Apparently we just never had one? I feel like I'm crazy. But I can't find any trace in the logs.
I'm actually not 100% sold on this being better because it's a color glyph on OSX and those feel a little out of place / tacky to me compared to the black-and-white ones. So I'd be fine with just leaving it off, too. Clearly not important if no one noticed it until I caught it in T7055.
Test Plan: {F276917}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7055
Differential Revision: https://secure.phabricator.com/D11524
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary: Ref T5952, T3404. This lays the basic plumbing for how this will work, all the way to deploying on Maniphest. Aside from what is mentioned on T5952, I think page(s) on editing application emails could use a little more helpful text about what's going on, similar to how the config page that's getting deprecated works.
Test Plan: ran migration and noted my create email address migrated successfully. used bin/mail to make a task. added another email and used bin/mail to make a task. deleted an email. edited an email. invoked various error states and they all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404, T5952
Differential Revision: https://secure.phabricator.com/D11418
Summary: In Maniphest, we provide an additional caption shortcut if you can create projects, which has no use if you cant. Fixes T6969
Test Plan: Check page with and without a user's capability.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6969
Differential Revision: https://secure.phabricator.com/D11390
Summary:
Fixes T5352. This is very useful for finding things that should be easy to do ("not blocked") as well as things that are important to do ("blocking"). I have wanted to check out the latter case in our installation, though no promises on what I would end up actually doing from that search result list. =D
I also think supporting something like T6638 is reasonable but the UI seems trickier to me; its some sort of task tokenizer, which I don't think we've done before?
Test Plan: toggled various search options and got reasonable results. When i clicked conflicting things like "blocking" and "not blocking" verified it was like I had not clicked anything at all.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5352
Differential Revision: https://secure.phabricator.com/D11306
Summary: Adds more user friendly copy to the result list
Test Plan: Test on a project with and without tasks.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11352
Summary: Ref T5752, moves mobile action menus to the object box instead of crumbs.
Test Plan: View action menus at tablet, desktop, and mobile break points. Verify clicking buttons works as expected opening menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5752
Differential Revision: https://secure.phabricator.com/D11340
Summary:
Fixes T6932. Fixes some issues from D11303.
- When claiming a task, if it was previously unassigned, we would try to CC `null`.
- When claiming a task, if the current owner was already CC'd, the viewer would incorrectly be warned about all subscribers being CC'd.
Test Plan:
- Claimed an unclaimed task.
- Claimed a task with owner CC'd.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T6932
Differential Revision: https://secure.phabricator.com/D11336
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
