Summary:
Ref T12928. The index doesn't work for these, so show the user that there's a problem and drop the terms.
This doesn't fix the problem, but makes the behavior more clear.
Test Plan:
{F5053703}
{F5053704}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12928
Differential Revision: https://secure.phabricator.com/D18254
Summary: Cleans up a bunch of Differential odd/special colors. Adds some basic "highlight" colors instead of pure yellow.
Test Plan: Test each color change in normal and dark modes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18239
Summary: Gives some strength to name (needed to over-ride new images) and new create copy.
Test Plan: Create a new mock, see proper story in feed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18246
Summary: Ref T10769. See PHI8. We have an unconditional logging write which we can skip in read-only mode.
Test Plan:
- Put Phabricator in read-only mode with `cluster.read-only`.
- Called `conduit.ping` via web UI.
- Before: write-on-read-only exception.
- After: good result.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T10769
Differential Revision: https://secure.phabricator.com/D18233
Summary: Mostly this is an exercise to clean up our CSS and Celerity processor by making sure all important color decisions are generatable. It's somewhat resonable to use if you don't review code. Posting it up here mostly so I don't lose the work.
Test Plan: Visit lots and lots of pages with dark mode on and off.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18227
Summary: This spelling can definitely feel a little overplayed at times, but I still think it's a gold standard in spellings of "capabilities".
Test Plan: Felt old and uncool.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18215
Summary: Just some cleanup. Make sure action-bar has consistent space if locate is there or not, hide tabs if repository has no content. Use clone or checkout language depending on SCM. Fixes T12915.
Test Plan:
Test git, hg, svn blank states.
{F5042707}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12915
Differential Revision: https://secure.phabricator.com/D18208
Summary: This moves the clone details on the Repository Home to a button / dialog. Functionally this is to pull content on the page way up, while giving full space to all the clone options. I think we can build this into some FancyJS if needed, but this seems to clean ui the UI dramatically with little overhead. I don't want to attempt the JS dropdown unless we're sure that's the best path (it exposes the most common URI by default, saving a click).
Test Plan: Tested hg, svn, git repositories and the raw URL page. Test close button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18203
Summary: First pass at providing a skeleton framework for laying out basic items in a left/right view. Will likely add some mobile-responsive options.
Test Plan: UIExamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18200
Summary: Better validation for setting a default image in project.icon
Test Plan: Test adding `"0"` and `""` as image options in project.icon, see error.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18197
Summary: Adds a page to view all and their path in UIExamples.
Test Plan: Review page in UIExamples, hover over image for path.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18196
Summary: This moves actions into the Diffusion main header, removes the locate file box, and widens description and cloning details. Projects are not currently in this layout, but will follow up in another diff. Trying to keep these changes small and iterative.
Test Plan:
Locate some files, test actions dropdown, repository with and without description. Also tablet, mobile layouts.
{F5040026}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18193
Summary: Builds out a map for icon->image in Projects, selects the icon's image as the default project image if there is no custom image chosen by the user.
Test Plan: Select various icons, see image change. Test choose picture, pick a new image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18174
Summary: Cleans up colors, removes commit hash and links the text instead. Also unsure how valuable "lint" column is here, but left it. I'd maybe like to understand that workflow since it just seems like clutter overall. Also Fixes T12905
Test Plan:
Review Phabricator, hg, and a few other test repositories locally. Holler if anything here seems bad, but this feels easier to read and use to me.
{F5038425}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12905
Differential Revision: https://secure.phabricator.com/D18189
Summary: More pretty images.
Test Plan: Set a robot as image for security project. So pretty.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18191
Summary:
Ref T12681. We need this to update the "paid until" window on support pacts.
(Instance billing doesn't use this because everything just checks if you have unpaid invoices, nothing actually happens when you pay them.)
Test Plan: See D18187.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12681
Differential Revision: https://secure.phabricator.com/D18188
Summary: Ref T12900. We implement one rule, but tell users a different (older) rule. See T12900 for discussion and history.
Test Plan:
- Verified draft/archived posts can't be seen by users who don't have permission to edit the blog.
- Drafted, archived, and published posts and read the related text.
- Looked through the changes I dug up in T12900#228748 for other strings I might have missed.
{F5033860}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12900
Differential Revision: https://secure.phabricator.com/D18182
Summary:
Adds a responsive tab bar navigation to Diffusion. Working through the new design here in pieces, so keep in mind M1477 is the target. Notably:
- Removes "branches" and "tags" from RevisionView, now on tabs
- Keeps "browse", "history", "readme" on RevisionView
- Adds tabs for all main views, including Graph... unless how that feels, so let me know.
Test Plan: Browse all pages, desktop and mobile. Test hg, svn, git repositories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18161
Summary: Fixes T12894. See that task for discussion.
Test Plan:
- Created repositories `abcdef`, then `abcdef-a` through `abcdef-f`.
- Before patch, awkward sort order.
- After patch, query for `abcdef` hits `abcdef` first.
- See T12894 for details and screenshots.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12894
Differential Revision: https://secure.phabricator.com/D18179
Summary: The main change here is moving (compare, search, history) into buttons in the header bar on all browse views. This allows Directory Browsing to be full width, since there is no other curtain information. File, Image, LFS, Binary all stay in TwoColumn layouts with the same buttons in the header.
Test Plan: Test viewing a directory, file, image, binary file, readme, and fake a gitlfs.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17766
Summary: Fixes T12883. The task seems correct to me and I think this is a copy/paste mistake that probably blames to me.
Test Plan: Fiddled these numbers, viewed a build in Harbormaster, saw the adjusted time.
Reviewers: chad, amckinley
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12883
Differential Revision: https://secure.phabricator.com/D18177
Summary:
Fixes T12884. In cases other than this UI, applications access URIs through the Repository they're part of. This means that applications interact with URIs which have gone through the correction/adjustment logic in `PhabricatorRepository->attachURIs()`, which fixes up "builtin" URIs to have the right values based on configuration.
In this case (and, as far as I can tell, only this case) we load the URI directly //and// act on its properties which depend on configuration and repository state.
This can mean we're using a different view of the URI than we should be.
To fix this: after loading the URI, reload it through the repository so the relevant adjustments are applied.
I think this is the most reasonable fix. We could try to make `RepositoryURIQuery` somehow enforce this, but the cost of this error is small (mild confusion about display state), the other things which do direct loads don't depend on this state (editing), and everything else loads via a repository and is likely to continue doing that forever.
Test Plan: {F5026633}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12884
Differential Revision: https://secure.phabricator.com/D18176
Summary:
When users use the web UI to enter text like "Reviewers: x" into the "Summary" or "Test Plan", we can end up with an ambiguous commit message.
Some time ago we added a warning about this to the "Summary" field, and //attempted// to add it to the "Test Plan" field, but it actually gets called from the wrong place.
Remove the code from the wrong place (no callers, not reachable) and put it in the right place.
This fixes an issue where users could edit a test plan from the web UI to add the text "Tests: ..." and cause ambiguities on a later "arc diff --edit".
Test Plan: {F5026603}
Reviewers: chad, amckinley
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18175
Summary: Ref T12845. Converts the cluster and project config options to the new stuff; this is mostly just shifting boilerplate around.
Test Plan: Edited, deleted, and mangled these options from the web UI and CLI.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18166
Summary:
Fixes T12870. Ref T12845.
Technically, this addresses the core issue in T12845 too, but I'm going to convert the rest of the `custom:...` types before closing that.
In particular, for T12870:
- Validates that keywords are unique across priorities.
- Fixes missing newline in documentation.
- Updates documentation to note that keywords are now mandatory and must be unique across priorities.
Test Plan: Edited, deleted and mangled all the Maniphest custom options (priorities, statuses, points, subtypes).
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12870, T12845
Differential Revision: https://secure.phabricator.com/D18165
Summary:
Ref T12845. This is the last of the hard-coded types.
These are mostly used for values which users don't directly edit, so it's largely OK that they aren't carefully validated. In some cases, it would be good to introduce a separate validator eventually.
Test Plan: Edited, deleted and mangled these values via the web UI and CLI.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18164
Summary: Ref T12845. This move 'set' options (a set of values).
Test Plan: Set, deleted and mangled 'set' options from CLI and web UI.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18160
Summary: Ref T12845. These options prompt the user to select from among concrete subclasses of some base class.
Test Plan: Set, deleted and mangled these values from the web UI and CLI.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18159
Summary: Ref T12845. This updates the "list<string>" and "list<regex>" options.
Test Plan: Set, deleted, and mangled options of these types from the web UI and CLI.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18157
Summary: Ref T12845. This moves the "enum" and "string" types to the new code.
Test Plan: Set, deleted, and tried to set invalid values for various enum and string config values (header color, mail prefixes, etc) from the CLI and web.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18156
Summary:
Ref T12845. Config options are "modular", but the modularity is very old, half-implemented, and doesn't use modern patterns.
Half the types are hard-coded, while half the types are semi-modular but in a weird hacky way where you prefix the type with `custom:...`.
The actual API is also weird and requires types to return a lot of `array($stuff, $thing, $other_thing, $more_stuff)` sorts of tuples.
Instead:
- Add a new replacement layer which uses modern modularity patterns and overrides the older stuff if available, so we can migrate things one at a time.
- New layer uses a more modern API -- no `return array($thing, $other_thing, ...)`, and more modern building blocks (like AphrontHTTPParameterType).
- New layer allows custom types to be deleted, which will ultimately let us deal with T12845.
Then, convert the `'int'` type to use the new layer.
Test Plan:
- Set, edited, tried-to-change-in-an-invalid-way, and deleted an `'int'` option from the web UI.
- Same from the CLI.
- Edited `config.json` to have an invalid value, verified that the error was detected and config was repaired.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18155
Summary: Updates the builtin images, leaves the old choose... icons for now. I'd like to automate this based on icon when creating a project.
Test Plan: Visit edit picture page, pick a few. Purge cache, see new default image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18162
Summary: This was accidentally caught in the crossfire in D18150. This is stable enough to formalize instead of adding with an event hook.
Test Plan: Looked at a candidate revision, saw "Land Revision" appear in UI again.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18154
Summary:
Ref T12871. This replaces a dead end UI (user totally locked out) with one where the menu is still available, if the default menu item is one which generates a policy exception (e.g., because users can't see the dashboard).
Really, we should do better than this and not select this item as the default item if the viewer can't see it, but there is currently no reliable way to test for "can the viewer see this item?" so this is a more involved change. I'm thinking we get this minor improvement into the release, then pursue a more detailed fix afterward.
Test Plan:
- Added a dashboard as the top item in the global menu.
- Changed the dashboard to be visible to only user B.
- Viewed Home as user A.
- Before patch: entire page is a policy exception dialog.
- After patch, things are better:
{F5014179}
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12871
Differential Revision: https://secure.phabricator.com/D18152
Summary:
Fixes T12851.
This should fix the error I'm seeing, which is:
* `Argument 1 passed to array_fuse() must be of the type array, boolean given`
There may be a better way to patch this up than overriding the getValue() method,
however.
Test Plan:
- Changed the default "Tags" filter to specify `true` instead of `array('self')`, then viewed that filter in the UI.
- Before patch: fatal.
- After patch: page loads. Note that `true` is not interpreted as `array('self')`, but the page isn't broken, which is a big improvement.
Reviewers: #blessed_reviewers, 20after4, chad, amckinley
Reviewed By: #blessed_reviewers, amckinley
Subscribers: Korvin
Maniphest Tasks: T12851
Differential Revision: https://secure.phabricator.com/D18132
Summary:
Fixes T12869. This is a very old, pre-Drydock chunk of code from D7486 and some followups.
It does three things:
- "Land to Hosted Git": Obsoleted by Drydock, has been commented out in HEAD for a very long time with no complaints. Disabled by D8719 in 2014.
- "Land to Hosted Mercurial": Could be obsoleted by Drydock with a fairly small amount of work, but currently has no replacement. Unclear if this sees any real use. Not actually disabled at HEAD.
- "Land to GitHub": Use GitHub OAuth credentials to land to GitHub. This is sort of theoretically useful and has no analog today. Disabled by D13022 in 2015.
This stuff was largely disabled a long time ago and we haven't seen users hitting issues with it. This could all be moved to an extension today if anyone still relies on it.
Test Plan: Grepped for removed classes, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12869
Differential Revision: https://secure.phabricator.com/D18150
Summary: Ref T12859. This is an older command with a lot of hard-coded flags. Modularize cache purging in a modern way so it can be extended.
Test Plan: Ran `bin/cache purge --trace` with various valid and invalid flags.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12859
Differential Revision: https://secure.phabricator.com/D18146
Summary: Fixes T12860. Some joins were being dropped because we didn't call `parent::...`
Test Plan:
- Tagged a document.
- Searched for documents with that tag.
- Before change: got all documents.
- After change: got only tagged documents.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12860
Differential Revision: https://secure.phabricator.com/D18145
Summary:
Ref T12124. After D18134 we accept either "25" or "low" via HTTP parameters and when the field renders as a control, but if the form has a default value for the field but locks or hides it we don't actually run through that logic.
Canonicalize both when rendering the control and when using a raw saved default value.
Test Plan:
- Created a form with "Priority: Low".
- Hid the "Priority" field.
- Before patch: Tried to create a task, was rebuffed with a (now verbose and helpful, after D18135) error.
- Applied patch: things worked.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12124
Differential Revision: https://secure.phabricator.com/D18142
Summary:
Ref T12857. This is generally fairly fuzzy for now, but here's something concrete: when we build a large file with `diffusion.filecontentquery`, we compute the MIME type of all chunks, not just the initial chunk.
Instead, pass a dummy MIME type to non-initial chunks so we don't try to compute them. This mirrors logic elsewhere, in `file.uploadchunk`. This should perhaps be centralized at some point, but it's a bit tricky since the file doesn't know that it's a chunk until later.
Also, clean up the `TempFile` immediately -- this shouldn't actually affect anything, but we don't need it to live any longer than this.
Test Plan:
- Made `hashFileContent()` return `null` to skip the chunk cache.
- Added `phlog()` to the MIME type computation.
- Loaded a 12MB file in Diffusion.
- Before patch: Saw 3x MIME type computations, one for each 4MB chunk.
- After patch: Saw 1x MIME type computation, for initial chunk only.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12857
Differential Revision: https://secure.phabricator.com/D18138
Summary:
Ref T12124. This is a fairly narrow fix for existing saved EditEngine forms with a default priority value.
These saved forms have a numeric (or probably "string-numeric") default value, like "50". They lost their meaning after D18111, when "50" no longer appears in the dropdown. Instead, these forms all select the highest available priority.
At time of writing, this form was broken on this install, for example:
> https://secure.phabricator.com/transactions/editengine/maniphest.task/view/13/
Additionally, `/task/edit/form/123/?priority=...` (for templating forms) stopped working with `priority=50`. This isn't nearly as important, but a larger and more sudden compatiblity break than we need to make.
Add support for an "alias map" on `<select />` controls, so if the value comes in with something we don't recognize we'll treat it like some other value. Then alias all the numeric constants -- and other keywords -- to the right constants.
This ended up only affecting the `<select />` control in the web UI.
Test Plan:
- On `stable`, created a form with "Priority: Low".
- Before patch: form has "Priority: Unbreak Now!" on `master`.
- After patch: form has "Priority: Low" again.
- Used `?priority=25`, `?priority=wish`, `?priority=wishlist` to template forms: all forms worked.
Reviewers: amckinley, chad
Reviewed By: amckinley
Maniphest Tasks: T12124
Differential Revision: https://secure.phabricator.com/D18134
Summary:
Ref T12124. Currently, Conduit provides a fairly rough error message if you provide an invalid priority.
Instead, provide a more tailored message. Also, block `!!unknown!!` except from web edits.
Test Plan:
Before:
{F5007964}
After:
{F5007965}
Also, changed a priority to `999` in the database, edited it with the normal web UI form, it let me make the edit without being forced to adjust the priority.
Reviewers: amckinley, chad
Reviewed By: amckinley
Maniphest Tasks: T12124
Differential Revision: https://secure.phabricator.com/D18135
Summary: Fixes T12840. This adds a parallel "graph" button next to history on home and on the history list page. I'll think more about better placement of how to get to this page with the upcoming redesign that's still sitting in Pholio.
Test Plan: View History, View Graph, Try pager, go to a file, click view history, see no graph button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12840
Differential Revision: https://secure.phabricator.com/D18131
Summary:
Fixes T8909. Ref T12733.
UI attempts to follow the mock, but is a bit rough since PHUIXButtonView without text in this menu gets weird spacing, we don't have circular buttons yet, and PHUIXActionView without an icon also gets odd spacing.
Test Plan: {F5003125}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733, T8909
Differential Revision: https://secure.phabricator.com/D18128
Summary:
Ref T12733. This paves the way for a separate "hide" operation which completely hides things.
(I didn't extend this to the server side because that would require schema changes and the new "hide" state is client-only.)
Test Plan: Collapsed and expanded inlines, viewed tooltips.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18126
Summary: Fixes T12124. Changes `ManiphestEditEngine` to populate the select using priority keywords instead of the integer value. Marks `maniphest.querystatuses` as frozen. Adds a new Conduit method for fetching potential task statuses.
Test Plan: Created tasks and changed their priorities, observed that transactions in the DB still have the same type (integers as strings). Invoked `maniphest.update` with `priority => '90'` and observed that it still works. Invoked `maniphest.edit` with `priority => 'unbreak'` and observed that it now works.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T12124
Differential Revision: https://secure.phabricator.com/D18111
Summary:
Fixes T12844. This code is misleading: the daemon insert is happening on a different connection, and is not inside the transaction on the Mail connection.
What actually happens is this:
- (Connection A) `BEGIN`
- (Connection A) `INSERT INTO mail ...`
- (Connection B) `INSERT INTO worker ...` <-- This is a different connection, and it is NOT in a transaction!
- There's a race window here: the worker row is globally visible but the mail row is still isolated inside the transaction.
- (Connection A) `COMMIT`
- Now we're clear: the mail row is globally visible.
Change this code to reflect what's actually happening.
This means that if the worker row insert fails for some reason, we'll now throw with a mail row written to the database. But this is fine: it doesn't send on its own (so it can't cause mail loops or anything) and it can be re-queued with `bin/mail resend` if necessary without too much trouble.
Test Plan: See T12844 for particulars. Made some comments on tasks, saw the daemons send mail.
Reviewers: chad, amckinley, jmeador
Reviewed By: jmeador
Maniphest Tasks: T12844
Differential Revision: https://secure.phabricator.com/D18124
Summary:
Just add the monogram to the datasource's `name` field
so that it will match when typing Wnn in the typeahead field.
Test Plan:
Tested locally on my dev phab. Try searching for a panel
by monogram in the 'Add existing panel' dialog on the
'arrange workboard' interface.
Previously: typing W123 showed no results.
After this change: typing W123 finds the panel W123
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18121
Summary: Moves DiffusionTagsListView to uhhh, list. Separates out table view which is still in use now, implements mobile friendly UI for tags.
Test Plan:
Review KDE's Krita repository locally with lots of tags, desktop and mobile.
{F4997708}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18115
Summary: Adds a new DiffusionBranchListView which replaces the BranchTable when browsing all branches in Diffusion. Has all the same capabilities, but is easier to read, adds a Compare button, and plays nicely on mobile. It does take up more space, but I think that's generally OK here since we expect our branches to not be heaping piles of intern revert branches.
Test Plan:
Follow a few repositories with branches, like Phabricator and KDE's Krita. View layouts on mobile, tablet, desktop. Try out new compare button.
{F4996207}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: avivey, Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18113
Summary: Adds a very basic list of all inline comments, threaded, and their status. Kept this a little simpler than the mock, mostly because sorting here feels a little strange given threads would be all over the place. Not sure sorted is needed in practice anyways. I'd probably lean towards just adding a JS checkbox to hide certain rows if needed in the future.
Test Plan:
Test various commenting structures:
- Leave Comment
- Update Diff
- Leave new comment
- Reply to comment
- Reply to comment as revision author
- Mark items as done
- Update diff again
{F4996915}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18112
Summary: Fixes T12828. This API was tightened up D17616, but I missed this callsite.
Test Plan: I've just got a good feeling in my bones.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12828
Differential Revision: https://secure.phabricator.com/D18119
Summary: Fixes T12821. (Some events only happen once, so the default behavior doesn't let you pick this rule, and objects must opt into it by saying "yeah, I support multiple edits".)
Test Plan:
Note "only the first time" selected:
{F4999093}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12821
Differential Revision: https://secure.phabricator.com/D18117
Summary: We seem to use these a lot. Makes the code cleaner.
Test Plan: UIExamples.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18114
Summary: Builds out some images to use to identify repositories. Fixes T12825.
Test Plan:
Try setting custom, built in, and null images.
{F4998175}
{F4998192}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12825
Differential Revision: https://secure.phabricator.com/D18116
Summary:
Fixes T12803. An install is having difficulty diagnosing mail failures, and one component is that permanent task failures aren't reaching the log.
It's reasonable to send these to the log even when "phd.verbose" is off. See T12803 for a rough review of when we generate these failrues today.
Test Plan:
- Faked some exceptions.
- Got a result in the log (P2058) with `phd.verbose` turned off.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12803
Differential Revision: https://secure.phabricator.com/D18106
Summary:
Fixes T12807. Some shells may apparently mangle/strip UTF8 characters? Just dodge this whole problem by sending the pattern over stdin rather than actually figuring out the particulars.
Related tasks, like T7339 and T5554, discuss finding broader fixes for this class of issue, and this definitely isn't exactly a fully legitimate fix, but in many cases (as here) we can reasonably just avoid the problem rather than actually fixing it, at least for a long time.
Test Plan: Searched for emoji and non-emoji locally, but this worked fine (on OSX) for me before the patch too.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12807
Differential Revision: https://secure.phabricator.com/D18105
Summary:
Fixes T12811. The issue here //appears// to be that both the "alice committed rXYZabc" and "Herald added projects..." actions have the same (default) strength and end up applying in arbitrary order, and probably got shuffled around as this transitioned to Modular transactions.
Give "alice committed rXYZabc" an explicitly higher action strength.
Test Plan:
- Wrote an "Always, add project X" Herald rule for commits.
- Ran `bin/repository reparse --herald ...`.
- Saw an "alice committed rXYZabc" story instead of a "Herald added projects: X" story.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12811
Differential Revision: https://secure.phabricator.com/D18104
Summary: Does the UI work that's part of T12234 and adds migrations for both of the old-style duplicate transactions.
Test Plan:
- Started with a clean DB.
- Checked out really old code that marks tasks as dupes using comments.
- Made a bunch of tasks and closed some as dupes. Made a bunch of additional comments.
- Checked out D10427 and did a `storage upgrade`.
- Made a bunch more new tasks and dupes.
- Snapshotted DB.
- Ran migration repeatedly until all expected edges showed up in the `phabricator_maniphest.edge`table.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T12234
Differential Revision: https://secure.phabricator.com/D18037
Summary: Ran across a few straglers. Convert to the correct color.
Test Plan: grep for profile-image-button, check profile image selection page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18096
Summary: These are now unused.
Test Plan: grep, remove uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18090
Summary: Porting over a fix that we could miss the tail end of commits. Also use the new tag borderless option.
Test Plan: Review various commit pages in profile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18086
Summary: Little nits and spacing changes to viewing diffusion commit history on phones.
Test Plan:
Review in Chrome, iOS Simulator.
{F4990749}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18085
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.
Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18077
Summary: Formally support borderless tags in PHUITagView.
Test Plan: Used in Diffusion History List
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18074
Summary:
Ref T12793. I'd like to understand exactly when we broke this, but this seems to be a minimal fix that shouldn't do anything surprising.
When you move document `/a/` to `/a/b/` and that path doesn't exist yet, the Content transaction currently fails because there's "no content". The content gets added later by the "move" transaction but this is implicit.
To make this work, just ignore the "missing field" error. This is a little roundabout but unlikely to break anything in weird ways.
Test Plan:
- Moved document `/a/b/` to `/a/b/c/`.
- Before patch: error about missing content.
- After patch: move worked properly.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12793
Differential Revision: https://secure.phabricator.com/D18069
Summary: Saturate the color a little more, add yellow
Test Plan: uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18068
Summary:
Commits in the list are grouped by the date they occurred in server time. This may not be the date they occurred in client time.
Use client time, not server time, to group commits.
Test Plan:
- Set server timezone to "Asia/Famagusta".
- Set client timezone to "America/Los_Angeles".
- Viewed Phabricator repository history.
Here's what it looks like before the change:
{F4987094}
Note that the headers of the first two groups both say "Yesterday".
This is because the first commits in each group occurred on June 1 and June 2, respectively, in Famagusta, but both occurred on June 1 in Los Angeles.
Here's what it looks like after the change:
{F4987095}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18067
Summary:
Currently, the last group of commits is not shown in the list view because the final `$list` is never added to `$view`.
For example, if the first page would contain commits from "April 7", "April 6", and "April 5", commits from "April 5" are not shown.
(If a repository has 100 commits in a single day, nothing is shown.)
On this server, here's the bottom of page 1:
{F4987087}
Here's the top of page 2:
{F4987088}
However, here's `git log` between those commits:
```
$ git log --oneline 7e46^..5f49f
5f49f9c793 Add sound to logged out Conpherence
1644b45050 Disperse task subpriorities in blocks
c6a7bcfe89 Make Pholio description behave as a remarkup field (e.g., subscribe mentioned users)
bbc5f79227 Make membership lock/unlock feed stories read more naturally
789d57522b Make editing project images redirect to "Manage" more consistently
10b3879232 Make Project slug/hashtag transactions render a little more nicely
abd791889c Update Maniphest title transaction again
5a34b299e4 Update Maniphest title language
601622013d Clarify milestone/subproject creation language
c9889e3d55 Fix an issue in Phriction where moving a document just copied it instead
fdf00f6df4 Clean up some minor UI behaviors in Differential
6c46f27d98 Add quest objectives to the minimap
d783299a19 Fix Phriction status not set property on new document
93e28da76e Add more "disabled" UI to PHUIObjectItemView
7e46d7ab6a Migrate Project color to modular transactions
```
This group of commits does not currently appear anywhere in the list.
Test Plan: Viewed a page of commits, saw 100 commits.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18066
Summary:
Ref T12733. Some minor issues:
- The `strlen(...)` test against `$this->text` fails if a caller does something like `setText(array(...))`. This is rare, but used in `DiffusionBrowseController`, from D15487.
- Add PHUIX examples for icon-only buttons.
- Remove unused `SIMPLE` constant now that no callsites remain.
Test Plan:
- Viewed a directory in Diffusion's "Browse" view in a Git repository, no longer saw a warning / error log.
- Viewed PHUIX Components UI examples.
- Grepped for `::SIMPLE`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18065
Summary:
Fixes T12789. See that task for discussion. Currently, when multiple packages own the same path but have different dominion rules we get some weird/aribtrary/inconsistent results.
Instead, implement these rules:
- If zero or more weak and one or more strong packages claim a path, the strong packages (exactly) all own it.
- If one or more weak packages and zero strong packages claim a path, the weak packages all own it.
The major change here is that instead of keeping the //first// weak package we run into, we keep all the weak packages with the longest claim that we run into.
This needs to be implemented twice because Owners has two different near-copies of this logic, only one of which has test coverage. Some day maybe this will get fixed.
Test Plan:
- Added failing unit tests, made them pass.
- Viewed all A/B strong/weak combinations in Diffusion, saw sensible ownership results.
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Maniphest Tasks: T12789
Differential Revision: https://secure.phabricator.com/D18064
Summary:
Fixes T12790. I don't think this was actually a regression, Settings just wasn't launchable before global settings (since it had no real landing page, and the profile menu always had a link) and didn't get marked launchable once we added them.
I also double-checked other un-launchable apps; Nuance is probably close enough to make launchable now while I'm in here.
Test Plan: Typed "settings" into global typeahead, got settings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12790
Differential Revision: https://secure.phabricator.com/D18062
Summary: Ref rPf2fcafb40dde94ddf4ee22716fea74fca0334a64#38208, I think this is a more usable layout. Gets rid of clippy, audit. Adds back Differential link as tag, Build Status as button.
Test Plan: Faked data on this for Differential, Builds, should all work though. Test on real and fake repositories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18061
Summary:
See D18018. Ref T12787. This doesn't actually work; we started publishing these stories as a side effect of converting to ModularTransactions, then I fixed the rendering.
This mechanism has very few callsites and I suspect we may want to get rid of it (see T12787) so just keep publishing these stories for now.
Test Plan: Changed the point value of a task, saw a feed story both before and after the patch.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12787
Differential Revision: https://secure.phabricator.com/D18059
Summary: Fixes T12787. Modular Transactions don't actually support `shouldHideForFeed()`. I'll add some discussion to the task.
Test Plan: Created a subtask, saw no more "X reopened Y, a subtask of P" feed story.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12787
Differential Revision: https://secure.phabricator.com/D18058
Summary: Let's buttons just be an icon, no pressure to also have text.
Test Plan: UIExamples, Search, Home, Policy Controls... Probably 99% of them.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18056
Summary: Ref T12780. I'd like 18,000 GitHub stars now please thank you
Test Plan: this feature is awful
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12780
Differential Revision: https://secure.phabricator.com/D18053
Summary: Ref T12780. Makes the button do something useful, like link to the history at the right spot in the graph.
Test Plan: Click on various browse buttons, get correct url.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12780
Differential Revision: https://secure.phabricator.com/D18054
Summary:
Ref T12733. Ref M1476. This adds `PHUIXButtonView`, for client-side button rendering.
It also adds a PHUIX example which renders the server and client versions of each component side-by-side so it's easier to see if they're messed up.
Test Plan: {F4984128}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18051
Summary:
Ref M1476. I'm planning to add some PHUIX examples, but sort out the existing examples a little first. I added some categories:
- Catalogs: these are what I look at most often (emoji, icons, colors).
- Single use: elements with only one use (badges, feed stories, hovercards, setup issues).
- Technical: examples which are really just "test this thing in the browser" (avatars, gestures, notifications, remarkup).
- Other: evrything else, mostly general-purpose multi-use components.
Test Plan:
(See left nav.)
{F4984042}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18050
Summary: Ref M1476. I'm going to see if I can set up side-by-side "PHUI" vs "PHUIX" to make maintaining them a touch easier. Before doing that, nuke some really old UI examples that don't seem very useful.
Test Plan: Viewed UIExamples, saw fewer bad ones.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18049
Summary:
Ref M1476. Currently, `setColor('simple')` is meaningful. Instead, `setButtonType('simple')`.
Depends on D18047.
Test Plan: Looked at UI examples, Phame, Auth. Notifications mooted by D18047.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18048
Summary: Ref T12733. Completely removes the objectives UI.
Test Plan:
- Grepped for `objective`, etc.
- Browsed revisions, no JS errors / broken stuff.
- (If I missed anything, it's likely to turn up in followup changes.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18043
Summary: This moves Diffusion History to use an easier to parse list view for commits and their (diff, audit, build) status. I left TableView around, which is used on a repositories home, and we can maybe add a "graph view" history back as another controller. Not sure what the real use is for that kind of feature though. I don't have Harbormaster set up locally so I could use another install to give this a run. I also expect to maybe not live with this UI as final, I like the UX, but the icons for indicating status don't really feel great to me, just OK.
Test Plan:
pull various repositories, check various history displays.
{F4980356}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18039
Summary: Ref T12423. Set the grouping by priority. Note this doesn't render headers but I don't want to spend a lot of time on this.
Test Plan: Review tasks in my sandbox, see them ordered by priority.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12423
Differential Revision: https://secure.phabricator.com/D18046
Summary: Ref T12776. This extraction of file PHIDs extracted "PholioImage" object PHIDs (`PHID-PIMG-...`), not "File" PHIDs (`PHID-FILE-...`). Instead, dig into the Pholio images and actually extract the file PHIDs. This method is now similar to the `PholioImageFileTransaction` method, which works already.
Test Plan:
- Create a mock.
- Update one of the images.
- In Files, view the "Attached" tab of the updated image.
- Before patch: not attached to mock.
- After patch: properly attached to mock.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12776
Differential Revision: https://secure.phabricator.com/D18042
Summary: Fixes T12775. Currently, we do not validate this option and it's possible to configure it in an invalid way.
Test Plan: Tried to misconfigure things, was helpfully pointed toward errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12775
Differential Revision: https://secure.phabricator.com/D18041
Summary: Start on plan outlined in T12124. Adds a new Conduit method for querying information about task priorities.
Test Plan: Ran locally; observed expected output: {F4979109}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D18035
Summary:
Ref T12314. Open to counterdiffs / iterating / suggestions / skipping most or all of this, mostly just throwing this out there as a maybe-reasonable first pass.
When a task has a subtype (like "Plant" or "Animal"), provide some hints on the task list, workboards, and task detail.
To make these hints more useful, allow subtypes to have icons and colors.
Also use these icons and colors in the typeahead tokens.
The current rule is that we show the subtype if it's not the default subtype. Another rule we could use is "show the subtype if there's more than one subtype defined", but my guess is that most installs will mostly have something like "normal task" as the default subtype.
Test Plan:
The interfaces this affects are: task detail view, task list view, workboard cards, subtype typeahead.
{F3539128}
{F3539144}
{F3539167}
{F3539185}
Reviewers: chad
Reviewed By: chad
Subscribers: johnny-bit, bbrdaric, benwick, fooishbar
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17451
Summary: I think this is reasonable for my current use case, but stacking icons overally is pretty clunky.
Test Plan:
UIExamples
{F4978899}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18032
Summary: I think this name is more accurate, also add proper links to author image.
Test Plan: Review commits in sandbox, see new URL on image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18026
Summary: Fixes T12505. `PhabricatorProjectsMembershipIndexEngineExtension->materializeProject()` was incorrectly bailing early for milestone objects, which prevented milestone members from being calculated correctly. This was causing problems where (for example) an Owners package owned by a milestone wasn't being satisfied when a member of the milestone approved a revision.
Test Plan: Invoked migration, observed that a user's milestones correctly showed up when searched for. Also observed that accepting a revision on behalf of a milestone now satisfies Owners rules.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12505
Differential Revision: https://secure.phabricator.com/D18033
Summary:
Fixes T12762. Currently, there's no way to get from these boxes into generaly history in Feed, and it isn't clear that the operation is possible.
For now, add some simple links. See T12762 for future work.
Test Plan:
- Viewed user profles, saw "View All".
- Viewed project profiles, saw "View All".
{F4978858}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18030
Summary: Ref T12762.
Test Plan:
- Ran queries with start date, end date, both, neither.
- Used EXPLAIN to try to make sure doing the bitshift isn't going to be a performance issue.
{F4978842}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18029
Summary:
Ref T12762. This updates FeedSeachEngine to user modern construction.
I've tried to retain behavior exactly, although the "Include stories about projects I'm a member of" checkbox is now nonstandard/obsolete. We could likely fold that into "Include Projects" in a future change which does a backward compatibility break.
Test Plan:
- Queried feed without constraints.
- Queried feed by user, project, "stuff I'm a member of", prebuilt "Tags" query.
- Viewed user profile / project profile feeds.
- Used function tokens (`viewer()`, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18028
Summary: Ref T12762. Updates some conventions and methods. This has no (meaningful) behavioral changes.
Test Plan:
- Grepped for `setFilterPHIDs()`.
- Viewed main feed, user feed, project feed.
- Called `feed.query`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18027
Summary:
Fixes T12720. Currently, old daemon records are collected based on creation date. By default, the GC collects them after 7 days.
After T12298, this can incorrectly collect hibernating daemons which are in state "wait".
In all cases, this could fail to collect daemons which are stuck in "running" for a long time for some reason. This doesn't seem to be causing any problems right now, but it makes me hesitant to do "dateCreated + not running or waiting" since that might make this become a problem, or make an existing problem with this that we just haven't bumped into worse.
Daemons always heartbeat periodically and update their rows, so `dateModified` is always fresh, so collect rows based only on modification date.
Test Plan:
- Ran daemons (`bin/phd start`).
- Waited a few minutes.
- Verified that hibernating daemons in the "wait" state had fresh timestamps.
- Verified that very old daemons still got GC'd properly.
```
mysql> select id, daemon, status, FROM_UNIXTIME(dateCreated), FROM_UNIXTIME(dateModified) from daemon_log;
+-------+--------------------------------------+--------+----------------------------+-----------------------------+
| id | daemon | status | FROM_UNIXTIME(dateCreated) | FROM_UNIXTIME(dateModified) |
+-------+--------------------------------------+--------+----------------------------+-----------------------------+
| 73377 | PhabricatorTaskmasterDaemon | exit | 2017-05-19 10:53:03 | 2017-05-19 12:38:54 |
...
| 73388 | PhabricatorRepositoryPullLocalDaemon | run | 2017-05-26 08:43:29 | 2017-05-26 08:45:30 |
| 73389 | PhabricatorTriggerDaemon | run | 2017-05-26 08:43:29 | 2017-05-26 08:46:35 |
| 73390 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:29 | 2017-05-26 08:46:35 |
| 73391 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:33 | 2017-05-26 08:46:33 |
| 73392 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:37 | 2017-05-26 08:46:31 |
| 73393 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:40 | 2017-05-26 08:46:33 |
+-------+--------------------------------------+--------+----------------------------+-----------------------------+
17 rows in set (0.00 sec)
```
Note that:
- The oldest daemon is <7 days old -- I had some other older rows but they got GC'd properly.
- The hibernating taskmasters (at the bottom, in state "wait") have recent/more-current `dateModified` dates than their `dateCreated` dates.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12720
Differential Revision: https://secure.phabricator.com/D18024
Summary:
Ref T12738. By default, we process Nuance commands in the background. The intent is to let the user continue working at full speed if Twitter or GitHub (or whatever) is being a little slow.
Some commands don't do anything heavy and can be processed in the foreground. Let commands choose to try foreground execution.
Test Plan: Threw complaints in the trash, saw them immediately go into the trash.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18015
Summary: Makes this a bit more flexible and allow UI to take over `col-2` completely. Also cleaned up application search a little with tags
Test Plan: Review various pages, grep for callsites.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18021
Summary: Some of these are unused, defaults to a lighter color naturally.
Test Plan: uiexamples, grep, phriction
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18020
Summary:
Fixes T12757. Here's a simple repro for this:
- Add a package you own as a reviewer to a revision you're reviewing.
- Open two windows, select "Accept", don't submit the form.
- Submit the form in window A.
- Submit the fomr in window B.
Previously, window B would show an error, because we considered accepting on behalf of the package invalid, as the package had already accepted.
Instead, let repeat-accepts through without complaint.
Some product stuff:
- We could roadblock users with a more narrow validation error message here instead, like "Package X has already been accepted.", but I think this would be more annoying than helpful.
- If your accept has no effect (i.e., everything you're accepting for has already accepted) we currently just let it through. I think this is fine -- and a bit tricky to tailor -- but the ideal/consistent beavior is to do a "no effect" warning like "All the reviewers you're accepting for have already accepted.". This is sufficiently finnicky/rare (and probably not terribly useful/desiable in this specific case)that I'm just punting.
Test Plan: Did the flow above, got an "Accept" instead of a validation error.
Reviewers: chad, lvital
Reviewed By: chad, lvital
Subscribers: lvital
Maniphest Tasks: T12757
Differential Revision: https://secure.phabricator.com/D18019
Summary: In some cases we may want a different URI for the image on an item than the header/title of the item (like user / title). This prioritizes ImageHref over Href.
Test Plan: uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18016
Test Plan: Unit tests pass, manually changed the default sort and filter on a workboard and observed expected transactions in the DB.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D18013
Summary: Ref T12738. Implements some modular behavior for Nuance commands.
Test Plan: {F4975322}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18011
Summary:
Ref T12738. This makes clicking "Throw In Trash" technically do something, sort of.
In Nuance, the default mode of operation for actions is asynchronous -- so you don't have to wait for a response from Twitter or GitHub after you mash the "send default reply tweet" / "close this pull request with a nice response" button and can move directly to the next item instead.
In the future, some operations will attempt to apply synchronously (e.g., local actions like "ignore this item forever"). This fakes our way through that for now.
There's also no connection to the action actually doing anything yet, but I'll probably rig that up next.
Test Plan: {F4975227}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18010
Summary:
Ref T12738. This doesn't actually do anything yet, but allows items to define commands that show up in the UI.
Adds a "Throw in Trash" item for complaints.
This construction will allow future changes to add an `EngineExtension` which can provide generic/default commands across item types.
Test Plan: {F4975086}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18009
Summary:
Ref T12738. This is mostly just laying in groundwork and prerequisites, like the ability to query items by queue.
Eventually, this will become the main UI which staff use to process a queue of items. For now, it does nothing and renders nonsense.
This and probably the next big chunk of changes are all going to be made-up, nonfinal things that just make basic operations work until we have fundamental flows -- like "assign", "comment", "close" -- working at a basic level and can think more about UI/workflow.
Test Plan:
Visited the page, it loaded a mostly-reasonable item and then rendered nonsense:
{F4975050}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18008
Summary: Ref T12738. Some of the Nuance "form" workflows currently fatal after work on the GitHub stuff. Try to make everything stop fataling, at least.
Test Plan: Using "Complaints Form" no longer fatals, and now lodges a complaint instead.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18007
Summary: Minor, just shows the slugs on the manage project page, also normalized language to "details"
Test Plan: review a project with slugs, description.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D17985
Summary: Gives the ability to hide a big long block of text in an ObjectListItem without cluttering the UI.
Test Plan:
Added a test case to UIExamples. Click on icon, see content. Click again, content go away.
{F4974153}
{F4974311}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18006
Summary: Going to play a bit with this layout (diffusion sans audit) and see how it feels on profile. Uses a user image, moves the commit hash (easily selectible) and separates commits by date.
Test Plan:
Review profiles with and without commits.
{F4973987}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18005
Summary: This was interesting, because there were a mix of callsites using transactions and others that just set the property on the `Project` object. I made everything consistent in using transactions to change this property. I also found an implementation of `getTitle()` that I don't think is ever being invoked since `shouldHide()` is returning `true`, but I migrated it anyway.
Test Plan: Unit tests pass + enabling/disabling workboards (and importing).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D18004
Summary: This moves the navigation to a standard sidebar, and moves all actions to the curtain. Also pulled out info view when available for cleaner UI.
Test Plan:
Create a git, svn, hg test repository and verify each page in the sidebar renders as expected.
{F4973792}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18003
Test Plan: Unit tests pass. Went through the UI for creating new subprojects and milestones, but didn't setup some API calls to check that all the validation errors were still caught.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17999
Summary: Fixes T12744. Unclear why `null` doesn't work here but does for the title, but `!strlen` seems to work fine in both cases.
Test Plan: Create a new task, check mail folder, see [Created]
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12744
Differential Revision: https://secure.phabricator.com/D18002
Summary: Ref T12625
Test Plan: Move a document to a new location, verify the old and new document. Edit both. Grep for MOVE_AWAY
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12625
Differential Revision: https://secure.phabricator.com/D17988
Summary: GREEN
Test Plan: View a dashboard page, see green button
Reviewers: amckinley, epriestley
Reviewed By: epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17997
Summary: Ref T12738. Update sources to modular transactions.
Test Plan: Created and edited a source.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D17994
Summary:
Ref T12738. Moves existing non-modular transactions to modular transactions.
Some of these are pretty flimsy, but a lot of them don't actually work or do anything in Nuance yet anyway.
Test Plan: Gently poked Nuance, nothing fell over.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D17990
Summary: Grep for phui-tag-shade and verify we're no longer calling shade-color directly.
Test Plan: Search, workboard, story points, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17993
Summary: Adds a new tag type, starts to try to clean up the mess that are PHUITags
Test Plan:
Review UIExamples.
{F4972323}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17991
Summary: See T12673
Test Plan: Unit tests pass. Locked and unlocked a project and saw timeline changes.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17986
Summary: Ref T12423. Adds back revisions as a user profile page. I don't want to think about custom profiles for a while.
Test Plan: Make some diffs, visit my profile, see diffs.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12423
Differential Revision: https://secure.phabricator.com/D17987
Summary: Adds a divider and better grouping
Test Plan: Click on dropdown menu on a workboard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17984
Summary:
Ref T12733.
- While editing a comment, show a pink star ({icon star, color=pink}) with a tooltip.
- Slight UI tweaks, including draft comments getting an indigo pencil ({icon pencil, color=indigo}).
Test Plan: {F4968470}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17977
Summary: See D17955.
Test Plan: Loaded a revision, no longer saw annotations with prototypes off. Still saw annotations with prototypes on.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17983
Summary: Fixes T12735. Adds a sound if the user is logged out, skips checking a setting.
Test Plan: set participants to null and verify sound plays, no exceptions1111
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12735
Differential Revision: https://secure.phabricator.com/D17973
Summary:
Ref T7664. The current algorithm for moving task subpriorities can end up stuck in a real sticky swamp in some unusual situations.
Instead, use an algorithm which works like this:
- When we notice two tasks are too close together, look at the area around those tasks (just a few paces).
- If things look pretty empty, we can just spread the tasks out a little bit.
- But, if things are still real crowded, take another look further.
- Keep doing that until we're looking at a real nice big spot which doesn't have too many tasks in it in total, even if they're all in one place right now.
- Then, move 'em out!
Also:
- Just swallow our pride and do the gross `INSERT INTO ... "", "", "", "", "", "", ... ON DUPLICATE KEY UPDATE` to bulk update.
- Fix an issue where a single move could cause two different subpriority recalculations.
Test Plan:
- Changed `ManiphesTaskTestCase->testTaskAdjacentBlocks()` to insert 1,000 tasks with identical subpriorities, saw them spread out in 11 queries instead of >1,000.
- Dragged tons of tasks around on workboards.
- Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7664
Differential Revision: https://secure.phabricator.com/D17959
Summary: Ref T12732. This is pre-existing but fix it since I caught it while banging around.
Test Plan: {F4967442}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17970
Summary:
Ref T12732. Currently, different ways of setting a profile image can leave you in different places.
Instead, always send the user back to the "Manage" page.
Test Plan: Used "Current Picture", "use picture", "Build picture" and "upload picture", always ended up in the same spot.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17967
