Summary: Added 6px to the left and right of each TD on the revision history table.
Test Plan: Used Chrome and a table with many revisions.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4197
Summary:
- Fixes a bug where dragging a flexible left nav and then pressing "f" to hide it didn't properly reset the content panel's left margin.
- Fixes a bug where D4185 removed "white-space: nowrap;" accidentally (from @asherkin)
Test Plan:
- Dragged bar, then pressed "f".
- Can't repro the nowrap thing for some reason.
iiam
Reviewers: asherkin, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4194
Summary: Permit the forcible release of Drydock leases. The implementation isn't very exciting for now.
Test Plan: Released leases via web and CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4181
Summary:
Fixes glitches in the side nav. Resolves T1828. Resolves T2156.
- Elastic scrolling (T2156): in Safari on OSX, using a scroll touch on the trackpad to scroll up past the top of the document caused newer-style side menus to scroll down, leaving a visible whitespace bar.
- Whitespace glitch: Particularly in Safari, scrolling down the document quickly from the top caused the top menu to scroll away before the side menu rose to meet it. Use a fixed background color bar that extends under the menu so this doesn't happen.
- Use of "!important": use CSS better so we don't need to "!important" things.
- Dark Console (T1828): Instead of hard-coding the top position, determine it dynamically by looking at where the content is. This also fixes the menu overlapping with the red "there are errors on this page" development bar.
- General "fixed" glitchiness: don't use fixed-position for menu content other than flexible (draggable) menus.
Test Plan:
- Viewed and scrolled menus in Paste. Opened and closed DarkConsole. Switched devices.
- Viewed and scrolled flexible menus in Differential and Diffusion. Opened and closed DarkConsole. Switched devices.
Reviewers: vrana, chad, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1828, T2156
Differential Revision: https://secure.phabricator.com/D4185
Summary: Add a WIKI block to the switch in PhabricatorObjectHandleData->loadObjects().
Test Plan: Went to /feed/ and saw my wiki edit stories.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4180
Summary:
We don't own the copyright to (or have a license to distribute) this media file.
Also improve performance.
Test Plan: ↑↑↓↓←→←→BA <Enter>
Reviewers: codeblock
Reviewed By: codeblock
CC: aran
Differential Revision: https://secure.phabricator.com/D4176
Summary: See discussion in T2014. Aligns this element more closely with @chad's `frame_v3.psd` mock, and implements the icon/label element. Removes "details".
Test Plan: {F27062} {F27063} {F27064} {F27065}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D4179
Summary: noticed when creating a task that the "you made a task - make another" ui element had a bottom margin element which seemed out of place in the re-design. killed the margin
Test Plan: looks good to me!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4178
Summary: most awesome is that differential-primary-pane no longer has a place in diffusion. less awesome is fixing the zebra striping on differential "Local Commits" view and making the font size of one of the table headers match the others.
Test Plan: looks good!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4177
Summary: we don't always have a diff so instead set an explicit title in the controller.
Test Plan: no more fatals. grepped carefully for every call site and tested them all
Summary: basically made the header elements for the individual panes and cleaned up the panes to make it look okay. tried to copy colors from @chad 's mocks.
Test Plan: looks good to me
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D4174
Summary:
If something that doesn't belong to any field appears in the commit message
below the differential revision field, it gets included as part of the
value for the field, which can mess up parsing.
Test Plan:
called differential.parsecommitmessage on a commit whose differential
revision field wasn't being parsed earlier (it had a line of dashes two
lines below the Differential Revision: line).
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4171
Summary:
Right now, Drydock gives out multiple leases to the same working copy and gives out leases to working copies with repository "P" in them when the user requested some other repository.
Add two callbacks:
- `canAllocateLease()` - allows a blueprint to reject a lease on a resource because of a fundamental incompatibility, like "it's a working copy with Phabricator in it, but the lease wants a working copy with Javelin in it".
- `shouldAllocateLease()` - allows a blueprint to reject a lease on a resource because of resource limits, like "only one active lease can own a working copy at a time".
Also cleaned up various other things.
Test Plan:
After implementing the callbacks, Drydock has the correct behavior:
- It gives multiple leases on `localhost`, but only one lease per working-copy resource.
- It does not grant leases on resources with repository X to requests for repository Y.
Ran `bin/drydock lease --type working-copy --repositoryID 12` and similar repeatedly and verified results in the web console.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4166
Summary:
- Remove "Diffusion" crumb (redundant with application icon)
- Put "All Repositories" in its place on the landing page.
- Render the repository crumb as "rX" instead of "X Repository".
Test Plan: Looked at various Diffusion pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4173
Summary:
upgrades are CrumbsView, HeaderView, PropertyListView, and ActionListView. I had to modify CrumbsView stuff a bit to handle the "advanced" diffusion crumbs.
Quirks fixed include making file tree view show up in diffusion, the page not have extra space when the file tree is hidden, links no longer breaking once you visit files (since without the change the files always got "/" appended and thus 404'd), and a differential quirk where it read "next step:" and that colon is a no no,
Test Plan: played around in diffusion and differential
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T2048, T2178
Differential Revision: https://secure.phabricator.com/D4169
Summary:
Clicking "show details" of a task description change in Maniphest currently throws an exception about the markup engine.
Since we don't actually need the engine an alternate fix would be "if ($this->markupEngine) { $renderer->setMarkupEngine($this->markupEngine); }" but we have one at the ready so just provide it. This should become part of the Transactions stuff anyway.
Test Plan: Clicked "show details".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4167
Summary:
This adds a configuration option for repositories to be marked as managed
by phabricator, which is then used by the pullLocal daemon to try to recover
from some errors it runs into.
Test Plan: Set a bogus uri for a repo, saw that the pullLocal daemon fixed it.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3680
Summary: Changes the tan table with the girly blue header color.
Test Plan: Chrome
Reviewers: vrana, epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2176
Differential Revision: https://secure.phabricator.com/D4164
Summary: Show attributes on the view pages.
Test Plan: {F26985} {F26986}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4165
Summary: wanted to play with some policy stuff as its been a bit. Turns out you can't edit questions so this is very silly "so long as you are a user you can view it" policy. also sorry if you have a diff or twelve out for this in your sandbox(es).
Test Plan: loaded up ponder and clicked about
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2113
Differential Revision: https://secure.phabricator.com/D4163
Summary:
When a user submits an action with no effect (like an empty comment, an "abandon" on an already-accepted revision, or a "close, resolved" on a closed task) we want to alert them that their action isn't effective. These warnings fall into two general buckets:
- User is submitting two or more actions, and some aren't effective but some are. Prompt them to apply the effective actions only.
- A special case of this is where the only effective action is a comment. We provide tailored text ("Post Comment") in this case.
- User is submitting one action, which isn't effective. Tell them they're out of luck.
- A special case of this is an empty comment. We provide tailored text in this case.
By default, the transaction editor throws when transactions have no effect. The caller can then deal with this, or use `PhabricatorApplicationTransactionNoEffectResponse` to provide a standard dialog that gives the user information as above. For cases where we expect transactions to have no effect (notably, "edit" forms) we just continue on no-effect unconditionally.
Also fix an issue where new, combined or filtered transactions would not be represented properly in the Ajax response (i.e., return final transactions from `applyTransactions()`), and translate some strings.
Test Plan:
- Submitted empty and nonempy comments in Macro and Pholio.
- Submitted comments with new and existing "@mentions".
- Submitted edits in both applications.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T912, T2104
Differential Revision: https://secure.phabricator.com/D4160
Summary: Currently, AphrontProxyResponse is expected to build a string. This prevents some response types (like Dialog) from being proxied, because they have special rules. Instead, make proxy responses reduce into a non-proxied response so it's possible to proxy any type of response and hit all the normal rules for it.
Test Plan: Built a proxied DialogResponse on top of this.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104, T912
Differential Revision: https://secure.phabricator.com/D4159
Summary:
For transaction interfaces, I want to prompt the user when they take an action that has no effect, e.g.:
Action Has No Effect
You can not close this task, because someone else has already closed it.
Do you want to post your comment anyway?
[Cancel] [Post Comment]
We already do this for Differential, but it's all hard-coded. T912 is an open task for fixing this for Maniphest.
To do this in a general way, I want to embed the entire request in the dialog as hidden inputs, then add a "__continue__" key and resubmit the form. The endpoint will read this key the second time through and apply what effects it can (e.g., just post a comment).
This adds a mechanism for getting all the request data, minus "magic" like __dialog__ and __csrf__. We need to jump through some hoops because of how PHP encodes arrays.
Test Plan: Ran unit tests, built "no effect" dialogs on top of this.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T912, T2104
Differential Revision: https://secure.phabricator.com/D4158
Summary: pull out some more stuff from the TwoUp renderer that's generically useful. also clean up $xhp variable and add a get method for the $coverage. Finally, fix T2177 while I'm in here.
Test Plan: played around with Differential. Also opened things up in Firefox to verify T2177.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009, T2177
Differential Revision: https://secure.phabricator.com/D4161
Summary: I thought I'd already implemented this, but hadn't. Implement a "USER" policy -- a USER phid means only that user has the capability.
Test Plan: Looked at macros as a user other than the comment owner.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4155
Summary: D4153 made these render with newlines between items; use commas instead.
Test Plan: {F26950}
Reviewers: btrahan, chad, vrana
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4162
Summary:
Git adds it automatically.
I don't like this solution much because there could be other unknown fields appended to the end of the commit message which will break parsing.
Test Plan:
Reparsed commit ending with:
> Differential Revision: ...
> Conflicts: ...
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4157
Summary:
Warning boxes in a haunted differential comment panel were showing up
without a background (so the contents below were showing through).
This adds the background.
Test Plan:
viewed haunted differential comment panel on a diff with lint errors
and selected "accept revision". Confirmed that the diff and sidebar did
not show through underneath the red warning box.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4154
Summary: actions are still a bit messy - unsatisfactory icons (T2013 will help!)
Test Plan: viewed diffs - they look good
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2007
Differential Revision: https://secure.phabricator.com/D3904
Summary:
For text like "MMM", make the right parts of the element scroll.
Also fixed a couple of 1px issues here and there.
Test Plan: Added, viewed UIExamples.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4152
Summary:
When possible, render application transactions via Ajax. Instead of reloading the page when the response returns, append new transactions to the transaction view.
Scroll the window to the new transactions, animate them in, and clear the form to make this interaction feel reasonable.
When editing transactions, fade them in but do not scroll to them (i.e., don't disrupt the user's position).
Test Plan: Edited and appended transactions via Ajax. Observed fade in animations and scroll behavior. Clicked anchors to verify proper anchor accounting.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4151
Summary: When possible, replace the edited or deleted transaction inline using Ajax.
Test Plan: Repeatedly edited and deleted transactions. Clicked their anchors to verify anchors were correctly preserved.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4150
Summary:
Allows you to edit or delete comments in appplications which support ApplicationTransactions.
UI/UX stuff:
- The dialogs are rough but I want to do a dialog design pass more generally, @chad has some mocks.
- When you add new mentions via edit, they don't currently count as mentions. I'm not sure what I want to do about this.
- When you edit or delete a comment, we do not publish any notifications about it. I think this is reasonable.
- I didn't separate "delete" out versus "edit"; I assume it will be reasonably intuitive that deleting all the text deletes effectively deletes the comment. I also want to discourage deletion somewhat (we still show the transaction, just show that the comment has been deleted).
Test Plan:
Transaction view, note "Edit" and "Edited" links:
{F26914}
Edit view, has some design issues but I want to do a pass on dialogs in general:
{F26915}
History view:
{F26913}
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4149
Summary: Aligns more styles to the `diff_full_view.png` mock.
Test Plan: {F26859}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4142
Summary:
Suhosin has about 50 options for filtering input variables, doucmented here:
http://www.hardened-php.net/suhosin/configuration.html
The default behavior of Suhosin is to drop the variable entirely if it violates any of the rules, then continue with the request. It doesn't affect 'php://input' and doesn't drop other variables, so it evades existing detection, and we can't figure out that it's happened at runtime. We could add blanket checks (Suhosin enabled + suhosin.filter.action set to nothing means this may happen, and will be undetectable if it does happen) but can't tailor a check or recovery to this specific problem.
Instead, raise a better error in the specific case where we encounter this, which is Conduit calls of "arc diff" of files over 1MB (the default POST limit). In these cases, Suhosin drops the variable entirely. If there is no 'params', scream. We never encounter this case normall (`arc`, including `arc call-conduit`, always sends this parameter) although other clients might omit it. The only exception is the web console with `conduit.ping`, which submits nothing; make it submit something so it keeps working.
See also https://github.com/facebook/phabricator/issues/233#issuecomment-11186074
Test Plan: Brought up a Debian + Suhosin box, verified the behavior of Suhosin, made requests with and without 'params'.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4144
Summary: Publish feed stories, including from Pholio. Actual stories are somewhat garbage but it's all display-time; I'm going to do a pass on feed in general.
Test Plan: {F26832}
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4140
Summary:
- Adds mail support to the generic transaction construct.
- Restores mail support to Pholio (now much improved; the mails are actually useful).
Test Plan: Updated a Pholio mock, got mail.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4139
Summary: I got rid of the "#4" and just linked the timestamps.
Test Plan: {F26826}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4138
Summary:
Split Pholio's transaction implementation into generic and application-specific parts. Moves us toward generic transactions, with support for:
- Editing and deleting comments.
- Setting visibility of individual comments (I'm not a fan of this feature but we'll see).
I want to move everything to a more generic piece of infrastructure but there's very little they can share right now so adding transactions to, e.g., Paste or Macros (T2157) means massive amounts of similar code.
Tons of work left to do here, but I think it basically works. Here's a screenshot:
{F26820}
Test Plan: Made transactions in Pholio.
Reviewers: btrahan, vrana, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4136
Summary: This is to reduce number of calls from Arcanist.
Test Plan: Called it from web interface.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4146
