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
Summary: Minor issue from D4117, user doesn't get passed down so inline comments are missing their "reply" link.
Test Plan: Looked at Differential, saw reply link.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4145
Summary: The dark highlight was bothering me, I'm assuming it probably bothered others but hadn't come up yet.
Test Plan: Hover reload hover reload hover.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4143
Summary:
Currently, code blocks inside Phabricator remarkup are constrained to ~80col. There's no technical or design reason for this -- it looks and works fine without the restriction. Join the new world of fluid widths.
(Note that //inline// comments can get a bit whacky in some cases with large code blocks, but we have no restrictions there currently, so this doesn't break anything.)
Test Plan: {F26814}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4135
Summary:
Support SMTP as the mailer and user could turn on SMTP authentication if needed.
Import PHPMailer as PHPMailerLite doesn't support SMTP.
Make class PhabricatorMailImplementationPHPMailerAdapter final.
Test Plan: N/A
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2139
Differential Revision: https://secure.phabricator.com/D4063
Summary:
D4117 (which is otherwise awesome :)) requires you to `setMarkupEngine()` and
Phriction's diff rendering wasn't changed to call that with a
`PhabricatorMarkupEngine`.
Test Plan: Went to a Phriction diff page and saw it render correctly.
Reviewers: epriestley, btrahan, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4132
Summary: This updates spacing and colors on the calendar page.
Test Plan: View calendar, make an event, view it again.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4130
