Summary: Ref T7199. Convert Differential to modern modular commands.
Test Plan: Used `bin/mail receive-test` to send command and comment mail to Differential.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12239
Summary: Ref T7199. Everyone can have a mail command! You can have a mail command! You can have a mail command! Mail commands for everyone!
Test Plan: Used `bin/mail receive-test` to issue commands against files and pastes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12238
Summary: Ref T7199. Half of these aren't even reachable, but make some progress toward reducing the amount of nonsense and garbage in mail handling.
Test Plan: Tested all reachable handlers with `bin/mail receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12237
Summary:
Ref T7199. Essentially all of the reply handlers now apply transactions to something which implements PhabricatorApplicationTransactionInterface.
We can share code between them by lifting this stuff into a superclass.
First, convert paste. Also rename `PasteMockMailReceiver` to `PasteMailReceiver` (this got mis-copied from Pholio at some point, I think).
Test Plan: Used `bin/mail receive-test` to send comments + `!unsubscribe` to pastes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12236
Summary:
Ref T7199. Ref T7712. This improves the file rules for email:
- Embed visible images as thumbnails.
- Put all other file types in a nice list.
This "fixes" an issue caused by the opposite of the problem described in T7712 -- files being dropped if the default ruleset is too restrictive. T7712 is the real solution here, but use a half-measure for now.
Test Plan:
- Sent mail with two non-images and two images.
- Got a nice list of non-images and embeds of images.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7712, T7199
Differential Revision: https://secure.phabricator.com/D12235
Summary: Fixes T7721. Looks like this got renamed at some point but skipped. Also, change the true / false idx look ups to correct integers here since there's a warning in the error logs in this case without it.
Test Plan: made new user. loaded up conpherence and it worked. tried all the links too for good measure and they worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7717, T7721
Differential Revision: https://secure.phabricator.com/D12243
Summary:
Ref T7199. Two notable changes:
- Process multiple commands.
- Process commands when creating //or// updating a task.
And generally clean things up a bit.
Test Plan:
- Used `receive-test` to execute all commands for new tasks.
- Used `receive-test` to execute all commands for existing tasks.
- Used a combination of commands to produce varied effects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12234
Summary:
Ref T7199. In the vein of D12231, these options were a bad idea.
- They once served a very narrow, Facebook-specific need (see T1992), except even Facebook only used the Differential setting AFAIK.
- Outside of that special case, they are unused and essentially unusable (generally speaking, they do not meaningfully implement anything modular or replaceable).
- I have no knowledge of any install ever changing these settings, and can imagine no reason why they would.
Moving forward:
- If they really need to, they can fork locally and chagne one line.
- I expect "!actions" to make mail at least somewhat more modular soon, anyway.
- Any derived handlers would break after T7199 and need to be rewritten anyway, so this is just taking advantage of a BC break to do cleanup.
Test Plan:
- Grepped for removed configuration.
- Sent some mail from applications, verified the reply handlers set proper reply addresses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12233
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.
- There is zero reason to ever set these to different things.
- Unsurprisingly, I don't know of any install which has them set to different things.
Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.
We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.
Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12231
Summary:
Ref T7199. This prepares for an exciting new world of more powerful "!action" commands. In particular:
- We parse multiple commands per mail.
- We parse command arguments (these are currently not used).
- We parse commands at the beginning or end of mail.
Additionally:
- Do a quick modernization pass on all handlers.
- Break legacy compatibility with really hacky Facebook stuff (see T1992). They've theoretically been on notice for a year and a half, and their setup relies on calling very old reply handler APIs directly.
- Some of these handlers had some copy/paste fluff.
- The Releeph handler is unreachable, but fix it //in theory//.
Test Plan:
- Sent mail to a file; used "!unsubscribe".
- Sent mail to a legalpad document; used "!unsubscribe".
- Sent mail to a task; used various "!close", "!claim", "!assign", etc.
- Sent mail to a paste.
- Sent mail to a revision; used various "!reject", "!claim", etc.
- Tried to send mail to a pull request but it's not actually reachable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12230
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary: Ref T7199. Guess no one has ever tried to reply to file mail.
Test Plan: Used `bin/mail receive-test` to send mail to files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12228
Summary: if users have no conpherences I think this is possible? just init it with the rest of the variables we init when we have no conpherence and we should be good to go. fixes T7671.
Test Plan: logicypoo
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7671
Differential Revision: https://secure.phabricator.com/D12227
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.
Test Plan:
- Sent a new message; used "To".
- I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
- OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^
Did not test:
- Lint is nontrivial to test locally, I'll test it in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T7689
Differential Revision: https://secure.phabricator.com/D12224
Summary:
Ref T7689. This serves two goals:
- I want to remove Controller->loadViewerHandles(). A nontrivial number of these callsites are loading handles to pass to tokenizers. Since tokenizers need to take strings eventually anyway, we can do less work by letting them take PHIDs now.
- A few changes out, I want tokenizers to accept parameterized tokens (like `viewer()`, `members(differential)`, etc.), so the `setValues()` signature needs to change eventually anyway.
I made this work and converted a handful of callsites as an example; upcoming changes will convert more.
Test Plan:
- Viewed Almanac binding editor; used "Interface".
- Edited Almanac services; used "Projects".
- Edited Almanac devices; used "Projects".
- Searched for commits; used "Auditors"; "Commit Authors", "Repositories".
- Searched for calendar events; used "Created By"; used "Invited".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12218
Summary: D12222 did an evil thing. This correctly scopes this to just the threads you are participating in and not all threads which then get filtered from policy later, super slowly...
Test Plan: still 'works' and probably wont kill prod
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12223
Summary:
Ref T7566. Prior to this diff, we had a broken mess in the "Messages" section. Now, "Messages" behave like rooms in that whatever is loaded at page load time is at the top of the list.
Additionally, refine "show more" behavior such that it simply shows the next X, but if there exists X + 1 then we have another "show more" that kicks you to application search. Theoretically, there are still corner cases where users are in a ton of rooms or a ton of messages respectively, but this feels pretty good.
Consolidates title rendering code so we always render the list of participants and no more "No Title".
Also remove the policy icons for messages consistently, helping to differentiate them from rooms at a glance.
Test Plan: clicked around in conpherence main - looked good. tried "show more" and it worked! played around in durable column and things seemed reasonable there too.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7566
Differential Revision: https://secure.phabricator.com/D12222
Summary: Previously, this text was transparent, moved it to a new span and set visibility on it.
Test Plan: review search in FF, Chrome. Text no longer displays.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7710
Differential Revision: https://secure.phabricator.com/D12219
Summary:
Ref T7689. This gives HandleLists `renderList()` and `renderHandle()` methods, which return views that can perform just-in-time data fetching and generally look and feel like other rendering code, instead of being odd pseudo-functional methods on `Controller`.
Also converts callsites on the Maniphest detail page to use these methods.
Next changes will wipe out more of the callsites.
Test Plan:
- Viewed Maniphest detail page with many relevant handles.
- Created a new subtask.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12205
Summary:
Ref T7689, which discusses some of the motivation here. Briefly, these methods are awkward:
- Controller->loadHandles()
- Controller->loadViewerHandles()
- Controller->renderHandlesForPHIDs()
This moves us toward better semantics, less awkwardness, and a more reasonable attack on T7688 which won't double-fetch a bunch of data.
Test Plan:
- Added unit tests.
- Converted one controller to the new stuff.
- Viewed countdown lists, saw handles render.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12202
Summary: pebkac issue of some sort and I didn't actually commit removing the defunct /conpherence/room/ uri route
Test Plan: made a new room and it worked
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12216
Summary: Ref T7584. In Conpherence main view, this adds a "search" link right in the "Rooms" header. This piece addresses an outstanding item on T7584. This diff also adds a search button in the durable column that takes you to the application search. This kind of a big product bet that rooms are going to be dominating things and its most useful to find another room quickly from this view. That said, I think the application search should get massaged slightly to allow searching threads and this won't be much of a trade off at all.
Test Plan: verified new search links took me to correct place and displayed reasonably.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7584
Differential Revision: https://secure.phabricator.com/D12215
Summary: Ref T7061. Quicksand still needs an ajax-style response here.
Test Plan: Clicked a file detail page (this redirects) with column open, ended up in the right place.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7061
Differential Revision: https://secure.phabricator.com/D12206
Summary: Swaps out search icon for font-awesome. Tweaks colors a bit for different color headers.
Test Plan: Test a number of colors in the search box. Click on button and still go to advanced search.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12200
Summary:
Fixes T7061. Although it's very simple, I think this is a complete fix.
Quicksand technically is Ajax and uses Workflow as a transport mechanism, but the server should always pretend the user clicked a normal link when rendering.
Test Plan: Links that were autoconverting into dialogs (like "Edit Task") or otherwise making the wrong behavioral choices now work as expected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7061
Differential Revision: https://secure.phabricator.com/D12194
Summary:
Fixes T7063. Fixes T7059. We were running some extra unnecessary behaviors. In particular, we would render the menu but not actually send it over the wire, so some behaviors would execute, fail to find the nodes they expected, and throw. This cascaded into some other failures.
Do less work and don't activate behaviors which won't be able to run.
Test Plan: Clicked around, no more errors on page transition. Comment previews work.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7059, T7063
Differential Revision: https://secure.phabricator.com/D12188
Summary:
Ref T1460. Overall:
- Pass `objectOwnerPHID` consistently.
- Pass viewer consistently.
- Set the correct draft state for checkboxes on the client.
Test Plan:
- Made inline comments in Differential.
- Made inline comments in Diffusion.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12186
Summary:
Rebuilds the UI in Differential commenting. Specifically we look at the following design patterns:
**To the author:**
- The author of the diff should be able to easily identify what comments are done and not done.
- We keep undone comments yellow
- Clicking done turns comment block into 'unsubmitted state'
**To the reviewer:**
- Easier understanding of unsubmitted states
- All conversations to be yellow/important
**Todo**
- Not all color CSS states correct
- Unpulished checkbox support
Test Plan:
Test leaving comments, published and unpublished. Checking Done, unpublished and published. Check delete states.
From the Diff Author's perspective:
{F352094}
For a Diff commenter's perspective:
{F352095}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1460, T7660, T7503
Differential Revision: https://secure.phabricator.com/D12171
Summary:
This returns the PHID of the current revision owner, or the commit author, if one exists.
NOTE: For drafts, we currently return `null`; I'll fix that in a future change. Should be correct for submitted comments.
Test Plan: Added an inline, nothing seemed broken.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12185
Summary: Fixes T7648. The existence of this file is not necessarily obvious and is important to backup, transfer, and restore.
Test Plan: Read documentation.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7648
Differential Revision: https://secure.phabricator.com/D12183
Summary: Fixes T7675. epriestley caught this in code review and I didn't implement it all the way
Test Plan: logicypoo
Reviewers: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7675
Differential Revision: https://secure.phabricator.com/D12181
Summary: Fixes T7672. This had two `%d` conversions but only one parameter.
Test Plan: Adjusted limit to 0, viewed a merge, saw proper message.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7672
Differential Revision: https://secure.phabricator.com/D12180
Summary: Ref T7670. Add a few unit tests to make sure deleting everyone works. Also change remaining processRequest to handleRequest while in there.
Test Plan: `arc unit` passed
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7670
Differential Revision: https://secure.phabricator.com/D12179
Summary:
Ref T7566. This does a big chunk of what's left
- Main view
- "Rooms" sub header
- 5 Rooms shown at a time, with room you're looking at in the top on page load
- e.g. viewing /conpherence/x/ the room x is at top always
- solves corner case of when you have yet to "join" the room
- "See More" link takes you to application search for rooms you have participated in
- if no rooms, there is a "Create Room" and "Find Rooms" links.
- "Messages" sub header
- same as before
- policy icons showing up in the menu
- Durable column view - still just the latest N, no changes really there
- Transactions - special cased rendering to try to say room vs thread as appropos
- Bug fix - we weren't recording the initial participants transaction post D12177 / D12163. This fixes that.
Should probably test pagination, and if you want to show more than 5 rooms of have it behave more like messages (where you can wind up in the middle of a paginated list) that will be more work. Also, if lots of messages / rooms (100 is the limit) we might not display rooms if we're supposed to. Yay whale usage! :D
Test Plan: made a new room - success. made a new message - success. viewed a room from /conpherenece/room/ i wasn't a participant in and noted it showed up at the top of the five rooms. clicked around rooms and stuff loaded nicely.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7566
Differential Revision: https://secure.phabricator.com/D12178
Summary: Fixes T7669. Broken by D12163 re-factoring and foolihardiness of test coverage. Notably / interestingly, this was broken before D12163 from not implementing policy correctly, so Conpherence has been broken for a bit with few reports.
Test Plan: had user send himself a message
Reviewers: chad, epriestley
Reviewed By: chad, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7669
Differential Revision: https://secure.phabricator.com/D12177
Summary: Moving to an rgba color here to work better with all the various header colors.
Test Plan: Reload sandbox, see new icon color.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12176
Summary: This is just a quick pass to fix a few bugs and spacing issues, Phortune itself could probably use some more custom UI, but that'll require some thought and abstraction. This also adds a new taller table CSS, which I mayyyy make automatic on tables with few rows, we'll see.
Test Plan: Browsed my Phortune account, tested new spacing on `admin` for 'full effect'
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12115
Summary: Fixes T7665. Unfortunately when the user messes this up its not easy to partially recover so we just reset that time to the default.
Test Plan: set time to "00:00 AM" and got a sensible error.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7665
Differential Revision: https://secure.phabricator.com/D12174
Summary: Fixes T7655. We'll set tighter spacing around edit clusters. Also darkened up the date marker and remove unused `phabricator-transaction-view` CSS that was still scattered around the site.
Test Plan: Test a full and column multi-edit spam. Visited Ponder and Diffusion, noticed no issues using those apps. Grepped for other users of `phabricator-transaction-view`
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T7655
Differential Revision: https://secure.phabricator.com/D12148
Summary:
Fixes T7629 plus an un filed bug that's breaking creating new threads since we need to add participants EVEN EARLIER than we were doing it now that policy is actually enforced.
Back to the main thrust of this, there is one UI corner case - in the main view if you go from 1:1 to 1:1:1 (i.e. add a 3rd recipient, or Nth in a row) the icon only updates on page reload. I figure this will get sorted out at a later refactor as we make the client better / share more code with durable column.
One other small behavioral oddity is in the main view sometime we start loading with no conpherence. in that case, rather than show some incorrect icon, we show no icon (and "no title") and then things change at load. Seems okay-ish.
Finally, @chad - the CSS is a very work-man-like "use the built in stuff you can specify from PHP" so I'm sure it needs some love.
Test Plan: made all sorts of rooms and threads and liked the icons. noted smooth loading action as i switched around
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, chad, epriestley
Maniphest Tasks: T7629
Differential Revision: https://secure.phabricator.com/D12163
Summary:
Ref T6755. In Git and Subversion, running `git clone http://google.com/` or `svn checkout http://google.com/` does not echo the response body.
In Mercurial, it does. Censor it from the output of `hg pull` and `hg clone`. This prevents an attacker from:
- Creating a Mercurial remote repository with URI `http://10.0.0.1/secrets/`; and
- reading the secrets out of the error message after the clone fails.
Test Plan: Set a Mercurial remote URI to a non-Mercurial repository, ran `repository update`, saw censored error message.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12170
Summary: Ref T6755. I'll add some notes there about specifics.
Test Plan:
- Made connections to HTTP and HTTPS URIs.
- Added some debugging code to verify that HTTP URIs were pre-resolved.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12169
Summary:
Ref T6755. Although we do not return response bodies, it is possible to perform crude portscanning if you can execute a DNS rebinding attack (which, for now, remains theoretical).
Limit users to 60 requests / hour to make it less feasible. This would require ~30 years to portscan all ports on a `/32` netblock.
Users who can guess that services may exist can confirm their existence more quickly than this, but if the attacker already had a very small set of candidate services it seems unlikely that portscanning would be of much use in executing the attack.
This protection should eventually be applied to T4190, too (that task also has other considerations).
Test Plan: Set rate limit very low, hit rate limit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12168
Summary:
Fixes T7602. This is similar to the existing behavior for "changes planned" and "needs revision" revisions.
Also fix the "Update Diff" workflow so it correctly selects closed revisions as attachable.
Test Plan: Updated an abandoned revision, saw it change to "Needs Review".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7602
Differential Revision: https://secure.phabricator.com/D12167
Summary:
Fixes T7664. When there are a large number of tasks (400+) with the same subpriority (which can happen if the subpriority features are rarely used), it may take more than 30 seconds to rebalance them.
Make the algorithm more aggressive about rebalancing homogenous blocks of tasks.
This may need to get even fancier, but I'd guess it can process blocks 1-2 orders of magnitude larger, which should be ~all installs.
(If someone still hits issues with this, I'll make it fancier.)
Once a block is rebalanced, it doesn't need to be rebalanced again (at least, not as a whole block) so we basically just need to get over the initial hurdle here and then we're good.
In the worst case, we can provide `bin/maniphest rebalance` or similar and do the rebalance step offline.
And, in any case, we have more test coverage here now.
Test Plan:
- Existing tests.
- New tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7664
Differential Revision: https://secure.phabricator.com/D12166
Summary: Enables a basic tooltip when using icon buttons and a convenience method for setting an icon.
Test Plan: Built a UIExample.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12172
Summary: Ref T1460, this adds additional buttons colors and styles for use in inline comments. Will also backport to Calendar and PHUIInfoView
Test Plan:
Review new buttons and hover states in UI Examples.
{F350549}
{F350550}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12162
Summary: Fixes T7586. If you can't edit a room, the pertinent UI is greyed out. One exception is the title of the room in the full viewer; this crumb is not disabled as it would be hard to read. Otherwise though, everything is disabled nicely.
Test Plan: tried to add participants when I wasn't allowed to and got an error. added participants otherwise okay. tried to edit title when i wasn't allowed and got an error. otherwise okay. left conpherence threads / rooms successfully.
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7586
Differential Revision: https://secure.phabricator.com/D12161
Summary: Ref T7660. I'm not toggling "inline-state-is-draft" correctly in JS yet since it's a little tricky (you can reload to see it) but the main state should work.
Test Plan:
- Clicked "done", saw comment opacity fade with placeholder style.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7660
Differential Revision: https://secure.phabricator.com/D12160
Summary: Fixes T5658. Over a long period of time, some cruft can build up here. Only show revisions which have been updated in the last 30 days.
Test Plan:
- Viewed panel in Differential and Diffusion.
- Changed limit from 30 days to 30 seconds and saw no revisions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5658
Differential Revision: https://secure.phabricator.com/D12158
Summary:
Ref T6755. This mitigates an attack where you:
- compromise an administrative account;
- configure "text/plain" as an "image" MIME type; and
- create a new macro sourced from a sensitive resource which is locally accessible over HTTP GET, using DNS rebinding.
You can then view the content of the resource in Files. By preventing the compromised account from reconfiguring the MIME types, the server will instead destroy the response and prevent the attacker from seeing it.
In general, these options should change very rarely, and they often sit just beyond the edge of security vulnerabilities anyway.
For example, if you ignore the warnings about an alternate file domain and elect to serve content from the primary domain, it's still somewhat difficult for an attacker to exploit the vulnerability. If they can add "text/html" or "image/svg+xml" as image MIME types, it becomes trivial. In this case not having an alternate domain is the main issue, but easy modification of this config increases risk/exposure.
Test Plan: Viewed affected config and saw that it is locked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12154
Summary: Fixes T6378.
Test Plan: Set config to `/.*/`, created a new diff, everything was collapsed as generated.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6378
Differential Revision: https://secure.phabricator.com/D12159
Summary:
Ref T6755. This improves our resistance to SSRF attacks:
- Follow redirects manually and verify each component of the redirect chain.
- Handle authentication provider profile picture fetches more strictly.
Test Plan:
- Tried to download macros from various URIs which issued redirects, etc.
- Downloaded an actual macro.
- Went through external account workflow.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12151
Summary: This signature changed at some point after I tested things and I didn't catch it.
Test Plan: Destroyed a chunked large file with `bin/remove`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12152
Summary:
Ref T7585. This implements everything specified, with a few caveats
- since rooms you have yet to join can't be viewed in the column yet, the column view has some bugs and isn't expected to work.
- the room you're looking at is just pre-pending to the top of the "recent" list
Test Plan: made a room that no one could join. verified when viewing that there was no comment ui. made a room that others could join. verified folks who had yet to join had a "join" button with an area for text. tried joining with / without message text and it worked in both cases
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7585
Differential Revision: https://secure.phabricator.com/D12149
Summary:
Ref T1266. We won't detect a move/copy if fewer than 3 lines are changed.
However, you may move a block like:
Complicated Line A
Trivial Line B
Complicated Line C
...where "Trivial Line B" is something like a curly brace. If you move this block somewhere that happened to previously have a similar trivial curly brace line, we won't be able to find 3 contiguous added lines in order to detect the copy/move.
Instead, consider both changed and unchanged lines when trying to find contiguous blocks. This allows us to detect across gaps where lines were not actually changed.
This new algorithm may be too liberal (for example, we may end up incorrectly identifying moved/copied code before or after changed lines, not just between changed lines), but we can keep an eye on it and tweak it. The algorithm is better factored and better covered, now.
Test Plan:
- Added a unit test for this case.
- Spot-checked a handful of diffs and generally saw behavior that made sense and looked better than before.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1266
Differential Revision: https://secure.phabricator.com/D12146
Summary:
Ref T1266. This doesn't change any behaviors, but some of this code has a lot of really complicated conditionals and I tried to break that up a bit.
Also, reexpress this stuff in terms of the "structured" parser in D12144.
Test Plan: Unit tests still pass. They aren't hugely comprehensive but did reliably fail when I screwed stuff up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1266
Differential Revision: https://secure.phabricator.com/D12145
Summary:
Ref T1266. This prepares to fix case (2) on T1266 by improving the robustness of hunk parsing.
In particular, the copy detection code abuses this API because it isn't currently expressive or flexible enough.
Make it more flexible and cover it exhaustively.
I'll move callsites to the new stuff in upcoming revisions.
Test Plan: Unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1266
Differential Revision: https://secure.phabricator.com/D12144
Summary: Fixes T7578. This was pretty easy because conpherence funnels all transacton stuff through this spot
Test Plan: made a new room so only my user was a participant. wrote "@myself will work and @anotherguy will be greyed out" and so it was as expected
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7578
Differential Revision: https://secure.phabricator.com/D12114
Summary: Ref T7584. This hits all the major bullets there. Next step on T7584 is figuring out how it integrates into the full UI and column UI. That said, this is a bit buggy feeling right now since Conpherence as is assumes you are a participant all over the place and rooms make no such assumption. I'll probably this bit up next.
Test Plan:
viewed /conpherence/room/ and saw stuff. viewed the "participant" query as two different users and saw different correct result sets. made a room via the button and it worked. tried to view a room I wasn't a participant in and it failed horribly, which is something to fix in a future diff
created a thread via "send message" on a user profile and it worked
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7584
Differential Revision: https://secure.phabricator.com/D12113
Summary: Fixes T7377. We don't expand projects into members when sending notifications right now. Instead, expand them.
Test Plan:
- Added a project as a reviewer to a revision, made a comment, saw project members receive a read notification + email (with appropriate preferences).
- There's meaningful test coverage on the core mail stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7377
Differential Revision: https://secure.phabricator.com/D12142
Summary:
Ref T5644. Ref T7472. Currently, we highlight each line of pattern search results in Diffusion.
- This is incredibly slow for non-PHP languages which need to shell out to Pygments.
- A lot of this highlighting isn't very useful anyway, because it doesn't have any context.
Instead, try to highlight pattern matches but don't highlight the source itself.
Test Plan: {F349637}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7472, T5644
Differential Revision: https://secure.phabricator.com/D12141
Summary:
Ref T5644. See some discussion in D8040.
When a file is very large (more than 64KB of text), don't activate syntax highlighting by default. This should prevent us from wasting resources running `pygmentize` on enormous files.
Users who want the file highlighted can still select "Highlight As...".
The tricky part of this diff is separating the headers into "changeset" headers and "undershield" (rendering) headers. Specifically, a file might have these headers/shields:
- "This file is newly added."
- "This file is generated. Show Changes"
- "Highlighting is disabled for this large file."
In this case, I want the user to see "added" and "generated" when they load the page, and only see "highlighting disabled" after they click "Show Changes". So there are several categories:
- "Changeset" headers, which discuss the changeset as a whole (binary file, image file, moved, added, deleted, etc.)
- "Property" headers, which describe metadata changes (not relevant here).
- "Shields", which hide files from view by default.
- "Undershield" headers, which provide rendering information that is only relevant if there is no shield on the file.
Test Plan:
- Viewed a diff with the library map, clicked "show changes", got a "highlighting disabled" header back with highlighting disabled.
- Enabled highlighting explicitly (this currently restores the shield, which it probably shouldn't, but that feels out of scope for this change). The deshielded file is highlighted per the user's request.
- Loaded context on normal files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T5644
Differential Revision: https://secure.phabricator.com/D12132
Summary:
Ref T1460. Ref T6403. Replace `Diffusion::INLINEDONE` with `Transactions::INLINESTATE` and generalize things enough that we can lift it into core.
The next change will lift Differential's similar implementation into the core.
Also start implementing a fix for T6403, providing an alternate hook for optional builtin transactions.
Test Plan: Changed inline state in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6403, T1460
Differential Revision: https://secure.phabricator.com/D12129
Summary:
Ref T1460. See D12126. This is essentially the same change, but for Diffusion.
This is a bit copy/pastey. I'm going to make an effort to lift inline handling into the core before pushing this in, so hopefully that will clean things up a bit.
Test Plan: Submitted stuff in Diffusion and got checkmarks to publish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12128
Summary:
Ref T1460. When a revision author updates/comments/etc on a revision, publish all their checkmarks.
This doesn't handle Diffusion/audits yet.
Test Plan: {F346870}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12126
Summary:
Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc.
Specifically, these are the behaviors:
- You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider..").
- You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff.
- When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold:
- Be consistent with how inlines work.
- Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories.
- The actual bit where done-ness publishes isn't implemented.
- UI is bare bones.
- No integration with the rest of the UI yet.
Test Plan: Clicked some checkboxes.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: paulshen, chasemp, epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12033
Summary:
Fixes T1102. If you don't use `arc`, the web workflow requires some extra needless steps when updating diffs.
Provide a more streamlined "Update Diff" workflow.
Test Plan: {F347750}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1102
Differential Revision: https://secure.phabricator.com/D12131
Summary:
Ref T6755. This is a partial fix, but:
- Allow netblocks to be blacklisted instead of making the feature all-or-nothing.
- Default to disallow requests to all reserved private/local/special IP blocks. This should generally be a "safe" setting.
- Explain the risks better.
- Improve the errors rasied by Macro when failing.
- Removed `security.allow-outbound-http`, as it is superseded by this setting and is somewhat misleading.
- We still make outbound HTTP requests to OAuth.
- We still make outbound HTTP requests for repositories.
From a technical perspective:
- Separate URIs that are safe to link to or redirect to (basically, not "javascript://") from URIs that are safe to fetch (nothing in a private block).
- Add the default blacklist.
- Be more careful with response data in Macro fetching, and don't let the user see it if it isn't ultimately valid.
Additionally:
- I want to do this check before pulling repositories, but that's enough of a mess that it should go in a separate diff.
- The future implementation of T4190 needs to perform the fetch check.
Test Plan:
- Fetched a valid macro.
- Fetched a non-image, verified it didn't result in a viewable file.
- Fetched a private-ip-space image, got an error.
- Fetched a 404, got a useful-enough error without additional revealing response content (which is usually HTML anyway and not useful).
- Fetched a bad protocol, got an error.
- Linked to a local resource, a phriction page, a valid remote site, all worked.
- Linked to private IP space, which worked fine (we want to let you link and redierect to other private services, just not fetch them).
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12136
Summary: Ref T7627. This centralizes this transaction construction code so the unit tests and Instances can both use it.
Test Plan: See D12116.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7627
Differential Revision: https://secure.phabricator.com/D12118
Summary: Fixes T7625. The way the regexp worked, "unusual" terminal characters required at least one character as a prefix in order to match. Allow 0 instead, so `#a1` matches.
Test Plan: Added and executed unit test.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7625
Differential Revision: https://secure.phabricator.com/D12123
Summary: Ref T7611. This should let us figure out the root cause, hopefully.
Test Plan: iiam
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7611
Differential Revision: https://secure.phabricator.com/D12124
Summary:
Fixes T7485. Before applications had proper policies, we gated access by requiring the viewer be an administrator.
This is now redundant (CAN_EDIT on applications has the same effect, and performs the same check), and may some day be wrong (we might let administrators configure a different policy to control who can configure applications). Today, it gets the policy dialog wrong.
Test Plan:
Clicked "Edit Policies" as a non-administrator, was unable to, got nice error:
{F346598}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7485
Differential Revision: https://secure.phabricator.com/D12125
Summary: Fixes T7620. I don't fully understand exactly what's going on here, but we don't actually need to call `flush()`.
Test Plan:
- Put timing code around the `echo`.
- Made a fake page that emitted a lot of data.
- Saw the `echo` block proportionate to data size under `curl --limit-rate ...`.
- See T7620.
- Downloaded a large file, got a reasonable progress bar and no obvious memory use issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: jlarouche, rbalik, epriestley
Maniphest Tasks: T7620
Differential Revision: https://secure.phabricator.com/D12127
Summary: Fixes T7647. We float both the time and description here and want the fluidity.
Test Plan: Review a new Conpherence in FF, Safari, Chrome.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7647
Differential Revision: https://secure.phabricator.com/D12137
Summary:
Fixes T7563. Fixes T5201. Reframe this as two separate operations:
- Move before or after a task.
- Move to the beginning or end of a priority.
Then:
- Make all the order queries unambiguous and properly reversible, with an explicit `id` order.
- Just reuse `ManiphestTask` to get results in the correct order.
- Simplify the actual transaction apply logic.
- Detect and recover from cases where tasks have identical or similar subpriorities.
Test Plan:
- Wrote and executed unit tests.
- Dragged and dropped tasks within priorities and between priorities in the main Maniphest view.
- Dragged and dropped tasks within priorities in the workboard view, when ordered by priority.
- Also poked at the "natural" order, but that shouldn't be affected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T5201, T7563
Differential Revision: https://secure.phabricator.com/D12121
Summary:
Via HackerOne. We aren't correctly escaping the date, so a user can XSS themselves by setting their date format creatively.
This construction is very unusual and I don't think we do anything similar elsewhere, so I can't come up with a systematic change which would prevent this in the general case.
Test Plan: Set date format to tag junk, got self-XSS before patch and proper escaping after the patch.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12117
Summary:
Fixes T7621. The engine selection code started out making sense, but didn't make as much sense by the time I was done with it.
Specifically, from the vanilla file upload, we may incorrectly try to write directly to the chunk storage engine. This is incorrect, and produces a confusing/bad error.
Make chunk storage engines explicit and don't try to do single-file one-shot writes to them.
Test Plan:
- Tried to upload a large file with vanilla uploader, got better error message.
- Uploaded small and large files with drag and drop.
- Viewed {nav Files > Help/Options}.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7621
Differential Revision: https://secure.phabricator.com/D12110
Summary:
Ref T7522. This is mostly useful in the cluster, but could be useful for external installs too.
If you want to import an instance into a test/dry-run state (in the cluster, to test an import; in the general case, to do something like test new hardware or configuration), you currently risk spamming users with a lot of duplicate notifications. In particular, if Phabricator tracks remotes, both instances will continue importing commits and sending email about them. Both instances will try to publish to mirrors, too, which could be bad news, and both instances will try to update linked services.
Instead, provide a flag to let an instance run in "silent mode", which disables all outbound messaging and data.
We need to remember to support this flag on any new outbound channels, but we add about one of those per year so I think that's reasonable.
Test Plan:
- Flipped config.
- Saw it void email, feed and mirroring.
- Didn't test SMS since it's not really in use yet and not convenient to test.
- (Can you think of any publishing I missed?)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7522
Differential Revision: https://secure.phabricator.com/D12109
Summary: Fixes T7618. The "button" needs to be a PHUIButtonView later on.
Test Plan: Forced condition, loaded page, saw button instead of fatal.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7618
Differential Revision: https://secure.phabricator.com/D12108
Summary:
Indigo apparently used to be pink and was fixed in D10707.
This looks like it never got changed
Test Plan: Flag was purple, then it was pink
Reviewers: chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: epriestley, #flags
Differential Revision: https://secure.phabricator.com/D12101
Summary:
Ref T7149. When users give us dumpfiles for import, they will almost inevitably use the `phabricator` namespace. They need to be renamed to use an instance namespace.
We can do this either by:
- importing the data first, then renaming; or
- renaming first, then importing.
This implements the second one, basically `storage renamespace --in dump.sql --from phabricator --to instancename > instance.sql`.
Renaming first is a little hackier since we have to `preg_match()` a SQL dump file, but I think it's better overall:
- With only one database, it lets you dump/import without downtime.
- If you have development stuff in a development environment in the `phabricator` namespace, you don't have to move it aside to do an import.
- No possibility that two people doing an import at the same time on the same box will collide with each other.
- You can do the rename once and then repeat the import process with the renamed dump more easily.
- No tricky stuff with modern Phabricator running against an old dump and the database names not matching up.
None of this is super important, but it just makes large dumps a bit easier to work with, and the dumpfile format is regular enough that this seems unlikely to ever really not work.
Test Plan: Renamespaced a dump, did a `diff -u`, saw all the relevant parts changed (and only those parts changed).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12105
Summary: Ref T7582. Also adds the basic logic for "rooms" implementation. Also makes sure we use the initializeNewThread method as appropriate.
Test Plan: made a new conpherence and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7582
Differential Revision: https://secure.phabricator.com/D12103
Summary: Fixes T7583. We also add `key_room`, which uses isRoom and dateModified since a very common view of rooms is going to be ordered by last updated.
Test Plan: made the conpherence view controller query specify `withIsRoom(true)` and `withIsRoom(false)`. The former made the controller correctly 404 while the latter had no change in functionality.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7583
Differential Revision: https://secure.phabricator.com/D12102
Summary:
Ref T7607. Ref T7522.
- For the import tools, I want to send from "Phacility Support <support@phacility.com>".
- In the general case, I want to send billing mail from merchants (T7607) later on.
Test Plan: Sent an email and saw the desired "From" address.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7607, T7522
Differential Revision: https://secure.phabricator.com/D12100
Summary:
Ref T7522. This seems like the least-bad approach to a messy issue:
- When backfilling accounts from an imported instance, I need to write ExternalAccount rows to the instance to link instance accounts with upstream accounts.
- We do this in the daemons in some other cases, which lets us run all the code in the context of the instance. However, I really want to do this in-process here because it's way way simpler and we need to do writes to //both// the instance and the upstream, and they're interleaved, and they depend on one another.
- I can hard-code the query with `qsprintf()` but that feels like 100x worse than this.
This allows me to do this:
```
id(new PhabricatorExternalAccount())
->setForcedConnnection($instance_conn)
->...
->save();
```
...and get a write to the instance database, which is at least not completely a minefield.
Test Plan: Backfilled instance accounts and got interleaved instance and upstream writes as expected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7522
Differential Revision: https://secure.phabricator.com/D12098
Summary: Conpherence Full modernize pass, setting standard space and colors on all widget panels. Moved menu back to 240px as the narrow column wasn't really usefull. Removed 'subtitle' on menu, seems simpler but almost under-designed. Subtitle isn't particularly useful and I plan on adding audience icons next (single, group, project, public) so I think this is the right direction.
Test Plan:
Tested with and without number columns on the menu, test with files, calendar dates, removing participants.
{F337941}
{F337942}
{F337943}
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12078
Summary:
Ref T7149. Currently, global drag and drop always uses the most open visibility policy on the install. This was appropriate before the application preference was introduced, but default to the application preference now.
In particular, this supports a default value of "Administrators" in the Phacility cluster.
Also simplify/clean up some code.
Test Plan:
- Set application default policy to "Adminstrators".
- Uploaded file via drag-and-drop, saw "administrators" policy.
- Uploaded file via `arc upload`, saw "administrators" policy.
- Saw better URI for a text file upload after patch.
- Uploaded file via drag-and-drop-to-textarea, saw "only viewer" policy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12093
Summary: Rewrite this expression so that the second parameter to `qsprintf` is a scalar.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11913
Summary:
Fixes T6713. The idea is to keep checking what's going on in the update paths that touch the DOM. If we're doing an update or should be doing a different update, then we bail early.
This is the type of code + testing that makes me dizzy after awhile, but I think it works...
Test Plan:
added a "forceStall" parameter to the column view controller, which when specified sleeps for seconds before returning. I then augmented the JS such that the "send message" code for the durable column would specifiy this parameter.
For actual testing, I then spammed the heck out of the durable column channel and saw each message only once. I also spammed the column, switched browsers to a user on the same thread in the normal "speedy" view, sent messages there, and also only received one copy
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6713
Differential Revision: https://secure.phabricator.com/D12092
Summary:
Fixes T7545. Turns out we had the right logic to handle this basically, and just needed to variablize the CSS class that gets added / removed as appropos.
Note the new behavior is to keep the icon highlighted just with no number. This emulates how it would work if e.g. there was no unread message in the first place and you just clicked the message icon to invoke the message menu.
Test Plan: had a durable conpherence open for user A with user B. used a separate browser to send message as user B. reloaded as user A - saw new message in conpherence durable column and the "1" unread icon. I then clicked the "1" and saw it disappear as expected
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7545
Differential Revision: https://secure.phabricator.com/D12091
These parameters were set inconsistently. Use the value that the storage task uses.
Also, allow `bin/phortune invoice` to invoice in the past and future to aid testing.
Summary:
Ref T7149. We can simplify configuration somewhat by removing the upload limit setting, now that we support arbitrarily large files.
- Merge configuration documentation.
- Tell users to set things to at least 32MB. This is 8MB maximum one-shot file + 4x headroom. Chunk sizes are 4MB.
Test Plan:
- Faked all the setup warnings.
- Read documentation.
- Uploaded some files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12083
Summary: Ref T7149. This works now, so enable it.
Test Plan:
- Uploaded large and small files in Firefox, Safari and Chrome.
- Uploaded large files with `arc upload`.
- Stopped/resumed large files with all clients.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12079
Summary: Ref T7149. This was just to make testing easier, but chunking substantially works now.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12076
Summary:
Right now, if a daemon dies it can leave the setup warning around for like 10 minutes or something until we reap it.
Tighten the warning so we only care about actively running daemons.
Test Plan: Checked setup issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12088
Summary: Ref T7149. Return a real iterator from the Chunk engine, which processes chunks sequentially.
Test Plan:
This is a bit hard to read, but shows the underlying chunks being accessed one at a time and only some being accessed when requesting a range of a file:
```
$ ./bin/files cat F878 --trace --begin 100 --end 256
...
>>> [10] <query> SELECT * FROM `file_storageblob` WHERE `id` = 85
<<< [10] <query> 240 us
better software.
Phabricat>>> [11] <query> SELECT * FROM `file_storageblob` WHERE `id` = 84
<<< [11] <query> 205 us
or includes applications for:
>>> [12] <query> SELECT * FROM `file_storageblob` WHERE `id` = 83
<<< [12] <query> 226 us
- reviewing and auditing source>>> [13] <query> SELECT * FROM `file_storageblob` WHERE `id` = 82
<<< [13] <query> 203 us
code;
- hosting and browsing >>> [14] <query> SELECT * FROM `file_storageblob` WHERE `id` = 81
<<< [14] <query> 231 us
repositories;
- tracking bugs;
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12073
Summary:
Ref T7149. This still buffers the whole file, but is reaaaaal close to not doing that.
Allow Responses to be streamed, and rewrite the range stuff in the FileResponse so it does not rely on having the entire content available.
Test Plan:
- Artificially slowed down downloads, suspended/resumed them (works in chrome, not so much in Safari/Firefox?)
- Played sounds in Safari/Chrome.
- Viewed a bunch of pages and files in every browser.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12072
Summary: Ref T7149. A couple diffs down the line, this will let us emit chunked files without doing all the work up front or holding the entire file in RAM.
Test Plan:
(Some newlines added for clarity.)
```
$ ./bin/files cat F942
ABCDEFGHIJKLMNOPQRSTUVWXYZ
$ ./bin/files cat F942 --begin 1
BCDEFGHIJKLMNOPQRSTUVWXYZ
$ ./bin/files cat F942 --end 10
ABCDEFGHIJ
$ ./bin/files cat F942 --begin 3 --end 5
DE
$
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12071
Summary: Ref T7149. We can't compute hashes of large files efficiently, but we can resume uploads by the same author, with the same name and file size, which are only partially completed. This seems like a reasonable heuristic that is unlikely to ever misfire, even if it's a little magical.
Test Plan:
- Forced chunking on.
- Started uploading a chunked file.
- Closed the browser window.
- Dropped it into a new window.
- Upload resumed //(!!!)//
- Did this again.
- Downloaded the final file, which successfully reconstructed the original file.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, chad, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12070
Summary: Changes the text to just "Stay", which is still obvious what it means, with less copy. Fixes T7027
Test Plan: Now works on mobile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7027
Differential Revision: https://secure.phabricator.com/D12075
Summary:
Ref T7149. This adds chunking support to drag-and-drop uploads. It never activates right now unless you hack things up, since the chunk engine is still hard-coded as disabled.
The overall approach is the same as `arc upload` in D12061, with some slight changes to the API return values to avoid a few extra HTTP calls.
Test Plan:
- Enabled chunk engine.
- Uploaded some READMEs in a bunch of tiny 32 byte chunks.
- Worked out of the box in Safari, Chrome, Firefox.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12066
Summary:
Ref T7149. This flags allocated but incomplete files and doesn't explode when trying to download them.
Files are marked complete when the last chunk is uploaded.
I added a key on `<authorPHID, isPartial>` so we can show you a list of partially uploaded files and prompt you to resume them at some point down the road.
Test Plan: Massaged debugging settings and uploaded README.md very slowly in 32b chunks. Saw the file lose its "Partial" flag when the last chunk finished.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12063
Summary:
Ref T7149. This makes debugging some of this stuff a bit easier by removing the HTTP part in the middle.
Particularly, I anticipate having this stream data chunk-by-chunk in the near future.
Test Plan: Ran `files cat F23`, got output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12062
Summary:
Ref T7149. This isn't complete and isn't active yet, but does basically work. I'll shore it up in the next few diffs.
The new workflow goes like this:
> Client, file.allocate(): I'd like to upload a file with length L, metadata M, and hash H.
Then the server returns `upload` (a boolean) and `filePHID` (a PHID). These mean:
| upload | filePHID | means |
|---|---|---|
| false | false | Server can't accept file.
| false | true | File data already known, file created from hash.
| true | false | Just upload normally.
| true | true | Query chunks to start or resume a chunked upload.
All but the last case are uninteresting and work like exising uploads with `file.uploadhash` (which we can eventually deprecate).
In the last case:
> Client, file.querychunks(): Give me a list of chunks that I should upload.
This returns all the chunks for the file. Chunks have a start byte, an end byte, and a "complete" flag to indicate that the server already has the data.
Then, the client fills in chunks by sending them:
> Client, file.uploadchunk(): Here is the data for one chunk.
This stuff doesn't work yet or has some caveats:
- I haven't tested resume much.
- Files need an "isPartial()" flag for partial uploads, and the UI needs to respect it.
- The JS client needs to become chunk-aware.
- Chunk size is set crazy low to make testing easier.
- Some debugging flags that I'll remove soon-ish.
- Downloading works, but still streams the whole file into memory.
- This storage engine is disabled by default (hardcoded as a unit test engine) because it's still sketchy.
- Need some code to remove the "isParital" flag when the last chunk is uploaded.
- Maybe do checksumming on chunks.
Test Plan:
- Hacked up `arc upload` (see next diff) to be chunk-aware and uploaded a readme in 18 32-byte chunks. Then downloaded it. Got the same file back that I uploaded.
- File UI now shows some basic chunk info for chunked files:
{F336434}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12060
Summary: Fixes T7539. We need to set the "with-column" css class on the document body to make things like the jx-mask style-able. Also, make the global upload control only do it for the standard phabrcator page and not the document body.
Test Plan: dragged a file to conpherence column and it worked! uploaded a file to homepage with column open and it worked! uploaded a file to /file/ with column open and it worked!
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7539
Differential Revision: https://secure.phabricator.com/D12055
Summary:
Fixes T5843. File storage engines use a very old "selector" mechanism which makes them difficult to extend.
This mechanism predates widespread use of `PhutilSymbolLoader` to discover available implementations at runtime. Runtime discovery has generally proven more flexible and easier to use than explicit selection (although it sometimes needs more UI to support it in cases where order or enabled/disabled flags can not be directly determined).
Use a modern runtime discovery mechanism instead of an explicit selector. This might break any installs which subclassed the `Selector`, but I believe almost no such installs exist, and they'll receive a meaningful exception upon upgrading (any custom engines will no longer implement all of the required methods).
Looking forward, this modernizes infrastructure to prepare for new "virtual" chunked-storage engines, with the eventual goal of supporting very large file uploads and data import into the Phacility cluster.
This uses D12051 to add UI to make it easier to understand the state of storage engines.
Test Plan:
Used new UI panel to assess storage engines:
{F336270}
- Uploaded a small file, saw it go to MySQL engine.
- Uploaded a larger file, saw it go to S3 engine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5843
Differential Revision: https://secure.phabricator.com/D12053
Summary:
Ref T7149. This is a few steps away, but:
- Generally, I'd like to reduce the amount of "Config" configuration we have.
- One good way to do this is to move it into UIs in Application configuration. We did this with email recently.
- I think this was a great change and I'd like to keep moving in this direction.
- T7149 touches configuration related to file storage engines. Although I'm not planning to fully move configuration into applications yet, it would be easier to debug and test if I could drop a read-only panel there to show engines.
- So, modularize the config stuff so I can add a new panel without hard-coding it.
Test Plan:
- Added, edited, and deleted application emails.
- Viewed non-email application detail pages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12051
Summary: I left in an opacity change by mistake, and fix language on threads.
Test Plan: review in sandbox
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12054
Summary: This adds a parameter for time only on Conpherence Transactions, although grepping around, Conpherence might be the only user of this View at this point. Since we have the date markers separately, we can use just the timestamp for a cleaner feel. Also updated a bit of the spacing and colors to match Conpherence Full. Ref T7531
Test Plan:
A lot of Photoshop, and different types of chats.
{F336204}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7531
Differential Revision: https://secure.phabricator.com/D12049
Summary: I don't know the names of all the Conpherences I have ongoing and all my test icons are Psyducks. haha ha ...
Test Plan: Hover over icons, now it's all Psyduck and [No Title].
Reviewers: epriestley, btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12050
Summary: This makes macros and memes grow to 100% of their container //at most//, instead of showing a scrollbar. This is useful for overly large macros, smaller spaces like Feed and Conpherences, and Inline Comments. Fixes T7528
Test Plan: Tested a very large macro, a very large meme, and a very very tiny macro. It looks like memes get cached though, unsure if we should clean them up or just leave them
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7528
Differential Revision: https://secure.phabricator.com/D12045
Summary: Ref T7014. This got broken in today's action. For whatever reason the only way I can get the CSS to show up correctly is to move the require statement to where it was before rP5ef99dba2afc9f9ed3ca77707366a78be15f4871. Otherwise, this feature massages the UI a bit to make sure the "loading" stuff is set correctly in this state.
Test Plan: toggled conpherence open and it looked good. reloaded and it looked good.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12047
Summary: Ref T7014. This changes the title and selected icon right as the user clicks it. This could //maybe// be in the "willLoadThread" callback hook, but it doesn't happen every time we load a thread, just **this** time so keep it right in the listener for now.
Test Plan: switched some threads and liked what I saw
Reviewers: epriestley, chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12043
Summary: Ref T7014. This just makes it so there's almost no UI and a simple "You have no messages. <button>Send a message.</button>" UI
Test Plan: hacked the code such that should_404 and conpherence were false and null respectively. verified i got the right ui in the durable column. verified send a message button worked, ending up with me in main conpherence view on the right message
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12042
Summary: Numerous visual updates to the Durable Column, mostly to emulate current Conpherence look and feel.
Test Plan: Lots of little pixel chasing. Also Chrome, Firefox.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12041
Summary:
This is cool in theory, but has broken like 5 times and is broken now too. The CSS magic just isn't robust enough to keep up with CSS changes.
Just strip it out for now; if we come up with some more durable replacement we can put that back in its place.
Test Plan: Typed konami code, page didn't break horribly.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12039
Summary: When you open the column, keep it open on future requests.
Test Plan: Opened column, clicked to Conpherence (no column), clicked elsewhere (column again), reloaded page (column), closed column, clicked something (no column).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12038
Summary:
Fixes T7060. Removes some hard-coding.
This assumes that "pages with no durable column" and "pages with no Quicksand" are the same, but that's correct today and I can't come up with a use case where they'd be different offhand.
Test Plan:
- Clicked a revision with column open, got Quicksand navigation.
- Clicked into Conpherence with column open, got real navigation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7060
Differential Revision: https://secure.phabricator.com/D12036
Summary: Ref T7380. This does the most basic thing ever and sticks up to 6 icons in there.
Test Plan: clicked the icons and noted new conpherences loaded in nicely
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7380
Differential Revision: https://secure.phabricator.com/D12037
Summary:
Ref T5369. New HTML5 version without flash dependencies.
This doesn't play any sounds.
Test Plan: Did not play any sounds.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T5369
Differential Revision: https://secure.phabricator.com/D9535
Summary: Ref T7014. The main conpherence view is kind of broken without this in subtle ways because of /conpherence/ versus /conpherence/x/ init'ing things differently; this fixes that. Moves more normal view conpherence logic into threadManager. Makes all the display code happen outside of threadManager, setting us up for some display manager later maybe.
Test Plan: sent messages, updated title, etc and the messages pane auto scrolled correctly!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12035
Summary:
Ref T7014. Fixes T7473. This adds a class to handle thread state about what thread is loaded and what transaction we've seen last. It is deployed 100% in the durable column and only partially deployed in the regular view. Future diff(s) should clean up regular view. Note ConpherenceThreadManager API might change a bit at that time.
Also includes a bonus bug fix so logged out users can't toggle this column
Test Plan: tried to use durable column while logged out and nothing happened. sent messages, aphlict-received messages, added people, and changed title from both views
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7473, T7014
Differential Revision: https://secure.phabricator.com/D12029
Summary:
Ref T2009. Ref T1460.
Fixes T2618. When users hit "Delete" on inline comments, delete immediately and offer them "Undo". If they delete indirectly (e.g., by clicking "Delete" from the preview at the bottom of the page), we still prompt them, because the "Undo" action either won't be available or may not be easy to find. This is a "refdelete".
Fixes T6464. This was just a mess. Make it not as much of a mess. It should work now. Pretty sure.
Fixes T4999. We did not refresh these links often enough to find targets for them, so they could race with content. Reevaluate them after loading new changes.
Test Plan:
- Deleted and undid deletion of inlines from main view and preview.
- Clicked "View" on inlines.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6464, T4999, T2618, T1460, T2009
Differential Revision: https://secure.phabricator.com/D12032
Summary:
Ref T2009. Ref T1460. The way Diffusion and Differential load inlines is horrible garbage right now:
- Differential does an ad-hoc query to get the PHIDs, then does a real load to policy check.
- Diffusion completely fakes things. In practice this is not a policy violation, but it's dangerous.
Make TransactionCommentQuery extensible so we can subclass it and get the query building correctly in the right Query layer.
Specifically, the Diffusion and Differential subclasses of this Query will add appropriate `withX()` methods to let us express the query in SQL.
Test Plan: Loaded, previewed, edited, and submitted inlines in Differential and Diffusion
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009, T1460
Differential Revision: https://secure.phabricator.com/D12026
Summary:
Ref T1460. Ref T2618.
When publishing a draft inline, mark the inline it replies to (if any) as replied to.
Also, don't load deleted comments as drafts (sets the stage for T2618).
I'll make an effort to clean up the loading mess here in the next revision, and find some more appropriate home for the shared code.
Test Plan: Made and replied to comments in Differential and Diffusion. Saw comments get marked as "Has Replies" and "Is Reply".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2618, T1460
Differential Revision: https://secure.phabricator.com/D12025
Summary: We respect this when adding inputs to the form, but not when guarding the actual fetch.
Test Plan: Reading
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12030
Summary: Ref T2009. These subclasses have a mixture of similar methods, move them all to the base class.
Test Plan: Created/edited/undo/submitted comments on the left and right sides of a diff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12024
Summary: Ref T7014.
Test Plan: changed the conpherence title from the column. since i can't get scrolling to work, i inspect the dom to verify the title change transaction showed up properly
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12002
Summary:
Ref T2009. This is another almost-identical copy of the row scaffolding, which has the same 1up/2up bugs as the 8 other copies of this code.
Turn the "undo" element into an InlineCommentView so we can scaffold it.
Then, scaffold it with the same code as everything else.
Test Plan: Hit "Undo", swapped from 1up to 2up, hit "undo" again, swapped back, tried left/right, everything rendered with proper scaffolding.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12019
Summary: Ref T2009. This has two more copies of the scaffolding.
Test Plan: Created, edited, deleted, replied to inline comments.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12018
Summary:
Ref T1460. Track and store which comments are threaded replies to other comments, vs merely appearing on the same lines.
This doesn't actually write `hasReplies` yet, since that needs to happen when we un-draft comments on submission.
Test Plan: Made inline comments in Differential and Diffusion, including replies. Replies were marked as "Is Reply".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12017
Summary: Ref T2009. Upgrade this from DetailView to ListView so we get "Highlight As", "View Unified", etc., and respect the unified diff prefernce.
Test Plan: Viewed diffs in Phriction.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12013
Summary:
Fixes T4452. Ref T2009. There's a hierarchy of changeset rendering power: only low-level calls, use of ChangesetDetailView, then use of ChangesetListView (a list of DetailViews).
Prior to work here, the various changeset rendering controllers got their hands dirty to varying degrees, with some using only the lowest-level rendering pipeline:
- Phriction: no view (lowest level)
- Diffusion: DetailView
- Differential Changeset: DetailView
- Differential Diff: ListView
- Differential Revision: ListView
I brought Phriction up to use DetailView, but want to bring everything all the way up to use ListView. Each composition layer adds more features to diff browsing. In particular, this change enables "Highlight As", switching 1up vs 2up, adding inlines, etc., on the standalone view.
Test Plan:
- Viewed a changeset standalone. Could change highlighting, switch 1up vs 2up, add and edit inlines, etc.
- Viewed a revision; no behavioral changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4452, T2009
Differential Revision: https://secure.phabricator.com/D12012
Summary: Fixes T7496, T7511. Sets text for registration is not enabled, sets can_manage on add_provider button.
Test Plan: Test with a logged in admin and logged in normal joe user.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7496, T7511
Differential Revision: https://secure.phabricator.com/D12014
Summary:
Ref T2009. Currently, the code figures out if a comment is on the left or right by looking at the `<th />` preceeding the enclosing `<td />`.
This gets the right result in 2-up, but in 1-up rows are always `<th />`, `<th />`, `<td />`, so it always detects every inline as being in the new file.
Because "old" and "new" cells aren't inherently distingushable in the 1up view, we can't use a DOM test for this at all. Instead, just track this state explicitly.
Test Plan:
- Made left/right comments in 1up view and 2up view.
- Viewed them in 1up and 2up views.
- Hovered in 1up and 2up views.
- Diff-of-diff'd and reviewed old/new comments, then made some more.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12011
Summary:
The "mlb" on the left nav creates a phantom bottom margin which gives the content measurable height but not scrollable height. Replace it with "plb" (padding) instead.
The 2px-spacer calculation was also not quite correct.
Test Plan:
- Viewed pages with navs; padding vs margin didn't seem to make any other differences.
- Scrollbar now stops in the right place in Safari, Chrome, Firefox.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12007
Summary: Renames the method in PHUIObjectBoxView to match the new PHUIInfoView class.
Test Plan: grepped codebase. Went to Calendar and tried a new status.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12005
Summary: Removes AphrontPanelView, and most of it's CSS - it seems some old previews still call it.
Test Plan: grep for AphrontPanelView, no callsites left. Verify CSS left is minimal needed.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7427
Differential Revision: https://secure.phabricator.com/D12004
Summary: Ref T2009. Still a touch glitch-ish but essentially functional now.
Test Plan: Viewed image diffs in 1up and 2up views. Made inline comments on them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12003
Summary: Ref T2009. Remove the 4 (!!) copies of this code.
Test Plan:
- Added, edited, and removed inline comments in 2up view.
- Stacked a bunch of comments on the same line and saw the JS place them correctly.
- Created an image diff and added, edited and removed inlines on it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12000
Summary: Ref T2009. This can now be removed.
Test Plan: Added, edited and deleted an inline comment in 1up view.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11998
Summary:
Ref T2009. Inline comments have "scaffolding", which is basically some empty table cells/rows around them to get the layout correct.
The scaffolding depends on the renderer, since the cells are different for side-by-side vs unified diffs.
This is currently duplicated all over the place:
- Edit view has 1up/2up.
- Detail view has 1up/2up.
- 1up renderer has 1up.
- 2up renderer has four separate copies of the 2up logic.
These all have subtle differences, which are mostly bugs. Start making the scaffolding more composable so we can get rid of that mess.
Test Plan: Added, edited, and removed inline comments on unified and side-by-side diffs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11997
Summary:
Ref T2009. These classes are "Differential" now, but are used elsewhere in diff infrastructure (e.g., Diffusion).
- Rename them to "PHUIDiff".
- Move them to "src/infrastructure/".
- Give them a base class.
Test Plan: Interacted with inlines in unified and side-by-side views.
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11996
Summary:
Ref T2009. This tweaks things a bit more to improve consecuitive groups of added and removed lines.
Generally, it gives us "old, old, old, new, new, new" intead of "old, new, old, new, old, new".
Feelin' real good about having unit tests for this stuff.
Test Plan: Unit tests, looked at diffs in web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11994
Summary: Uses standard sidenav width, more spacing in labels, added background around textarea, make background work in Firefox.
Test Plan:
Test Desktop, Mobile, and Tablet break points. Test Firefox and Chrome.
{F331201}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11993
Summary: This improves some cases with interleaved added and removed lines, and adds test coverage.
Test Plan:
- Added and executed unit tests.
- Viewed raw diff and saw sensible/expected output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11992
Summary:
Ref T7014. This diff addresses
- getting it to be the right set of options
- add participant
- view in conpherence
- close window
- making those options work
- make it so if you are on /conpherence/ you can't toggle the durable column
Test Plan: inspected dom via chrome tools and found last transaction. added a participant and inspected the single new transactin added for accuracy. used view in conpherence action to view in conpherence. used close window action to close window
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11991
Summary:
Ref T2009. This reduces how buggy inlines are. They're still buggy.
Specifically, the inline endpoint didn't know how to scaffold inlines before, so some of them ended up rendering in the wrong rows or breaking layouts.
This passes the current renderer through to the inline editor endpoint, so it can at least get the layout correct.
Test Plan: Interacted with inlines in unified and side-by-side views.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11988
Summary:
These aren't being populated yet; they mostly fix some JS errors with inlines.
For example, the inline hover reticle relies on adjusting its width to account for the "copy" column, and failed when the column did not exist.
Test Plan:
- Hovering inlines in unified now works, mostly.
- Interacted with inlines in side-by-side.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11985
Summary: Ref T2009. Unchanged lines should always go above inlines; we get nonsense results otherwise.
Test Plan: Inline now shows in correct place in unified view.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11987
Summary:
Ref T2009. Currently, lines don't get their "C123NL456" IDs set in the unified view. This is the major way that inlines are glued to changesets.
Simplify this rendering and bring it into the HTML renderer, then use it in the OneUp renderer.
Test Plan:
- Interacted with side-by-side inlines (hovered, added, edited, deleted), saw unchanged behavior.
- Interacted with unified inlines. They still don't work, but the error that breaks them is deeper in the stack.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11983
Summary: Ref T2009. I've clicked these links like 200 times in testing now, so I'm feeling pretty good about them.
Test Plan: Viewed links in side-by-side diff, clicked them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11981
Summary: Ref T2009. It doesn't make sense to have these as separate behaviors. We require a ChangesetViewManager to track view parameter state.
Test Plan: Interacted with changesets in Phriction, Differential and Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11979
Summary:
Ref T2009. Currently, we do not persist view parameters when making context rendering requests.
The big one is the renderer (1up vs 2up). This makes context on unified diffs come in with too many columns.
However, it impacts other parameters too. For example, at HEAD, if you change highlighting to "rainbow" and then load more context, the context uses the original highlighter instead of the rainbow highlighter.
This moves context loads into ChangesetViewManager, which maintains view parameters and can provide them correctly.
- This removes "ref"; it is no longer required, as the ChangesetViewManager tracks it.
- This removes URI management from `behavior-show-more`; it is no longer required, since the ChangesetViewManager knows how to render.
- This removes "whitespace" since this is handled properly by the view manager.
Test Plan:
- Used "Show Top" / "Show All" / "Show Bottom" in 1-up and 2-up views.
- Changed file highlighting to rainbow, loaded stuff, saw rainbow stick.
- Used "Show Entire File" in 1-up and 2-up views.
- Saw loading chrome.
- No loading chrome normally.
- Made inlines, verified `copyRows()` code runs.
- Poked around Diffusion -- it is missing some parameter handling, but works OK.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11977
Summary:
Ref T2009. This clears the stage for D11977.
Specifically, D11977 moves "show context" logic into ChangesetViewManager, but those objects won't exist if we don't run "behavior-populate" first.
Generally, this increases consistency across changeset views -- which is still very low overall, but getting slightly better.
Both of these should probably move up more and use ChangesetListView, but we don't need to do that quite yet.
Test Plan:
- Took changeset actions in Phriction diff view.
- Took changeset actions in Differential standalone view.
- Took changeset actions in normal Differential view.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11978
Summary:
Ref T2009. This basically copy/pastes them for now. Plans is:
- Make this actually work all the way.
- Add test coverage after D11970.
- Move 2-up here after test coverage.
Clicking the links does not work yet, because they use the 2-up renderer. I'll fix this in the next diff.
Test Plan: Viewed diffs in unified, saw links to show more.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11976
Summary: Ref T2009. Remove forced min-width of 780px in 1-up mode, and tweak a few other things to look better.
Test Plan: Looks better on mobile.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11974
Summary: Ref T2009. These aren't good enough to actually use so I won't land this yet, but it makes testing changes a lot easier.
Test Plan:
- Swapped setting.
- Loaded revisions.
- Saw setting respected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11972
Summary:
See D11468 and D11465. Fixes T5163. Fixes T4105. This makes it practical to test shields, unshielding, moves, etc.
This fixes the issue in D11468, where line maps from whitespace-ignored hunks could have fewer lines than line maps from whitespace-respected hunks, causing a warning.
This encodes the behavior which D11465 changed, making it the canon behavior. Specifically, we do **not** show a shield. I think this is correct. It seems misleading to show "the contents of this file were not changed", because they were changed in both the sense that the file was completely removed, and also changed in the sense that the content itself was (or may have been) changed at the destination. Instead, we just show nothing.
Test Plan:
- Added test coverage.
- Ran tests.
- Used `arc diff --raw --browse` to verify that web behavior was consistent with CLI/test behavior.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4105, T5163
Differential Revision: https://secure.phabricator.com/D11970
Summary: This ended up having a different signature; the discrepancy can cause a warning.
Test Plan: No more warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11971
Summary:
Ref T7014. This hooks up the durable column such that when you open it up it loads your most recent Conpherence. You can then switch amongst the various widgets and stuff and everything works nicely.
Except...
- scroll bar does not work
- also doesn't work at HEAD when I add a ton of text to the UI with no changes? (wrapped $copy in array_fill(0, 1000, $copy))
- "widget selector" does not collapse when you select something else
- this part wasn't really specified so I used the aphlict dropdown stuff. didn't want to keep working on that if this was the wrong UI choice
- can not edit title
- do we still want that to be done by clicking on the title, which pops a dialogue?
- can not add participants or calendar events
- what should this UI be? maybe just a button on the top for "participants" and a button on the bottom for calendar? both on top?
- this is not pixel perfect to the mock or two I've seen around. Aside from generally being bad at that, I definitely didn't get the name + timestamps formatting correctly, because the standard DOM of that has timestamp FIRST which appears second due to a "float right". Seemed like a lot of special-casing for what might not even be that important in the UI so I punted. (And again, there's likely many unknown ways in which this isn't pixel perfect)
There's also code quality issues
- `ConpherenceWidgetConfigConstants` is hopefully temporary or at least gets more sleek as we keep progressing here
- copied some CSS from main Conpherence app
- DOM structure is pretty different
- there's some minor CSS tweaks too given the different width (not to mention the DOM structure being different)
- copied some JS from behavior-pontificate.js to sync threads relative to aphlict updates
- JS in general is like a better version of existing JS; these should collapse I'd hope?
- maybe the aphlict-behavior-dropdown change was badsauce?
...but all that said, this definitely feels really nice and I feel like adding stuff is going to be really easy compared to how normal Conpherence is.
Also includes a bonus bug fix - we now correctly update participation. The user would encounter this issue if they were in a conpherence that got some updates and then they went to a different page; they would have unread status for the messages that were ajax'd in. This patch fixes that by making sure we mark participation up to date with the proper transaction in all cases.
Test Plan: hit "\" to invoke the column and saw nice loading UI and my latest conpherence load. sent messages and verified they received A-OK by looking in DOM console. toggled various widges and verified they rendered correctly. opened up a second browser with a second user on the thread, sent a message, and it was received in a nice asynchronous fashion
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11968
Summary: Switch to modern components, crumbs, pht, object boxes.
Test Plan:
Test browsing a list, click edit, click new repository, click delete, verify all work as expected.
{F330864}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7427
Differential Revision: https://secure.phabricator.com/D11984
Summary: Makes this just a plain box (we don't have UI for table-in-form just yet).
Test Plan: Faked some connection data to view the display.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7427
Differential Revision: https://secure.phabricator.com/D11982
Summary: Pass this as true when building XHProf pages
Test Plan: Verify setDeviceReady exists in class PhabricatorBarePageView
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11980
Summary: The participant list can sometimes be `null`, which fails when we try to `array_fuse()` it.
Test Plan: Created a new thread cleanly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11969
Summary:
Ref T6516. Although this behavior is somewhat-arguable as desirable, I think it's less surprising and more consistent to disable mentions when a repository is publishing.
In particular, if you import a repository developed on another Phabricator install, this stops all the `T123` in commit messages from creating mentions on your unrelated `T123` tasks.
We already disable autoclose, so `Closes T123` and `Ref T123` already have no effect, but a bare `T123` would generate a mention. Likewise, `@epriestley` would generate a mention.
If you import such a repository and then update it periodically, updates will activate autoclose and publishing (if you didn't disable them), but presumably this will hit a couple of tasks and you'll go change the settings if you forgot.
At some point, we may have some kind of use case for separating the "publish" setting into a "publish" setting and a "this is a local repository" setting. For example, if you work at Widget Corp, want to import Phabricator locally, //and// want to write Herald rules against it, you can't currently configure the repository to let you do all of this. But we haven't actually seen a use case for this yet.
Test Plan:
- Pushed some commits with bare `T11`, saw mentions.
- Disabled publishing for the repository, pushed some commits with
- Imported a bunch of commits without seeing pipeline failures.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6516
Differential Revision: https://secure.phabricator.com/D11966
Summary: Ref T4214. Reduce the need for installs to purge caches.
Test Plan:
{F329350}
- Changed header twice.
- New one showed up without dirtying cache.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4214
Differential Revision: https://secure.phabricator.com/D11965
Summary: Ref T6516. We incorrectly fail to set this flag on repositories created via Conduit, which activates too many actions on old commits.
Test Plan:
- Created a new repository via Conduit, verified it was "importing" after creation.
- Created a new repostiory via web UI, verified it was "importing" after creation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6516
Differential Revision: https://secure.phabricator.com/D11964
Summary: Fixes T7454. We weren't triggering "Show Older" unless there were hidden events because of a previous interaction.
Test Plan:
- Set page size to 3.
- Viewed a task with ~10 transactions.
- Before patch:
- Only 3 most recent transactions visible, no way to see older ones.
- Saw "show older" appear, paged backward through transaction histroy.
- Also, interacted with task and then viewed it, made sure "show older" still works.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7454
Differential Revision: https://secure.phabricator.com/D11963
Summary: Update fund for new merchant authority stuff.
Test Plan:
- Created a secret initiative.
- Could see related activity as merchant.
- Could not see it not-as-merchant.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11959
Summary: Fixes T7392. I kind of stink at SQL so my approach here was to "start over" conceptually and this way makes the most sense to me - we basically do one join on the dependency table and then a second join back from the dependency table to the main task table. In the where clause we filter the resulting rows, first checking the data from dependency join for existence as appropros and then checking the second join for main task table for the proper "open" task values.
Test Plan: made a task X be blocked by task Y. closed task y. search for "not blocked" tasks and saw task X.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7392
Differential Revision: https://secure.phabricator.com/D11962
Summary: Update Owners per current UI standards, add crumbs at each level, removed AphrontPanels, check spacing.
Test Plan: Tested a list of owner packages, editing a package, creating a package, and various filters.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11961
Summary: Remove AphrontPanels, use standard UI, test for mobile, add phts
Test Plan: Faked a few facts for layout purposes.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11960
Summary: Swaps out to modern UI components, update for mobile, fix some phts.
Test Plan: Test each Daemon page on desktop and mobile. Verify modern layout.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7427
Differential Revision: https://secure.phabricator.com/D11958
Summary: Remove some AphrontPanels, add some phts, fix some table layouts.
Test Plan: Browse many Conduit pages, test a few calls.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7427
Differential Revision: https://secure.phabricator.com/D11957
Summary:
Ref T7439. Fixes T7438. This is only used in email right now.
Remain backward compatible.
Test Plan:
- Sent a message, saw a "Z" reply address.
- Checked the PHIDType.
- Grepped for `"E"` and `'E'`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7439, T7438
Differential Revision: https://secure.phabricator.com/D11956
Summary:
Fixes T7102. These panels don't work, aren't meaningful, and don't seem very useful.
We could eventually support providing context to dashboards somehow ("merchant dashboard") but don't have much of an apparent need for this.
Test Plan:
- Tried to create cart/subscription/charge dashboard panels.
- Unable to create new ones.
- The ones from before the change show a relevant error now.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7102
Differential Revision: https://secure.phabricator.com/D11953
Summary: We've processed some payments without anything going wrong now, and in the Phacility case we control all the payment amounts and the goods are essentially-virtual and billed-after-delivery anyway, so abuse is fairly difficult/pointless and presumably unlikely.
Test Plan: Paid an invoice and saw it go to completed immediately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11951
Summary: See discussion in D11945. This finishes the rest of the merchant views to respect/use merchant authority in order to interact with objects.
Test Plan:
- As a merchant: accepted, refunded, updated, browsed orders.
- As a non-merchant: couldn't do any of that stuff for orders I don't own.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11950
Summary:
Currently, PhortuneAccounts have a very open default policy to allow merchants to see and interact with them.
This has the undesirable side effect of leaking their names in too many places, because all users are allowed to load the handles for the accounts. Although this information is not super sensitive, we shouldn't expose it.
I went through about 5 really messy diffs trying to fix this. It's very complicated because there are a lot of objects and many of them are related to PhortuneAccounts, but PhortuneAccounts are not bound to a specific merchant. This lead to a lot of threading viewers and merchants all over the place through the call stack and some really sketchy diffs with OmnipotentUsers that weren't going anywhere good.
This is the cleanest approach I came up with, by far:
- Introduce the concept of an "Authority", which gives a user more powers as a viewer. For now, since we only have one use case, this is pretty open-ended.
- When a viewer is acting as a merchant, grant them authority through the merchant.
- Have Accounts check if the viewer is acting with merchant authority. This lets us easily implement the rule "merchants can see this stuff" without being too broad.
Then update the Subscription view to respect Merchant Authority.
I partially updated the Cart views to respect it. I'll finish this up in a separate diff, but this seemed like a good checkpoint that introduced the concept without too much extra baggage.
This feels pretty good/clean to me, overall, even ignoring the series of horrible messes I made on my way here.
Test Plan:
- Verified I can see everything I need to as a merchant (modulo un-updated Cart UIs).
- Verified I can see nothing when acting as a normal user.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11945
Summary: This moves global search results to use standard UI, and hopefully allow us to easily add more information.
Test Plan:
Tested a number of open and closed task queries, tried a few users and projects. All seem to work well.
{F328075}
{F328078}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11948
Summary: Removes AphrontPanelViews from UIExamples
Test Plan: Tested new layouts in each of the pages.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7427
Differential Revision: https://secure.phabricator.com/D11947
Summary: Also exclude non-merchant cards.
Test Plan: Loaded subscription, saw better options in dropdown.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11943
Summary: This page doesn't actually exist and we don't really have any content to put on it right now.
Test Plan: No longer saw bogus link in subscriptions.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11942
Summary: Fixes T7434. We need to LEFT JOIN, not JOIN here, because we still want result rows where the value is `null`.
Test Plan: Issued blocked/not-blocked + blocking/not-blocking queries, got results in all cases.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7434
Differential Revision: https://secure.phabricator.com/D11939
Summary: Fixes T6815. This was overlooked in D9838. This could be prettier, but does the job.
Test Plan: {F327790}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6815
Differential Revision: https://secure.phabricator.com/D11937
Summary: Somewhat easier to parse and present information, with ICONS.
Test Plan:
Rebuilt current view with new layout. Tested toggling on and off some of the entries.
{F327816}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11938
Summary: Fixes T7425. Overall, this is surprising and confusing after jump nav was merged with global search.
Test Plan: Searched for "help", got documents matching the word "help".
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: chasemp, epriestley
Maniphest Tasks: T7425
Differential Revision: https://secure.phabricator.com/D11936
Summary:
Fixes T7424. Ref T6308.
Currently, there's no option to just add a card directly from the autopay UI. Add a button so this works.
Also, chip away at T6308 a bit. This isn't perfect but looks a little less out of place.
Test Plan:
{F327637}
- Added a payment method, then set it as autopay.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6308, T7424
Differential Revision: https://secure.phabricator.com/D11935
Summary: Fixes some UIExample UI issues, adds a new full-width setting for DocumentView
Test Plan:
Test UIExamples at desktop and mobile breakpoints
{F327446}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7431
Differential Revision: https://secure.phabricator.com/D11933
Summary:
Fixes T7422. We'll currently choose a "binary" charset with a "utf8_general_ci" collation on "sort" columns on older MySQL, which seems to be causing problems.
Choose "utf8" in this case instead.
(I attempted to simplify the logic, too, but that's the only actual change.)
Test Plan: Went back and forth with `--disable-utf8mb4` on `storage adjust`, but this is version dependent so I'm not 100% sure it's the right fix.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7422
Differential Revision: https://secure.phabricator.com/D11928
Summary:
Fixes T7422. After the recent fix for "sort" columns, we can end up with invalid SQL in some cases when running quickstart.
In particular, we do "COLLATE binary CHARACTER SET utf8_general_ci" (which is invalid).
Preprocess these so we get "COLLATE utf8 CHARACTER SET utf8_general_ci" (which is valid and correct).
Test Plan: Ran `bin/storage upgrade -f --namespace blahblhbaba` with and without `--disable-utf8mb4`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7422
Differential Revision: https://secure.phabricator.com/D11929
Summary: Fixes T7121.
Test Plan: Used `ssh-keygen -t ed25519` on an Ubuntu 14 box to generate a key; verified this is the header on the corresponding public key.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7121
Differential Revision: https://secure.phabricator.com/D11930
Summary: Adding better CSS and set correct tag and examples.
Test Plan: Test UIExamples, creating and click on similar task, empty task in Maniphest.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7423
Differential Revision: https://secure.phabricator.com/D11932
Summary: Fixes T7055. Omitting this from the crumbs is an improvement, but page titles like "New" seem better with a little more context.
Test Plan: Saw "Query:" in page titles only.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7055
Differential Revision: https://secure.phabricator.com/D11931
Summary: Fixes T7406. Unbreak this oldschool code.
Test Plan:
```
GENERATING: PhabricatorPholioMockTestDataGenerator
Generated Mock: M8: Ipsum est eu enim magna do; reprehenderit.
Generated Mock: M9: Dolor est culpa esse cillum ullamco.
Generated Mock: M10: Lorem do cillum sint mollit: minim, in, aliqua esse.
```
Also created a mock normally, worked/looked fine.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7406
Differential Revision: https://secure.phabricator.com/D11924
Summary: Fixes T7308. Multiple users have encountered confusion around how they should specify a set or list in JSON; provide examples.
Test Plan:
```
epriestley@orbital ~/dev/phabricator $ ./bin/config set files.image-mime-types true
Usage Exception: Config key 'files.image-mime-types' is of type 'set'. Specify it in JSON. For example:
./bin/config set '{"value1": true, "value2": true}'
epriestley@orbital ~/dev/phabricator $ ./bin/config set cluster.addresses true
Usage Exception: Config key 'cluster.addresses' is of type 'list<string>'. Specify it in JSON. For example:
./bin/config set '["a", "b", "c"]'
epriestley@orbital ~/dev/phabricator $
```
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7308
Differential Revision: https://secure.phabricator.com/D11925
Summary:
Fixes T7420. On Phacility, our ToS currently links to some policy documents, but users who haven't signed the ToS can't see them.
I've just created a blanket exemption and documented it; I //think// this is reasonable in all cases.
Test Plan: As an un-ToS'd user, viewed some other documents.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7420
Differential Revision: https://secure.phabricator.com/D11923
Summary: Fixes T7232.
Test Plan: Saw "View Raw" gone for removed comment, still present and functional for non-removed comment.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7232
Differential Revision: https://secure.phabricator.com/D11926
Summary: Fixes T7421. Now that we join the task table again to ignore //closed// blockers, all the column names are ambiguous. Make them unambiguous.
Test Plan: Issued some searches with various different parameters.
Reviewers: btrahan, joshuaspence, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7421
Differential Revision: https://secure.phabricator.com/D11922
Summary: This generates not-quite-correctly.
Test Plan: Clicked "Edit Subscription" on a Phortune subscription.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11921
Summary: Removes AphrontContext bar and uses PHUIInfoView instead. This also attaches to the ObjectBox instead for cleaner UI. Also moved phui-error-view.css which was missed.
Test Plan: Test creating a subtask or a new task, see updated info bar and action buttons.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11920
Summary: Since this element isn't strictly about errors, re-label as info view instead.
Test Plan: Grepped for all callsites, tested UIExamples and a few other random pages.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11867
Summary: Fixes T7398. Previously, we would redirect to get a token and then redirect back to make use of it, but lose "download" in the process, and thus not get the correct "Content-Disposition" header.
Test Plan: Clicked "Download" on a lightboxed file.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7398
Differential Revision: https://secure.phabricator.com/D11915
Summary:
Fixes T7379. Currently, builtin files generate with a "users" view policy even if an install is public.
Because these files TTL after 7 days, there's no migration here. Installs won't see the fix actually happen for up to 7 days after updating, though.
Test Plan:
- Deleted a builtin.
- Loaded projects page to regenerate it.
- Saw new builtin had most open policy and was marked as a builtin.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7379
Differential Revision: https://secure.phabricator.com/D11917
Summary: In D11722, a `getGroup()` method was added to all subclasses of `PhabricatorApplicationConfigOptions`, but no abstract method was added to the base class. This will fail if a custom `*ConfigOptions` class does not provide a `getGroup()` method, in which case `$group->getGroup()` (in `PhabricatorConfigListController`) will fatal.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11756
Summary: Just modernizing this a bit.
Test Plan: Ran `./bin/lipsum generate`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11907
Summary:
I was hitting the following error when attempting to use `./bin/lipsum generate`:
```
[2015-02-28 21:55:00] ERROR 2: array_rand(): Second argument has to be between 1 and the number of elements in the array at [/usr/src/phabricator/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php:93]
arcanist(head=master, ref.master=d8182cf55d50), phabricator(head=master, ref.master=658175925931), phutil(head=master, ref.master=260e6105dee5)
#0 array_rand(array, integer) called at [<phabricator>/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php:93]
#1 PhabricatorPholioMockTestDataGenerator::generateImages() called at [<phabricator>/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php:30]
#2 PhabricatorPholioMockTestDataGenerator::generate() called at [<phabricator>/src/applications/lipsum/management/PhabricatorLipsumGenerateWorkflow.php:67]
#3 PhabricatorLipsumGenerateWorkflow::infinitelyGenerate(array) called at [<phabricator>/src/applications/lipsum/management/PhabricatorLipsumGenerateWorkflow.php:36]
#4 PhabricatorLipsumGenerateWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:396]
#5 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:292]
#6 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/lipsum/manage_lipsum.php:21]
```
Test Plan: Ran `./bin/lipsum generate` and received less errors.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11908
Summary: Fixes T7392. When filtering blocked/blocking Maniphest tasks, don't consider closed tasks.
Test Plan:
# Created `T1` and `T2` with `T2 depends on T1`.
# Marked `T1` as resolved.
# Searched for tasks "blocked by other tasks" and noted that `T2` wasn't in the result set.
Reviewers: btrahan, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7392
Differential Revision: https://secure.phabricator.com/D11911
Summary: This diff moves the default monospace font from a Global Default config value to CSS. What this will allow is some flexibility in changing this font in other areas (like Diviner and DocumentView) without changing the defaults globally. However if the admin sets a config value or a user sets a config value, that value will trump all settings in the CSS files with an !important declaration in the page head.
Test Plan:
Currently tested:
- Setting no value
- Setting an admin value
- Setting a user value
Verify remarkup blocks in Differential, Diviner, Conpherence, and Diffusion look as expected.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11597
Summary: Fixes T6944. Create the Aphlict PID directory if it does not exist. See also D11387.
Test Plan: Started Aphlict... saw PID directory created.
Reviewers: anton.vladimirov, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6944
Differential Revision: https://secure.phabricator.com/D11906
Summary: There is a typo in one of the example mail test commands.
Test Plan: Read the command, maybe even paste it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, epriestley
Differential Revision: https://secure.phabricator.com/D10072
Summary: Feed currently returns nothing is there are no stories, we can present a better view here by allowing a base and customizable set of errors. Fixes T7383
Test Plan:
Test a Project feed with no noDataSting and People with a noDataSting
{F321700}
{F321701}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7383
Differential Revision: https://secure.phabricator.com/D11897
Summary: Ref T7384. This just sends SIGHUP to specified overseers in a nice package.
Test Plan: See D11898.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7384
Differential Revision: https://secure.phabricator.com/D11899
Summary: This still needs some fine tuning, but wanted to get opinions. Using it on a laptop feels pretty good. This also moves `durable-column.css` into its own file since it'll likely continue to grow. Minor CSS tweaks to the near perfect rendition of durable column from pixel based mockups.
Test Plan:
Press \ on my laptop. Having issues with Chrome however, but FF and Safari work as expected.
{F322506}
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11901
Summary: Moves the setting from Core to UI, also adds a link to the task for further instructions.
Test Plan: Load up config in sandbox, see new instructions.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4214
Differential Revision: https://secure.phabricator.com/D11900
Summary: See IRC. This check is somewhat misleading right now because it could arise from a mangled/broken Host header rather than a bad `phabricator.base-uri` configuration.
Test Plan: Faked this to trip, read all the text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11894
Summary: Consolidate colors / spacing.
Test Plan:
Test embedding a paste, a list of pastes, and a PasteView for new colors, space.
{F321622}
{F321623}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11896
Summary: Fixes T7382, specifically we were drawing double navs, removed those from each page and added the correct CSS rule.
Test Plan: Test a number of people and profile pages with a footer set.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7382
Differential Revision: https://secure.phabricator.com/D11895
Summary:
Fixes T7287. This trades off 4-byte character support for case insensitivity in these columns, which is a much better trade on the balance.
Also adds more warnings about old MySQL. Note that we already issue a warning when you run "storage adjust" (which I've made stronger) and already "strongly recommend" MySQL 5.5 or newer in the install documentation.
Test Plan:
- Ran `storage adjust --disable-utf8mb4` to go to old definitions, then ran `storage adjust` to get back to the new ones. Everything seemed OK in both cases.
- Verified that utf8mb4 data can be migrated out of these colums with `--unsafe` (which will truncate).
- Verified that manual explains this.
- Faked my way into the setup warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7287
Differential Revision: https://secure.phabricator.com/D11893
Summary: For consistency, we switch back to base font in a few places when using alternate fonts like source-sans or monospace, this makes sure the base font is consistently reset.
Test Plan: Review a Document, a Diff, and a Legalpad form
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11891
Summary: Fixes T7165. Let users specify a file phid in config, and then use that file via an inline style tag. Also, cache the URI so that we don't have to query the file on every page load.
Test Plan: {F319050}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7165
Differential Revision: https://secure.phabricator.com/D11886
Summary: See IRC. This regressed at some point, probably when we fixed these rendering links-inside-links.
Test Plan: Viewed a flagged, closed task.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11885
Summary: See D11882 for context and rationale.
Test Plan:
- Ran `almanac.querydevices`.
- Ran `almanac.queryserices`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11883
Summary:
This already exists on the Query, just expose it via API.
For context, I want to fix `bin/remote restart` so it actually identifies the services on a box rather than faking it.
To do this, I'll look up the device PHID of the box, then look up all the services which are bound to it, then look up all the instances attached to those services. This allows me to do the second step.
Test Plan: Called method from web UI, got expected result.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11882
Summary: This wasn't actually being skipped for invites; really skip it.
Test Plan:
- Registered without invite, captcha.
- Registered with invite, no captcha.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11877
Summary: Fixes T7367
Test Plan: I guess noone every used this? Click on mobile menu, get not a 404.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7367
Differential Revision: https://secure.phabricator.com/D11880
Summary:
Ref T7352. Make sure modern `phd stop` can still read the old PID file format and stop the daemons, at least for now.
Without this, `stop` still detects them and tells you to `stop --force`, which works, but this makes things a good deal cleaner.
Test Plan: Ran `phd stop` from master, then `phd stop` from this revision. Saw old daemons stop cleanly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11873
Summary:
Fixes T7352. This reduces the memory footprint for instances by combining these two similar daemons into one daemon which handles the responsibilities of both.
The fit isn't 100% perfect here but it's pretty close, and the GC daemon is fairly trivial.
Test Plan:
- Adjusted all the numbers to small numbers (5 second sleep, 120 second GC length).
- Added a ton of logging.
- Started trigger daemon.
- Saw it run a GC cycle.
- Saw it reschedule another cycle after 120 seconds (adjusted down from 4 hours).
- Reverted all the logging/small numbers.
- Ran `bin/phd start`, saw stable trigger daemon running.
- Grepped for removed daemon class name.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11872
Summary: Ref T7352. This is pretty straightforward. I renamed `phd.start-taskmasters` to `phd.taskmasters` for clarity.
Test Plan:
- Ran `phd start`, `phd start --autoscale-reserve 0.25`, `phd restart --autoscale-reserve 0.25`, etc.
- Examined PID file to see options were passed.
- I'm defaulting this off (0 reserve) and making it a flag rather than an option because it's a very advanced feature which is probably not useful outside of instancing.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11871
Summary:
Ref T7352. We were previously identifying things by `<daemonClass, overseerPID, startTime>` but that's not unique in a world where one overseer can run multiple daemons.
We already have an internal "daemonID", it just doesn't get written into the DB right now.
Start writing it, then use it to clean up `phd status`.
Test Plan: Ran `phd status`, got more accurate/useful output than previously.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11865
Summary:
Ref T7352. This isn't wildly useful for us but seems generally reasonable, can be helpful with testing, and @hach-que has a use case for it.
The only reason we issue this warning is to prevent user error; you can still launch all the daemons with `phd launch` manually and daemons all use locks to protect critical regions.
Test Plan: Ran `phd start --force` a bunch, saw zillions of daemons.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11861
Summary:
Ref T7352. This moves all the daemons under one overseer. The primary goal is to reduce the minimum footprint of an instance in the Phacility cluster, by reducing the number of processes each instance needs to run on daemon-tier hosts.
This improves scalability by roughly a factor of 2.
Test Plan:
- Ran `phd debug`, `phd launch, `phd start`. Saw normal behavior, with only one total overseer.
- Fataled dameons and saw the overseer restar them normally.
- Used `phd status` and `phd stop` and got reasonable results (`phd status` is still a touch off).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11857
Summary: Ref T7352. This makes `phd stop` and `phd status` produce more reasonable output with the new PID file format.
Test Plan: Ran `phd stop`, `phd status`, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11856
Summary:
Ref T7352. This changes `phd` to pass configuration to overseers over stdin. We still run one overseer per daemon.
The "status" stuff needs some cleanup, but it's mostly just UI/cosmetic.
Test Plan:
- Ran `phd debug`, `phd launch`, `phd start`, `phd status`, `phd stop`, etc.
- Verified PID files write in a reasonable format.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11855
Summary: Ref T7352. A couple of the APIs changed slightly with D11851.
Test Plan: See D11851.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11852
Summary: I missed testing this case when re-doing the navigation.
Test Plan: Test /project/, don't see fatal. Test mobile menu, works as expected.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11868
Summary: This sets $project at the controller level so it's available to building the icon nav and mobile menu, instead of guessing the $id. Fixes T7289
Test Plan: Test a project and its mobile menu when it does and does not have a workboard initialized. Also tested coming in via a slug and an id.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T7289
Differential Revision: https://secure.phabricator.com/D11848
Summary: I'm looking at beefing up PHUIErrorView for additional use cases as I remove some older AphrontViews. This will likely morph into PHUIInfoView and be a more lightweight version of PHUIObjectBox.
Test Plan:
UIExamples, mobile and desktop layouts. Have actual use cases coming in next diffs (may tweak design more then)
{F311943}
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11849
Summary: The mobile menu on people profiles has the incorrect order in the URLs and thus, 404s.
Test Plan: Went to a profile on a mobile display, click on feed and calendar links, got to correct place.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11847
Summary:
We currently cache previews, but the vast majority of previews are never rendered again (e.g., they're a preview of someone partway through typing a comment).
Especially when editing large documents (Legalpad, Phriction), this can bloat the markup cache with data that will never be read and won't get purged for 30 days.
In particular, most of the data on `admin.phacility.com` is currently 1,000 previews of legalpad documents as I made minor edits to them over the course of several hours.
This isn't a big concern, but it's a very easy fix.
Test Plan:
- Previewed a legalpad document, verified that cache rows were not written as I mashed the keyboard.
- Saved the document, verified a new cache row was written.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11832
Summary: This is a useful capability in Phacility for disabled/suspended instances.
Test Plan: Used `bin/phortune invoice` to invoice a disabled instance, saw it decline to invoice.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11837
Summary: This extra space isn't needed
Test Plan: Visit most UIExample pages
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11845
Summary: We should only be adding space then a status is set, not a state.
Test Plan: Test on UIExamples, still renders as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11844
Summary: Swaps out AphrontMiniPanelView usage with PHUIErrorView. Only used on homepage.
Test Plan:
Grepped for usage, only home. Revisit a new home, see modern componant.
{F310934}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11842
Summary: 4th times the charm? There is some confusion with Headers that could be simplified, obviously.
Test Plan: Read PHUIObjectBoxView and select correct method.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11841
Summary: The sky blue colors in the alerts was problematic in other color headers (not black). Rather than hand tweak each color, just going with white seems best. There is also a small animation now, which you may or may not like. It is playful and enjoyable to me at least.
Test Plan: Tested various header colors with and without alert notifications.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11838
Summary: Fixes T7325, T7326, T7328. When you have deleted a document already you have to specify content; this makes this more clear to the user in this specific delete pathway. Also, includes bonus bug fix for T7326 where we weren't moving the title of the wiki page with the rest of the page.
Test Plan: moved a wiki doc and verified it had the title I had specified. tried to delete an already deleted doc via setting the content to blank (i.e. hitting save after making some other edits) and got more clear error UI state
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7328, T7325, T7326
Differential Revision: https://secure.phabricator.com/D11829
Summary:
This just cleans things up a little:
- Don't show signature status if the document isn't signable.
- Show "Not Signable" instead of "No One" to make the meaning more clear in this context, where we don't have a "Who should sign:" sort of cue.
Test Plan: {F310538}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11834
Summary: This reads better?
Test Plan: reading
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11833
Summary: Fixes T7335. "help" gets you to a specific diviner doc which is an external link, so make sure the code sets is external for the redirect response in this case.
Test Plan: typed "help" and got some
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7335
Differential Revision: https://secure.phabricator.com/D11830
Summary: This increases the transparent space around the Phabricator logo. The logo itself is the same size. This allows for adding of other logos more easily without needing to alter the space provided. (Like Phacility)
Test Plan:
Reload page, screenshot logo, pull into Photoshop and verify spacing top and bottom.
{F309985}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11828
Summary:
Fixes T7229. Some usability issues around this controller - basically you can't leave comments with it and its not particular useful compared to the revision page.
Ergo, if there is a revision associated with a given diff, just re-direct back to the revision page with the proper diff loaded.
Test Plan: Tried to view a diff on the standalone controller attached to a revision and instead was re-directed to the revision view page with the proper diff loaded.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7229
Differential Revision: https://secure.phabricator.com/D11811
Summary:
Fixes T7298. There are two ways to import a repository that you want to host, today:
- Create it as "hosted", then push everything to it.
- Create it as "imported", let it import, then switch it to "hosted".
- (Neither of these work with SVN.)
We don't specifically recommend one or the other, although I believe both should work, and most users seem to go with the first one.
In the first workflow, the new empty repository imports completely and gets marked "imported", so our default behavior is then to publish commits. This can generate a lot of email/notification/feed spam.
If you're a fancy expert you might turn off "publish" before pushing, but normal users will frequently miss this.
Instead, when we receive an "import-like" push to an empty repository, put the repository back into "importing" after we accept the changes.
This has to be heuristic since we can't know for sure if a push is an import or new commits, but here's a simple rule that should do pretty well. We can refine it if necessary.
Test Plan:
- Created a new empty repository.
- Added some debugging code; verified the "commit count" and "empty" rules were calculated properly.
- Pushed 8+ commits and saw the repo go into "importing", import, and leave "importing".
- Pushed 8+ commits again and saw them publish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7298
Differential Revision: https://secure.phabricator.com/D11827
Summary:
Ref T7298. We are currently inconsistent about when we publish feed, email, notifications, audits and Herald rules.
Specifically, there are two settings which impact these things:
- The "importing" flag, which is set when we're importing old commits.
- The "herald-disabled" flag, which was expanded in scope some time ago and now actually means "disable publishing".
Various parts of the pipeline were checking only one of these flags. Instead, all of them should check both.
(For example, we should never email users about importing repositories, nor trigger audits on them.)
Test Plan: See next revision.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7298
Differential Revision: https://secure.phabricator.com/D11826
Summary:
Ref T6840. This feels a little dirty; open to alternate suggestions.
We currently have a race condition where multiple daemons may load a commit and then save it at the same time, when processing "reverts X" text. Prior to this feature, two daemons would never load a commit at the same time.
The "reverts X" load/save has no effect (doesn't change any object properties), but it will set the state back to the loaded state on save(). This overwrites any flag updates made to the commit in the meantime, and can produce the race in T6840.
In other cases (triggers, harbormaster, repositories) we deal with this kind of problem with "append-only-updates + single-consumer", or a bunch of locking. There isn't really a good place to add a single consumer for commits, since a lot of daemons need to access them. We could move the flags column to a separate table, but this feels pretty complicated. And locking is messy, also mostly because we have so many consumers.
Just exempting this column (which has unusual behavior) from `save()` feels OK-ish? I don't know if we'll have other use cases for this, and I like it even less if we never do, but this patch is pretty small and feels fairly understandable (that said, I also don't like that it can make some properties just silently not update if you aren't on the lookout).
So, this is //a// fix, and feels simplest/least-bad for the moment to me, I thiiink.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6840
Differential Revision: https://secure.phabricator.com/D11822
Summary: Fixes T6840. Depends on D11822, which is a little iffy.
Test Plan:
Verified all references to `importStatus` are either:
- SQL patches creating the column;
- reads;
- writes immediately before an insert; or
- explicit updates of the column.
That is, I identified no cases of `setImportStatus(X)->save()` on a Commit which may already exist. This //would// break that.
In general, almost all writes go through `$commit->writeImportStatusFlag()`, which is an explicit update.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6840
Differential Revision: https://secure.phabricator.com/D11823
Summary:
Ref T4340. The attack this prevents is:
- An adversary penetrates your network. They acquire one of two capabilities:
- Your server is either configured to accept both HTTP and HTTPS, and they acquire the capability to observe HTTP traffic.
- Or your server is configured to accept only HTTPS, and they acquire the capability to control DNS or routing. In this case, they start a proxy server to expose your secure service over HTTP.
- They send you a link to `http://secure.service.com` (note HTTP, not HTTPS!)
- You click it since everything looks fine and the domain is correct, not noticing that the "s" is missing.
- They read your traffic.
This is similar to attacks where `https://good.service.com` is proxied to `https://good.sorvace.com` (i.e., a similar looking domain), but can be more dangerous -- for example, the browser will send (non-SSL-only) cookies and the attacker can write cookies.
This header instructs browsers that they can never access the site over HTTP and must always use HTTPS, defusing this class of attack.
Test Plan:
- Configured HTTPS locally.
- Accessed site over HTTP (got application redirect) and HTTPS.
- Enabled HSTS.
- Accessed site over HTTPS (to set HSTS).
- Tore down HTTPS part of the server and tried to load the site over HTTP. Browser refused to load "http://" and automatically tried to load "https://". In another browser which had not received the "HSTS" header, loading over HTTP worked fine.
- Brought the HTTPS server back up, things worked fine.
- Turned off the HSTS config setting.
- Loaded a page (to set HSTS with expires 0, diabling it).
- Tore down the HTTPS part of the server again.
- Tried to load HTTP.
- Now it worked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D11820
Summary:
Fixes T7310. We have a whole mechanism for surfacing update errors, but only surface actual update errors, not pull errors.
Instead, surface pull errors too.
Then format them a little more nicely.
Test Plan: {F309769}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7310
Differential Revision: https://secure.phabricator.com/D11821
Summary: Cleans up spacing, hides footer if nothing present, uses common colors.
Test Plan:
Write some typical for a designer code.
{F309840}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11824
Summary: Uses PHUIObjectBoxView to display lists of diffs in Differential and Diffusion, unless embedded on a dashboard.
Test Plan:
Test Dashboard panel, Differential home, Commit, and Diff
{F282173}
{F282174}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11659
Summary: Fixes a few issues. The author of the commit is more prominent / not cut off. Auditors is in a more consistent location. More space is available for reasons. Commits by themselves look much less janky. Only downside is actual Audits are now 3 lines vs. 2, but the extra space is used well.
Test Plan:
Test list of audits and commits.
{F309237}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11817
Summary: Third times the charm?
Test Plan: pray
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11816
Summary: Fixes T7319. These need a "%s" for the count where they had a "%d"
Test Plan: plan in D11812 is no longer a lie! (watcher added / removed strings render correctly)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7319
Differential Revision: https://secure.phabricator.com/D11813
Summary: If we don't have a state in PHUIActionPanelView, don't set the extra padding to display it.
Test Plan: Review in UIExamples.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11814
Summary: Fixes T7319. ...except I can't get this working in my sandbox? Changes to the translation file don't seem to show up. TEST PLAN IS A LIE
Test Plan: became a watcher, un became a watcher - saw sensical translated strings
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7319
Differential Revision: https://secure.phabricator.com/D11812
Summary: Fixes T7299. Also re-direct the user to the initial request uri if the signature was required.
Test Plan: made a signature required legalpad doc. visit the instance at a specific uri, signed the document, and ended up at that specific uri
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7299
Differential Revision: https://secure.phabricator.com/D11809
Summary:
Fixes the long uptake we saw on `meta.phacility.com`. I regressed this in D11795.
We make three calls to this method, but only one actually consumes the messages. The other two are just checking to see if there are any messages.
Only move the cursor up if we're actually going to process the messages.
Test Plan: Sort of tricky to test convincingly since it's inherently race-prone, but ran `debug pulllocal` and pushed update messages and saw it pick them up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11808
Summary:
I am hitting this error when generating Diviner documentation:
```
COMMAND
'/usr/src/phabricator/bin/diviner' atomize --ugly --book $SOME_BOOK --atomizer 'DivinerPHPAtomizer' -- $SOME_PATHS
STDOUT
(empty)
STDERR
[2015-02-18 23:05:01] EXCEPTION: (RuntimeException) Undefined variable: type at [<phutil>/src/error/PhutilErrorHandler.php:210]
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/diviner/atomizer/DivinerPHPAtomizer.php:315]
#1 DivinerPHPAtomizer::parseReturnType(DivinerAtom, XHPASTNode) called at [<phabricator>/src/applications/diviner/atomizer/DivinerPHPAtomizer.php:116]
#2 DivinerPHPAtomizer::executeAtomize(string, string) called at [<phabricator>/src/applications/diviner/atomizer/DivinerAtomizer.php:23]
#3 DivinerAtomizer::atomize(string, string, array) called at [<phabricator>/src/applications/diviner/workflow/DivinerAtomizeWorkflow.php:109]
#4 DivinerAtomizeWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:396]
#5 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:292]
#6 PhutilArgument... (87 more bytes) ... at [<phutil>/src/future/exec/ExecFuture.php:416]
#0 ExecFuture::resolvex(NULL) called at [<phutil>/src/future/exec/ExecFuture.php:438]
#1 ExecFuture::resolveJSON() called at [<phabricator>/src/applications/diviner/workflow/DivinerGenerateWorkflow.php:349]
#2 DivinerGenerateWorkflow::resolveAtomizerFutures(array, array) called at [<phabricator>/src/applications/diviner/workflow/DivinerGenerateWorkflow.php:209]
#3 DivinerGenerateWorkflow::buildAtomCache() called at [<phabricator>/src/applications/diviner/workflow/DivinerGenerateWorkflow.php:170]
#4 DivinerGenerateWorkflow::generateBook(string, PhutilArgumentParser) called at [<phabricator>/src/applications/diviner/workflow/DivinerGenerateWorkflow.php:74]
#5 DivinerGenerateWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:396]
#6 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:292]
#7 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/diviner/diviner.php:21]
```
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11807
Summary: Use modern components, pht
Test Plan: I have no data locally, expect @epriestley to commandeer
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11805
Summary: Ref T7202.
Test Plan: Visited edit subscription page and it worked. Clicked edit link from subscription view page and got to the right place.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7202
Differential Revision: https://secure.phabricator.com/D11803
Summary: This change wraps the icon inline with the text, so smaller width icons have equal spacing between the border and text.
Test Plan:
review a number of different tag with icons, also UIExamples.
{F309048}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11802
Summary: Fixes T7317, allows public to be set on this list controller.
Test Plan: Tested a list of subscribers on a logged in and logged out Diff.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7317
Differential Revision: https://secure.phabricator.com/D11801
Summary:
Ref T6941. In the cluster (and in other reasonable setups) we've separated SSH load balancers from HTTP load balancers.
In particular, ELBs will not let you load balance port 22, so this is likely a reasonable/common issue in larger clusters in AWS.
Allow users to specify an alternate host for SSH traffic.
Test Plan: Set host to someting different, saw it reflected in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6941
Differential Revision: https://secure.phabricator.com/D11800
Summary: Pretty basic, but you can now search panels by type (query, text, tab).
Test Plan: Searched for a few different types of panels, results look correct
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11782
Summary: This port is always HTTP, so use HTTP even if users have set the URI to "https".
Test Plan: Launched server and hit status page, status good.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11799
Summary: Fixes T7106. If you have bad credentials AND you've pushed an "update this repository" message into the queue, the loop above this level ends up resetting the timer every time we go through it, so the daemon spins in a loop failing forever.
Test Plan:
- Created a repo with bad credentials.
- Clicekd "updated now" to queue an update message.
- Saw daemon run in a loop.
- Applied patch, no loop.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7106
Differential Revision: https://secure.phabricator.com/D11795
Summary: Fixes T7284. We were initialized the project name to the empty string, which was making things work like a rename, including automagically adding the old slug.
Test Plan: made a project and no more "empty" tag being made. also don't have that bad transaction story anymore.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7284
Differential Revision: https://secure.phabricator.com/D11794
Summary: Even if you --force, we can't kill PID 0. This sends the process itself the signal, and terminates it.
Test Plan: See D11786.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11787
Summary: Fixes T7263. Last bit there was to upgrade this dialogue to let users know they are letting their primary email address be exposed in these flows. Depends on D11791, D11792, at least in terms of being accurate to the user as the code ended up strangely decoupled.
Test Plan: wordsmithin'
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7263
Differential Revision: https://secure.phabricator.com/D11793
Summary: Ref T7263. We need this in the oauth case and otherwise it makes sense to include.
Test Plan: used the conduit console and saw my email address included in the results!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7263
Differential Revision: https://secure.phabricator.com/D11791
Summary: Ref T7123. Turns out that we might throw ConduitClientException now in proxied scenarios. For all but one callsite remove the try / catch bit and don't issue the call for SVN. For the remaining callsite, also don't issue the call for SVN but keep in the exception logic since its renders a pretty error message in the non-proxied case?
Test Plan: played around with diffusion and things looked okay.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7123
Differential Revision: https://secure.phabricator.com/D11789
Summary: Fixes T7256.
Test Plan: Looked at rXPRF0a7a5f69f5d7 in a local instance. things looked great both pre and post patch.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7256
Differential Revision: https://secure.phabricator.com/D11790
Summary: Fixes T7294. This lets legalpad store other documents that don't need signatures but conceptually belong in legalpad.
Test Plan: made a document with signature type "no one" and it saved. viewed the document and noted no signing UI was present.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7294
Differential Revision: https://secure.phabricator.com/D11788
Summary:
In the cluster, the box has a ton of stuff that "looks like a daemon" beacuse it is some other instance's daemon.
Stop `phd restart` from complaining about this if given a "--gently" flag, which is like the opposite of "--force".
(I'll make it `stop --force` at the beginning of a whole-box restart to kill stragglers.)
Test Plan: Ran `bin/phd restart --gently`, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11784
Summary:
Fixes T7291. There are a class of spam/annoyance attacks here that we should be more strict about preventing, since you can add an individual's address as a mailing list.
This application is likely on the way out so I didn't bother trying to do per-object policies.
Test Plan: Set policy restrictively and could no longer create or edit mailing lists.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7291
Differential Revision: https://secure.phabricator.com/D11783
Summary: Fixes T7295. Humbling debugging experience but I got it.
Test Plan: saved a legalpad doc without edits over and over and saw no "requires signature" transaction. toggled "requires signature", saved, and saw the transaction.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7295
Differential Revision: https://secure.phabricator.com/D11785
Summary: Fixes T7252. The UI is slightly different than in Maniphest - in Maniphest the error shows up at the bottom and here it shows up the top - but I think the UI here makes sense as you see the error right away on the newly returned dialogue?
Test Plan: set "created after" to "assdaasds" and got an error back. set filter to something that should work and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7252
Differential Revision: https://secure.phabricator.com/D11760
Summary:
Fixes T7130. Fixes T7041. Fixes T7012.
Major change here is partitioning clients. In the Phacility cluster, being able to get a huge pile of instances on a single server -- without needing to run a process per instance -- is desirable.
To accomplish this, just bucket clients by the path they connect with. This will let us set client URIs to `/instancename/` and then route connections to a small set of servers. This degrades cleanly in the common case and has no effect on installs which don't do instancing.
Also fix two unrelated issues:
- Fix the timeouts, which were incorrectly initializing in `open()` (which is called during reconnect, causing them to reset every time). Instead, initialize in the constructor. Cap timeout at 5 minutes.
- Probably fix subscriptions, which were using a property with an object definition. Since this is by-ref, all concrete instances of the object share the same property, so all users would be subscribed to everything. Probably.
Test Plan:
- Hit notification status page, saw version bump and instance/path name.
- Saw instance/path name in client and server logs.
- Stopped server, saw reconnects after 2, 4, 16, ... seconds.
- Sent test notification; received test notification.
- Didn't explicitly test the subscription thing but it should be obvious by looking at `/notification/status/` shortly after a push.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7041, T7012, T7130
Differential Revision: https://secure.phabricator.com/D11769
Summary: Fixes T7275. This makes the error stuff a little more consistent with other modern UIs.
Test Plan: {F307286}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7275
Differential Revision: https://secure.phabricator.com/D11778
Summary:
Right now, taskmasters on empty queues sleep for 30 seconds. With a default setup (4 taskmasters), this averages out to 7.5 seconds between the time you do anything that queues something and the time that the taskmasters start work on it.
On instances, which currently launch a smaller number of taskmasters, this wait is even longer.
Instead, sleep for the number of seconds that there are taskmasters, with a random offset. This makes the average wait to start a task from an empty queue 1 second, and the average maximum load of an empty queue also one query per second.
On loaded instances this doesn't matter, but this should dramatically improve behavior for less-loaded instances without any real tradeoffs.
Test Plan: Started several taskmasters, saw them jitter out of sync and then use short sleeps to give an empty queue about a 1s delay.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11772
Summary:
Fixes T7285. If the user tries to view a subscription they don't have permission to view, we may filter all the subscriptions out, then still try to load related data. This can fatal because it's invalid.
Instead, bail if we filtered everything.
Test Plan: Subscritption detail page of another user's subscription is now 404 instead of fatal.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7285
Differential Revision: https://secure.phabricator.com/D11780
Summary:
At least one user wanted to type more text here, and it seems reasonable that administrators may want to write a couple of paragraphs.
I didn't make this short for any particular reason, I just wasn't sure what the workflow would look like as I was building it.
Test Plan: Loaded page, saw normal height text area.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11779
Summary:
This mentions "like GitHub", but we purged all the issues and no longer accept them.
Generally, feature requests should be coming to the upstream only nowadays.
Also, don't overpromise IRC.
Test Plan: Read documentation.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11777
Summary: Fixes T7273. This shows a better title (like "No Such Instance") instead of a generic one ("Unhandled Exception") when the user hits an AphrontUsageException.
Test Plan: Visited a nonexistent instance, got a nice title.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7273
Differential Revision: https://secure.phabricator.com/D11771
Summary:
The correct conf variable is notification.pidfile, not notification.pid.
See also:
* src/applications/config/option/PhabricatorNotificationConfigOptions.php:58
* src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php:34
Test Plan:
Grepped the whole source of Phabricator for occurences of `notification.pid`,
but all matches pointed to `notification.pidfile` instead.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11768
Summary: Ref T7150. Show some basic information instead of nothing.
Test Plan: Used these in Instances.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7150
Differential Revision: https://secure.phabricator.com/D11767
Summary:
If your install isn't public, users can't see the Auth or People applications while logged out, so we can't load their invites.
Allow this query to go through no matter who the viewing user is.
Test Plan: Invite flow on `admin.phacility.com` now works better.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11765
Summary:
Ref T7185. These settings shouldn't be unlocked anywhere. Specifically:
- `reply-handler`: These are on the way out.
- `reply-handler-domain`: Also hopefully on the way out; locked because a compromised administrator account can redirect replies.
- `phabricator.cookie-prefix`: Not dangerous per se, but an admin could have a hard time fixing this if they changed it by accident since their session would become invalid immediately.
Test Plan: Browsed Config.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7185
Differential Revision: https://secure.phabricator.com/D11764
Summary:
Ref T7185. We currently have "locked", "masked", and "hidden" config.
However, "masked" does not really do anything. It was intended to mask values in DarkConsole, but Config got built out instead and "hidden" is strictly better in modern usage and protects against compromised administrator accounts. "hidden" implies "locked", so it's now strictly more powerful than just locked.
Remove "masked" and upgrade all "masked" config to "hidden". In particular, this hides some API keys and secret keys much more aggressively in Config, which is desirable.
Test Plan: Browsed things like S3 API keys in config and could no longer see them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7185
Differential Revision: https://secure.phabricator.com/D11763
Summary: Pretty sure this was me derping, not trying to make a joke.
Test Plan: New text makes sense.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11762
