Summary: Fixes T7724. Things are a bit tricky here as sometimes we don't want to display the help icon at all...! Change things such that we always render at least the icon, though it may have the style set to hidden if appropriate. Instrument quicksand javascript to then load the proper help dropdown if it can. Do this generally so other aphlict dropdowns could work pretty easily.
Test Plan: started on home and noted no help. clicked maniphest and saw maniphest help. clicked home and saw no help again. clicked differential and saw differential help.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7724
Differential Revision: https://secure.phabricator.com/D12499
Summary:
Fixes T7888. This is currently safe, but double quotes are incorrectly escaped.
To keep them unescaped, we have to punch through PhutilSafeHTML a bit. Since the allowable characters are strictly filtered this is still safe in practice, just not as theoretically-safe.
Test Plan: Set font to `32px "impact"` (with quotes), saw impact font.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7888
Differential Revision: https://secure.phabricator.com/D12506
Summary: Ref T7573. Unify code to fetch these counts and do some light formatting since we're going to need to do the same thing for some conpherence-specific ajax in the durable column (See T7708).
Test Plan: loaded up two tabs, one with a durable column on and one without. in the without browser, i read some messages, decrementing my unread count. when i navigated again in the durable column browser, the count updated correctly. with no notifications, commented on a task with another user to get a notification and it showed up properly. visited the task by clicking not the notification and the bubble count decremented correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7573
Differential Revision: https://secure.phabricator.com/D12498
Summary:
Ref T7447. Ref T7870.
When a ghostly inline appears on the page, the timeline isn't currently aware that it's present, so it links elsewhere.
Instead, apply adjustments before rendering the timeline.
Ref T5030. This makes the behaviors in T5030 irrelevant most of the time.
Test Plan:
Before:
{F378106}
After:
- Inlines visible on page are linked directly.
- Inlines which we can't port (e.g., on files not present on current page) are still linked off-page.
- Used "Show Older" to page through and verify consistent rendering.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T5030, T7870, T7447
Differential Revision: https://secure.phabricator.com/D12497
Summary:
Ref T7447. Ref T7870. When a diff affects more than 100 files, we collapse files by default, then expand files with inlines.
Ghostly inlines currently don't cause files to expand, but reasonably should.
Test Plan:
Before:
{F378065}
After:
{F378066}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12495
Summary:
Ref T7447. Ref T7870.
- Forward: When rendering some file "B" which was moved from "A", port ghosts for both "A" and "B" to it.
- Backward: When porting a comment on "X" which was moved from some "Y", allow it to port to a file named "Y" if it can't find a file named "X".
Test Plan:
Before:
{F377809}
After:
{F377806}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12493
Summary:
Ref T7447. Ref T7870. See T7870 for a detailed description of this issue.
NOTE: Replying to these inlines from the UI still does the wrong thing, because we use the database line number, not the UI line number.
Test Plan:
Before:
{F377732}
After:
{F377733}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12491
Summary:
Ref T7447. This might be overkill, but I want to over-explain things until we have more confidence that this is rarely confusing.
NOTE: I'm playing it a bit fast and loose with `setIsGhost()` (passing a dictionary) because making API changes requires changing the interface and Diffusion, which is a pain. I'll clean this up at the end once the interface is more final. This is well-contained for now.
Test Plan:
- Viewed "base vs 2" in a diff with 3 diffs, saw some "older comments" and some "newer comments".
- Hovered the tags for an explanation of comment spookiness.
{F377703}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12490
Summary:
Ref T7447. After compiling inlines which will appear on the changeset, remove inlines which
Later stages remove these anyway, so it doesn't change anything to keep them around, but we can filter them out here cheaply.
This will also let us drive the Differential timeline view with the same logic a few diffs from now, to improve how it renders inlines. Generalize things a little bit.
Test Plan:
- Made a comment on the left of diff 1.
- Made diff 2.
- Viewed diff 2 vs diff 1.
- Verified old-left comment was filtered out by the new loop.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12488
Summary:
Ref T7447. This ports comments forward and backward in the best case:
- The old comment is on a changeset with the same filename.
- The old and new files are pretty much the same, line-for-line.
This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice.
Errata:
- Design is me cobbling something together, probably worth tweaking.
- "Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff.
Test Plan: {F377214}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: johnny-bit, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12484
Summary:
Ref T7447. This class is currently a big mess with a lot of `withWeirdSpecialThingUsedInOnePlace()` type qualifiers.
Try to generalize/normalize it a bit.
Test Plan:
- Viewed inline comments.
- Created a new inline comment.
- Edited an inline comment.
- Marked an inline comment complete.
- Deleted, then undeleted an inline comment.
- Previewed inline comments.
- Viewed drafts as another user, verified they don't show up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12483
Summary: Ref D12448. Ref T7573. This changes quicksand up a bit so rather than caching just rendered HTML we also cache the initial response from the server. We also fire off a quicksand-redraw event which will let things like the page objects for notifications update correctly while using Quicksand (see D12448).
Test Plan: loaded up /p/btrahan/ Clicked the UI elements to navigate to various profile views up to maniphest. clicked back until back at /p/btrahan/ and it worked. clicked forward until all the way back to maniphest and it worked. clicked back 2x, then clicked new links, then back and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7573
Differential Revision: https://secure.phabricator.com/D12449
Summary: Ref T4100. This integration into the "Browse" dialog is probably a little more heavy-handed than we should shoot for.
Test Plan:
{F377131}
{F377132}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12482
Summary: Ref T4100. Support viewer(), members(), and add a new none().
Test Plan:
- Used all new functions.
- Batch edited tasks with unassign action.
- Saved a query from master, upgrade it to this patch, checkbox migrated cleanly into a "no one" token.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12470
Summary: Ref T4100. Let datasources specify a more meaningful title than the class name.
Test Plan: Browsed some sources.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12469
Summary:
Ref T4100. This just makes the "specify stuff in query parameters" workflow a little better:
- You can now do `?projects=differential,diffusion`.
- You can now do `?projects=projects(alincoln)`.
Test Plan: Did that stuff ^^^^
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12468
Summary:
Ref T4100. Ref T5595. This implements these fields in one mega-field:
- Projects
- Not in projects
- In any project
- Include results in no projects
- In users' projects
Hopefully, this is a step in the right direction.
Test Plan: {F375555}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, chad, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12463
Summary:
Ref T4100. Ref T5595. This allows you to execute all "Projects", "Any Project", "Not in projects" and "include no projects" operations in one field.
It doesn't actually implement such a field yet.
Test Plan: {F375516}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5595, T4100
Differential Revision: https://secure.phabricator.com/D12460
Summary: Ref T4100. Implement members() in terms of the real projects datasource.
Test Plan: Used members() functions in Differential, browsed members() functions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12459
Summary:
Ref T4100. This is like members(), but is implemented on top of the raw datasource. This is a lot simpler and involves way less code duplication.
I'll go back and implement members() like this, too.
Nothing actually uses this yet.
Test Plan:
- Used browse view to browse datasource.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12458
Summary:
Ref T4100. I want to reduce the amount of code duplication that function datasources currently need to wrap some parameter datasource.
For example, `ProjectMembersDatasource` should really just be a little bit of logic on top of `ProjectsDatasource`, which should do most of the heavy lifting.
Moving rendering into datasources brings us a step closer to being able to do this.
Test Plan:
- Rendered normal, function, and invalid tokens.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12457
Summary:
Ref T4100. I can simplify the logic a bit here by moving some rendering into the datasources, but a few TokenizerControls currently don't have datasources.
Require datasources and always provide datasources.
Test Plan:
- Used previously-datasourceless controls (e.g., "Add Reviewers").
- Used normal controls.
- Manually verified that no other controls are missing datasources.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12456
Summary:
Ref T4100. Ref T5595. This allows PolicyAwareQuery to write all the logic for AND, OR, NOT, and NULL (i.e., "not in any projects") queries against any edge type.
It accepts an edge type and a list of constraints (which are basically just operator-value pairs, like `<NOT, PHID-X-Y>`, meaning the results must not have an edge connecting them to `PHID-X-Y`).
This doesn't actually do anything yet; see future diffs.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12455
Summary:
Ref T4100. Ref T5595. These functions are trivial for now, but move us toward being able to define more default query behavior by default.
Future changes will give these methods meaningful, nontrivial behaviors.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5595, T4100
Differential Revision: https://secure.phabricator.com/D12454
Summary:
Ref T4100. Ref T5595.
To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.
With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.
For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.
For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.
This causes no behavioral changes.
Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, hach-que, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12453
Summary:
See some earlier discussion in D11593:
> One thing I'm vaguely thinking about is the possibilty that users may be able to invoice one another directly, eventually. For example, we might invoice a contracting client.
> We might need an `isInvoice` flag eventually, but `subscriptionPHID` is a reasonable stand-in for now.
This adds such a flag.
Test Plan:
- Generated an ad-hoc invoice and verified it showed up in the right place.
- Used `bin/phortune invoice` to invoice a subscription and verified it worked correctly.
- Paid an invoice and saw it leave "pending invoices" status.
{F377029}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12480
Summary:
This allows a merchant to send a user an invoice for something arbitrary, like services rendered.
Two major missing parts:
- These don't actually get marked as invoices. I'll fix that in the next diff, but it's not entirely trivial because `subscriptionPHID` is currently overloaded to also mean "is invoice".
- We don't send email automatically. I don't plan to fix that for now, since all our invoicing needs are covered by personal email.
Test Plan:
Merchants have a new "new invoice" option:
{F376999}
This leads to selecting a user and account, and then you can generate the invoice (only one actual "purchase" / line item for the moment). You can add a longer-form remarkup description to contextualize the billable items:
{F377001}
This sends the invoice and takes you to the merchant order overview screen:
{F377002}
For now, you copy/paste that link into a nice personal enterprisey business-to-business email; the recipient sees this:
{F377003}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12478
Summary:
Ref T5523. Adds a new workflow to make some kinds of bulk workboard operations easier.
New dropdown action:
{F376848}
This brings you into the existing bulk edit flow:
{F376849}
When you save an edit, you're taken back to the board:
{F376850}
If you try to edit a column with nothing in it, you get an error:
{F376851}
Note that the selected workboard filter is applied before choosing tasks, so if your filter is set to "open tasks" we only batch edit the open (i.e., currently visible) tasks in the column. I think this is more powerful (it lets you use filtering to select task subsets) but might not be completely obvious in all cases (although I do think it's more obvious than the alternative rule -- just an issue of neither rule being completely obvious).
Test Plan:
- Batch edited tasks in a column.
- Used "Batch Edit Tasks..." to move tasks to a different workboard by removing + adding a project.
- Batch edited a column with filtered-out tasks, verified only visible tasks were edited.
- Batch edited a column with no visible tasks, received error.
- Used the batch editor normally (Maniphest -> Maniphest, no boards).
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: johnny-bit, cburroughs, epriestley
Projects: #prioritized
Maniphest Tasks: T5523
Differential Revision: https://secure.phabricator.com/D12475
Summary:
Ref T4100. See D12465.
- Instead of putting CSS color classes on the tokenizer icons, put them on the container tags.
- Note that this removes the "bluegrey" default classes.
- This doesn't actually add CSS for the classes so, e.g., "green" doesn't make things green yet. This just supports D12465.
Test Plan: Viewed markup, saw classes.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12467
Summary: This accidentally got disabled; we need to pull one more result that we'll use to tell if there's a next page.
Test Plan: Saw "more results" again.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12461
Summary: Ref T4100. This restores the simpler behavior. See discussion in T4100#107445
Test Plan: Used Differential search, saw my token.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12447
Summary:
Ref T4100. Overall:
- Use token background color to communicate token type (blue = object, yellow = function, grey = disabled/closed, red = invalid).
- Use token icon color to make color choices consistent (specifically, use project icon colors in project tokens).
- For functions, use token icon to communicate function result type (e.g., viewer() has a user icon; members(...) has a group icon), since we don't need the icon to indicate "this is a function" anymore.
Test Plan:
{F374615}
{F374616}
{F374617}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12446
Summary:
Ref T4100. This is still a bit rough around the edges, but mostly does what we're after.
- Implements viewer() and members(...) functions.
- The new browse workflow makes these discoverable.
Test Plan: {F374201}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12444
Summary: Ref T5750. This makes browse work for all of the dynamic tokenizers in Herald, Policies, batch editor, etc.
Test Plan: Used tokenizers in Herald, Policies, Batch editor.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12442
Summary: Ref T5750. This adds a basic browse view. Design is a bit rough, see T7841 for some screenshots.
Test Plan: Used browse view to add tokens to tokenizers.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12441
Summary:
Ref T5750. These are a pain to modernize and most don't matter, so cheat:
- Mark a bunch nonbrowsable, including some that probably should be browsable but which I don't want to deal with for now.
- For static datasources, add an easy server-side filter (this isn't really cheating, and is appropriate for the status/priority/application datasources).
- Make composite sources browsable if their components are browsable.
Test Plan:
- Tried to browse an unbrowsable source, got a 404.
- Browsed a composite source.
- Browsed static sources (priority/status/applications).
- Browsed normal sources (people/projects).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12438
Summary: Ref T5750. Make browsing work and improve filtering.
Test Plan:
- Used browse UI.
- Used normal search UI.
{F373752}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12434
Summary: Ref T5750. Update the Almanac service query to be browsable.
Test Plan:
- Browsed and reordered Diffusion.
- Browsed and reordered services in Almanac.
{F373735}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12433
Summary:
Ref T5750. Same deal as D12427, but for projects.
(As before, mostly making the empty/browse view work instead of return nothing.)
Test Plan: {F372353}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12428
Summary: Ref T5750. Fix this query so it works better with the browse view. In particular, make it return results before the user types a query.
Test Plan:
- Used a people tokenizer.
- Used global search tokenizer to search for people.
- Ran new version of query in production to make sure it wasn't crazy.
- Browsed through results in browse view.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12427
Summary: Ref T5750. This adds a search filter which filters results (kind of: a lot of datasources don't do a great job with this right now -- but the correct data is sent to the server).
Test Plan: {F372313}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12426
Summary: Ref T5750. Adds a working "more results". Hard limits at 1000 results to mitigate the amount of trouble offset paging can get us into.
Test Plan:
Artificially set hard limit down; clicked through results.
{F372284}
{F372285}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12425
Summary: Ref T5750. Move the server-side token rendering code to a shared location and use it in the browse view.
Test Plan: {F372252}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12424
Summary:
Ref T5750. Give up and use offset paging.
Each page will become progressively slower, but the amount of time I was sinking into T7803 without making real headway was not justified by fixing this. In comparison, this took 10 minutes. It isn't really useful to browse through tens of thousands of results anyway, and no one is realistically going to try to do that (later in this sequence, I'll explicitly prevent it).
Test Plan:
This is completely primitive right now:
{F372224}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5750
Differential Revision: https://secure.phabricator.com/D12423
Summary: Fixes T7840. Add some unit tests. One explicitly covers the case in T7840. Other cases are "base" cases for rooms and threads, plus the version if T7840 for a thread, where a policy exception should be thrown.
Test Plan: `arc unit` and was able to successfully add a file to a conpherence room i did not have edit permissions on
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7840
Differential Revision: https://secure.phabricator.com/D12443
Summary: If match is found on document title, boost result.
Test Plan:
- Setup phabricator with elasticsearch.
- Index documents with `bin/search index --all`
- Use search and expect query matching titles appear on the top of results list.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: WikiChad, Korvin, epriestley
Maniphest Tasks: T7827
Differential Revision: https://secure.phabricator.com/D12432
Summary: Adds additional CSS to make the Table of Contents into it's own column (if one exists).
Test Plan:
Tested a page with and without a table of contents. Tested tablet, mobile, and desktop breakpoints. Tested Conpherence, scrolling seemed fine still on trackpad and mouse.
{F370973}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12417
Summary: Follow on to D12410. This kills the general CSS file that once powered application transaction in favor of a specific CSS file for the shared transaction styles for both full view and durable column view. I was able to delete some stuff and identify some shared stuff but there is probably more to go here. Also, rename "phabricator-x" to "conpherence-x" for class names.
Test Plan: viewed full conpherence and durable column conpherence and things looked nice
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12412
Summary:
D12409 made me realize this was a bit janky. `PhabricatorTransactionView` was only being used by Conpherence, so move and rename that class to `ConpherenceTransactionView`. Also, rename the existing `ConpherenceTransactionView` to `ConpherenceTransactionRenderer`, moving the actual view bits into the new `ConpherenceTransactionView`. Resulting code is a bit cleaner IMO.
Diff 1 of 2 (second diff has to be written. =D). Diff 2 will take care of the CSS and possibly clean things up further.
Test Plan: played around in conpherence full and conpherence column and things looked nice
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12410
Summary: Ref T7757. This diff made me realize that `PhabricatorTransactionView` is only used in Conpherence, as well as that `ConpherenceTransactionView` is more like a rendering class with a few static methods. Going to take a diff after this to clean all that up.
Test Plan: opened up durable column, clicked on timestamp, voila!
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7757
Differential Revision: https://secure.phabricator.com/D12409
Summary: Simplifies some common CSS rules.
Test Plan: Test some dialogs and menus.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12406
Summary: Fixes T7756. This is the last little stray bit, though finishing T7757 also helps this feature IMO.
Test Plan: said "ZXX is the best" in comment on DXX and saw proper mention transaction on ZXX
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7756
Differential Revision: https://secure.phabricator.com/D12405
Summary: Fixes T7786. Adds very basic mailtag support.
Test Plan: Tested changing language, title to a Paste, didn't get notifications on my test account.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7786
Differential Revision: https://secure.phabricator.com/D12404
Summary:
Ref T7757. Oddities include:
- not working in column view, since the generic anchor technology conflicts once you navigate to a page with a transaction timeline view
- not working if you are linking to a message not included in initial load
Remaining work is addressing these oddities.
- make column view timestamp link to full conpherence correctly?
- make back end load from hyperlinked transaction forward? or do it more like application transactions and have the client keep requesting stuff until it gets it?
Open to suggestions! :D
Test Plan: played around in conpherence full and stuff looked okay. noted no changes as intended in column view.
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7757
Differential Revision: https://secure.phabricator.com/D12402
Summary:
Fixes T7816. This just punts adding proper cursor-based paging to Applications for now, since they don't have a handy order.
If we get to 101, we can either fix this properly or change the SearchEngine to return 200 results.
(Previously, we generated a cursor only if we absolutely needed to, so this code wasn't called. We generate cursors in some cases where we may not need them now, but the code is simpler this way.)
Test Plan: `/applications/` no longer fatals.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7816
Differential Revision: https://secure.phabricator.com/D12403
Summary:
Fixes T7817. Not all documents have `$content`, so don't try to set a timestamp if there's no content.
An example is `/w/asdlkfnalskdnfaslk/`, i.e. the 404 document.
Test Plan: Hit `/w/junk/` and got a nice 404 instead of a fatal.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7817
Differential Revision: https://secure.phabricator.com/D12401
Summary: Ref T7756. Now viewing individual threads in Conpherence is `ZXXX` driven. Also adds remarkup support.
Test Plan: clicked around on list of conpherences in full view and it worked. selected 'view in conpherence' action from column and loaded correct `ZXXX` uri. Typed `ZXXX` in Maniphest and saw it link to Conpherence room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7756
Differential Revision: https://secure.phabricator.com/D12397
Summary: Turns out the pertinent views don't use this data anyway. Additonally, change the remaining pathway to fetch 15% of the rows it fetched in D12347. Fixes T7815.
Test Plan: clicked around and things worked. my instance always feels snappy though
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7815
Differential Revision: https://secure.phabricator.com/D12399
Summary: Ref T7815. Pretty sure this is right? Seems OK locally. Also: simpler!
Test Plan:
- Conpherence is right-side-up.
- Transactions on other objects are also still right-side-up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7815
Differential Revision: https://secure.phabricator.com/D12398
Summary:
Ref T7803. Prior to this change sequence, Query classes conflated paging values (the actual thing that goes in a "x > 3" clause) with cursor values (arbitrary identifiers which track where the user is in a result list).
Although the two can sometimes be the same, the vast majority of implementations are simpler and better when object IDs are used as cursors and paging values are derived from them.
The new stuff handles this in a consistent way, so we're free to separate getPagingValue() from paging. The new method is essentially getResultCursor().
This also implements getPageCursors(), which allows queries to return directional cursors. The inability to do this was a practical limitation blocking the implementation of T7803.
Test Plan:
- Browsed a bunch of results and paged through queries.
- Grepped for removed methods.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12383
Summary:
Ref T7803. Ref T5873. Allows Query methods to expose orderings from the underlying Query class nearly-for-free.
Callers can specify a string to use a builtin ordering, or an array to use a low-level column ordering.
Test Plan: {F368236}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12381
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary:
Ref T7803. Currently, available high-level orders are spread across Query and SearchEngine classes and implemented separately for each application.
Lift the concept of "builtin" (high-level, user-facing, named) orders (similar to "builtin" queries in ApplicationSearch) into the root Query class, and let it drive the SearchEngine implementation. This allows you to define a new order in one place and have it automatically work across the entire stack.
This will also let Conduit expose this information in a straightforward way.
Test Plan:
- Used ApplicationSearch in Diffusion.
- Used all result orderings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12379
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.
Test Plan: Sorted and paged results in various applications.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12378
Summary:
Ref T7803. The ApplicationSearch integration is still a little rough here, but it seems to have the correct behavior.
The rest of this is now at least relatively sane, cohesive, and properly behaved.
Test Plan:
- Used all grouping and ordering queries in Maniphest. Pagingated results.
- Used custom field ordering in Maniphest. Paginated results.
- Paginated through the `null` section of "Assigned" and "Projects" group-by queries. Pagingation now works correctly (it does not work at HEAD).
- Ran unit tests covering priority changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12372
Summary:
Ref T7803. Removes some getReversePaging().
This also fixes `null` column handling, by adding an explicit `'null'` key with possible values "head" (put NULL before other values) or "tail" (put NULL after other values).
Maniphest has some glitchiness in paging through NULLs right now, but I believe it's all pre-existing and will be resolved when it fully converts. Diffusion is fully converted and pages through NULL correctly.
Test Plan:
- Failed to identify any reason for ChangesetQuery to reverse paging.
- Paged thorugh Diffusion.
- Paged through Maniphest.
- Maniphest has some issues when paging inside a NULL section, but these issues are preexisting and will be resolved later in this change sequence.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12371
Summary:
Ref T7803. This is a performance hack, not a real order, and isn't really meaningful or pageable.
After D12158, we constraint his query on `dateModified` anyway, which should generally give the database a relatively small result set to examine.
Test Plan: Browsed Differential and Diffusion. Checked query plan, it didn't look too crazy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12361
Summary: Ref T7803. Move ProjectQuery off getReversePaging() / getPagingColumn() and onto order vectors.
Test Plan: Set project page size to 3 and paged back and forth.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12357
Summary:
Ref T7803. Some Query subclasses implement getPagingColumn() in a trivial way, usually to provide a table alias.
Formalize the concept of a primary table alias, and remove obsoleted getPagingColumn() implementations.
Test Plan: Issued affected queries.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12356
Summary: Ref T7803. Pastes which needed a cache fill would incorrectly be dropped to the bottom of the list. Stop doing that.
Test Plan: Loaded a list of pastes with some that needed cache fills, saw them appear in the correct order.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12354
Summary: Ref T7803. Reduce the amount of code we're trusting to build SQL queries.
Test Plan:
- Paged through results in Maniphest, Differential and Diffusion.
- Some of the NULLable groups in Maniphest are a bit funky but this was preexisting.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12353
Summary:
Ref T7803. Instead of trusting subqueries to provide safe values, escape them explicitly.
(We'll probably have a few cases somewhere where this doesn't work, but can make them the exception rather than the rule.)
Test Plan: Issued all "order" queries in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12351
Summary:
For the price of loading transactions more consistently, we get a better subtitle. We do this in all cases EXCEPT for when we're grabbing handles, because that makes the handles pretty heavy weight and I could even feel the perf hit on my development machine and we don't use subtitle there anyway. We may want to cache the latest message on the conpherence thread object to improve performance here as well as consider falling back to "A, B, C..." more often. Code is written such that no transactions means an automagical fallback.
Fixes T7795. (Technically, there's still a note about handle code conversion work on T7795 but we'll get that generally later.)
Test Plan:
played around with conpherence in both views and things seemed to work nicely.
made sure to try the original repro in T7795 and couldn't get that to go either
posted a long comment and verified that the CSS / string truncation both make it display nicely. Note that without the CSS the chosen glyph value can be too high to fit nicely at times.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7795
Differential Revision: https://secure.phabricator.com/D12347
Summary:
Ref T7811. Fixes two minor issues I observed in the cluster:
- Sometimes APC doesn't give us key names. Not sure exactly what's up here, but we can do a better job with this.
- The `%` in `25%` actually needs more escaping, since it's interpreted by both `pht()` (immediately) and `console_format()` (later).
Test Plan:
- First one is just from an error log, not sure how to repro offhand.
- Ran `bin/phd help start` for the second one.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7814, T7811
Differential Revision: https://secure.phabricator.com/D12395
Summary: Sets a consistent last update time in the header of PHUIDocuments, Legalpad, Diviner, Phriction. I'm not set on the exact language, just that there is consistency, feel free to suggest changes.
Test Plan:
Test Legalpad, Diviner, Phriction.
{F368270}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12384
Summary: Gives back 160px of document space, makes Phriction easier to read. Moves ActionList into menu
Test Plan: Review Phriction Actions Menu, Edit Document, etc. Test mobile, tablet, desktop breakpoints.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12375
Summary:
...because its always at least the string <ATTACHABLE>... Not sure when we'd hit this / see the TODO about making it better, but its definitely a logic bug right now.
(an update to D12347 helped me notice that this conditional is always hit and may fatal later)
Test Plan: NA
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12348
Summary: Saw this variant in a thread.
Test Plan: Unit tests.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12349
Summary:
Fixes T7601. Ref T7803, weakly (this removes a Query subclass with ad-hoc paging). Herald has a very old edit log which predates transactions and is essentially useless and not really policy-aware. I think it's doing more harm than good; remove it.
Herald rules have proper transactions, but rule edits don't currently render something nice into the transaction log. This is definitely the way forward, but we haven't seen requests for this so don't bother building it for now.
I did put a nice end-cap on the transaction log, though.
Test Plan:
- Viewed Herald UI.
- Grepped for removed classes and methods.
- Edited a rule.
- Viewed rule transaction log.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, chad, epriestley
Maniphest Tasks: T7601, T7803
Differential Revision: https://secure.phabricator.com/D12346
Summary: Ref T7795.
Test Plan: updating a conpherence works once more
Reviewers: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7795
Differential Revision: https://secure.phabricator.com/D12345
Summary: Fixes T7689. I'm not going to go clean up all the rest of the `loadViewerHandles()` calls right now since a lot of them are kind of a pain and they aren't really hurting anything so it doesn't feel very leveraged, but at least deprecate it and document the new hotness.
Test Plan:
have a look
in a book
reading rainbow
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12343
Summary:
Ref T7795.
I can't get this to reproduce and its confusing to me how its possible. The trace in T7795 uses the "LOAD" pathway on the update controller. Under the hood, this issues a ThreadQuery with needTransactions to true. With needTransactions to true, the transactions and pertinent handles are all loaded nicely.
So... best guess is there has been some LIMIT of transactions since the offending person participated...? Alternative fix which would probably work is to specify needParticipantCache to true.
More on T7795 - the user report found the "a, b, c..." subtitle thing in the messages dropdown confusing. Yet another fix here would be to change that to be something like "a: snippet of what a said...". I'll discuss that on the task.
Test Plan: iiam
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: nevogd, Korvin, epriestley
Maniphest Tasks: T7795
Differential Revision: https://secure.phabricator.com/D12336
Summary: Ref T7689. Use the more modern handle load mechanisms in Almanac.
Test Plan:
- Viewed Almanac binding detail page.
- Viewed Almanac device interface list.
- Grepped for other callsites to InterfaceTableView.
- Viewed Almanac binding table.
- Grepped for other callsites to BindingTableView.
- Viewed Alamanc service table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12340
Summary:
Fixes T7761. Fixes T7318.
When we send an empty message to the server, pretend its just a request to load the page. Make load a bit smarter such that if we don't get back any transactions, rather than error like the fool, just send down to the client the notion of a 'non_update'. Instrument the client to just turn off the appropriate loading state, etc for a non update.
T7318 is a tricky beast since we don't know exactly how to reproduce it but if / when it occurs again it would be some other bizarre application behavior maybe? We won't be getting the execption anymore, that's for sure.
Test Plan: removed code in `ConpherenceThreadManager.sendMessage` that protects against sending empty messages. sent empty messages (non updates) like whoa and everything worked on both durable column and main column view. re-added the code in `ConpherenceThreadManager.sendMessage` and noted empty messages did not send while any text including a space sent up nicely
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7318, T7761
Differential Revision: https://secure.phabricator.com/D12339
Summary: Ref T7795. This fixes the behavior where you end up with a "a, b, c..." as the list of participants, and yet user a just left.
Test Plan: joined and left a thread. verified database had correct values. observed correct behavior in messages dropdown
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7795
Differential Revision: https://secure.phabricator.com/D12338
Summary: This call got renamed but I missed the callsite.
Test Plan: No more fatal when viewing a custom policy from a transaction history page.
Reviewers: btrahan, chad, Mnkras
Reviewed By: chad, Mnkras
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12337
Summary: Fixes T7791.
Test Plan: grep'd for the typo and only the typo declaration had that functon name.
Reviewers: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7791
Differential Revision: https://secure.phabricator.com/D12334
Summary: Fixes T7735
Test Plan: switched threads in main conpherence view and observed working title glyph.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7735
Differential Revision: https://secure.phabricator.com/D12305
Summary: Fixes T7694. I had to complicate the `ConpherenceThreadQuery` code slightly so that if we specify id(s) or phid(s) then we don't bother with all that join stuff we need to make sure we have a reasonable query in production.
Test Plan: `bin/remove destroy ZXX` worked! tried to visit `ZXX` and got a nice 404. Clicked around and couldn't find anything broken because of the deletion
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7756, T7694
Differential Revision: https://secure.phabricator.com/D12304
Summary: Ref T5501. These settings reduce error log noise.
Test Plan: Faked into this branch and hit the warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12319
Summary:
Ref T5501. Currently, we emit some bad warnings about, e.g., "apc.stat" on PHP 5.5+ systems with OPcache, where the warnings are not relevant.
Generate and raise warnings out of the CacheSpec pipeline so we only run relevant code.
Test Plan: Faked various warnings and saw them render correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12318
Summary: This moves Markdown rendering from normal fonts to PHUIDocumentView with Source Sans improving readability of this longer form text.
Test Plan:
Test libphutil and Phabricator readmes in my sandbox.
{F363483}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12330
Summary: I considered at the time just making all tables taller. This removes the special casing and adds the space universally. On first glance all smaller tables look great, but Diffusion seems a little bloated. After a short time period though that went away for me. I do think Diffusion overall needs a UI refresh.
Test Plan: Tested numerous tables in Phortune, Diffusion, etc. Spacing feels more readable.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12328
Summary: Fixes T7778. This was likely caused by removing an `array_filter()` somewhere in the course of T7731, but I'd rather have the code be more correct.
Test Plan:
Sent mail on a task with no owner.
- Before patch: unknown recipient.
- After patch: expected recipients.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T7778
Differential Revision: https://secure.phabricator.com/D12320
Summary: Ref T5501. This expands cache information a little more.
Test Plan: {F362975}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12316
Summary:
Ref T5501. This code was headed down a bad road; dump an indirection layer between rendering and data gatehring.
In particular, this will make it much easier to lift these issues into setup warnings eventually.
Test Plan: Viewed cache status page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12315
Summary:
Ref T5501. This is just getting version detection and availability right, probably.
Eventually, this will get lifted up a bit and "$remedy" will turn into setup issues (or maybe one setup issue saying "your cache setup is messed up, click here to understand why").
Test Plan:
{F362935}
I intend to shove these up to production one-by-one since production is APC and local is Opcache + APCu.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12314
Summary: This reverts commit 55c00ebfa1. T7111, T7112 and T7113 have all been resolved now.
Test Plan: Maybe wait a few more weeks... I just wanted to ensure that this wasn't forgotten.
Reviewers: btrahan, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11910
Summary:
Fixes T7255.
Note however that some datasources - notably user or project - don't implement the class thing in a clean way since multiple classes apply. For now, we just show these datasources to the user.
Also, I guess this could be done more efficiently by querying for all the applications at once via an application query? LMK if you want me to make that change.
Test Plan: loaded /typeahead/class/ and played with it a bit with no issues
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7255
Differential Revision: https://secure.phabricator.com/D12307
Summary: Ref T6875. #chatlog is deprecated and will eventually be merged into #conpherence.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: cburroughs, epriestley
Maniphest Tasks: T6875
Differential Revision: https://secure.phabricator.com/D12097
Summary: Fixes T7502.
Test Plan: Went to `/diviner/` and saw a link to the documentation at `/help/documentation/PhabricatorDivinerApplication/`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7502
Differential Revision: https://secure.phabricator.com/D12094
Summary: Fixes T7693.
Test Plan: made a conpherence with my main test account and a throwaway test account. deleted the test account. removed myself from the thread. added myself back to the thread.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7693
Differential Revision: https://secure.phabricator.com/D12306
Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.
In particular, this is stronger than these downgrades:
- Downgrades due to "self actions";
- downgrades due to "mail tags".
Test Plan:
- Wrote various Herald rules with "Send me an email" rules.
- Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
- Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12300
Summary:
Ref T7731. Looking forward to T5791, I eventually anticipate writing an interface which looks like a webmail UI where users can review mail they've been sent and understand why they recieved (or did not receive) the mail. Roughly like `bin/mail list-outbound` / `bin/mail show-outbound` work today, but policy-aware (so you can only see messages where delivery was attempted to you).
We currently record a list of "reasons" why a mail is undeliverable, but this list is string-based (so it can not be translated once we start persisting it) and has only negative reasons (so it can not be used to fully understand reasons for delivery or nondelivery).
Make it code-based (so it can be translated) and allow both positive and negative reasons to be listed (so positive reasons can be understood).
Test Plan: Used `bin/mail show-outbound` to review mail delivery reasons, including the positive reason we currently have (forced delivery of authentication mail).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12297
Summary:
Ref T7731. For no particular reason, we currently put `ruleID` and `rulePHID` on `HeraldEffect` objects.
Pretty much all callers need the `HeraldRule` objects instead, and some go to great lengths to get them.
Just attach the `Rule` objects.
Test Plan: Will test thoroughly after next-ish changeset.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12269
Summary:
Ref T7731. Every adapter subclass currently implements this effect in an essentially identical way.
Some day far from now the effects will be modular and this mess will vanish completely, but reduce its sprawl for now.
Test Plan: I'll test this thoroughly at the end of the change sequence since writing rules is a pain.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12268
Summary:
Ref T7731.
- This does nothing.
- I don't know what this was supposed to do.
- It didn't do anything when it was introduced in rP084c79d85a in 2011, either.
iiam
Test Plan:
- `grep`
- ???
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12267
Summary: Fixes T7765. The existing sort was based on when people were added to the conpherence and basically feels random in time. Instead, sort current logged in user to the top and make the rest of the list alphabetical.
Test Plan:
viewed a conpherenece and noted my logged in user at top and the rest was sorted alphabetically
viewed a room that i was not a participant in and participant list rendered correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7765
Differential Revision: https://secure.phabricator.com/D12302
Summary: Fixes T7764. These settings have low utility, are no longer used by default, have become less useful on modern Windows which has a better selection of available fonts, and will eventually be subsumed (at least, for the most part) by T4103.
Test Plan:
- Grepped for strings.
- Viewed settings.
- Changed font to "24px impact".
- Viewed diffs with default and custom font.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T7764
Differential Revision: https://secure.phabricator.com/D12301
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary: Fixes T7582. Basically if its a room we should be able to change title + policy and if its a thread just the title. T7582 had ideas to do a dropdown but "view in column" doesn't make sense from conpherence afaik - what would the page you'd end up with the column be? (maybe home?) Anyway, that is iteration we can add laters
Test Plan: edited room metadata successfully from main and column view. edtied thread title from main and column view.
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7582
Differential Revision: https://secure.phabricator.com/D12252
Summary:
Fixes T7484. There's a bunch of spooky mystery here but the current behavior can probably cause problems in at least some situations.
Also moves a couple callsigns to monograms (see T4245).
Test Plan:
- Faked a short lock length to hit the exception.
- Updated normally.
- Grepped for other use sites, none seemed suspicious or likely to overflow the lock length.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7484
Differential Revision: https://secure.phabricator.com/D12263
Summary:
Fixes T7712. Currently, files sent via email get default policies, like they were dragged and dropped onto the home page.
User expectation is better aligned with giving files more restrictive policies, like they were draggged and dropped directly onto an object.
Make files sent via email have restricted default visibility. Once we identify the sender, set them as the file author. Later, the file will become visible to other users via attachment to a task, revision, etc.
Test Plan: Sent some files via email; verified they got restrictive policies, correct authorship, and appropriate object attachment.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7712
Differential Revision: https://secure.phabricator.com/D12255
Summary:
See IRC. We don't always reach `loadPage()`, particularly if `canViewerUseQueryApplication()` fails.
Perform initialization steps in `willExecute()` instead.
Trace from IRC:
```
2015/04/02 11:48:32 [error] 26979#0: *48012 FastCGI sent in stderr: "PHP message: [2015-04-02 11:48:32] EXCEPTION: (Exception) You must execute() the query before accessing the identifier map. at [<phabricator>/src/applications/repository/query/PhabricatorRepositoryQuery.php:134]
PHP message: #0 PhabricatorRepositoryQuery::getIdentifierMap() called at [<phabricator>/src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php:26]
PHP message: #1 DiffusionRepositoryRemarkupRule::loadObjects(array) called at [<phabricator>/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php:309]
PHP message: #2 PhabricatorObjectRemarkupRule::didMarkupText() called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:292]
PHP message: #3 PhutilRemarkupEngine::postprocessText(array) called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:138]
PHP message: #4 PhabricatorMarkupEngine::process() called at [<phabricator>/src/infrastructure/markup/PhabricatorMarkupEngine.php:71]
PHP message: #5 PhabricatorMarkupEngine::renderOneObject(PhabricatorMarkupOneOff, string, PhabricatorUser) called at [<phabricator>/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldRemarkup.php:47]
PHP message: #6 PhabricatorStandardCustomFieldRemarkup::renderPropertyViewValue(array) called at [<phabricator>/src/infrastructure/customfield/field/PhabricatorCustomField.php:1191]
PHP message: #7 PhabricatorCustomField::renderPropertyViewValue(array) called at [<phabricator>/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php:176]
PHP message: #8 PhabricatorCustomFieldList::appendFieldsToPropertyList(PhabricatorProject, PhabricatorUser, PHUIPropertyListView) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectProfileController.php:220]
PHP message: #9 PhabricatorProjectProfileController::buildPropertyListView(PhabricatorProject, PhabricatorActionListView) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectPr
```
Test Plan:
- Used `rX` in remarkup.
- Used `rX` in search.
- Browed Diffusion, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12264
Summary: Fixes T7730. Herald queries used to incorrectly label object rules as global rules. An object rule is now labeled as such.
Test Plan: Made a few rules and looked at the herald query page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7730
Differential Revision: https://secure.phabricator.com/D12259
Summary: Fixes T7484. If the lock failed, we'd still try to unlock it, which is incorrect.
Test Plan: Ran two `bin/repository update X` in different windows, got proper LockException instead of indirect symptomatic "not locked by this process" exception.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7484
Differential Revision: https://secure.phabricator.com/D12253
Summary: This shrinks the UI to fit more people in the participants list, useful for rooms. Also update the remove icon.
Test Plan:
Review a lot of people in a room, so so many.
{F354233}
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12213
Summary:
Fixes T7584. Adds the ability to specify rooms, messages, or both. Adds policy icon to rooms result view and envelope icon to messages result view. Fixes a missing group by clause in thread query. Enforces having participant phid if the query isn't looking at rooms and doesn't have other particpant phids.
This last bit has a small UI quirk if the user searches for "messages" or "both" with no participant phids as we don't give them the feedback that they were included in the query. We could just slap the viewer in the particpants list in this case but it seemed like a buggier feeling experience to have the viewer appear up there? (Especially so in messages case, where we are basically being smart about policy filtering to come.)
Test Plan: clicked around and got sensible results
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7584
Differential Revision: https://secure.phabricator.com/D12232
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.
- Reduce code duplication between existing receivers.
- Expose receiving objects in help menus where appropriate.
- Connect some "TODO" receivers.
Test Plan:
- Sent mail to every supported object type.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12249
Summary: Ref T7199. Implements `!projects` for all objects which implement `PhabricatorProjectInterface`.
Test Plan: Added projects to a task via email.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12246
Summary: Ref T7199. This makes the page look less janky and provides more context about how mail commands work and how to use them.
Test Plan: {F355959}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12245
Summary:
Ref T7199. Convert the single help menu item into a dropdown and allow applications to list multiple items there.
When an application has mail command objects, link them in the menu.
Test Plan:
{F355925}
{F355926}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12244
Summary: Ref T7199. This needs some polish and isn't reachable from the UI, but technically has all of the information.
Test Plan:
{F355899}
{F355900}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12241
Summary:
Ref T7199. This fully modularizes mail command handling in Maniphest.
I had to add a couple of minor not-totally-solid-feeling tricks to deal with the "create" case, but they feel not-too-bad, and a million times better than what came before.
Test Plan: Used all commands with `receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12240
Summary: Ref T7199. 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:
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:
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:
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 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: 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 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 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: 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
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: 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 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: 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: 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 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: 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 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: 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:
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: 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: 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: 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: 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: 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 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: 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:
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: 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
Summary: Fixes T7159.
Test Plan:
Created a legalpad document that needed a signature and I was required to sign it no matter what page I hit. Signed it and things worked! Added a new legalpad document and I had to sign again!
Ran unit tests and they passed!
Logged out as a user who was roadblocked into signing a bunch of stuff and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7159
Differential Revision: https://secure.phabricator.com/D11759
Summary: Fixes T7088. Mainly this updates the documentation but I also snuck in tweaking how the domain reply handler is built. This does two main things -- makes the behavior consistent as some applications who didn't override this behavior would send out emails with reply tos AND makes it easier for us to deprecate the custom domain thing on a per application basis, which is just silly. On that note, the main documentation doesn't get into how this can be overridden, though I left in that mini blurb on the config setting itself. We could deprecate this harder and LOCK things if you want as well.
Test Plan: read docs, looked good. reasoned through re-factor
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7088
Differential Revision: https://secure.phabricator.com/D11725
Summary:
Ref T7143. This is the simplest fix for adding a new route for Home, at the cost of possibly letting users break instances. However:
- It's kind of hard to get to the option to uninstall Home anyway.
- It's hard to imagine anyone will really uninstall Home by accident, right? Right?
- Put a really scary warning on the action just in case.
Dashboards was only required because Home was required, I think, so just drop that too.
Test Plan: Uninstalled home.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T7143
Differential Revision: https://secure.phabricator.com/D11753
Summary: Ref T7152. Gives us an event hook so we can go make users a member of any instance they've been invited to as soon as they verify an email address.
Test Plan:
- Used `bin/auth verify` to trigger the event.
- Build out the invite flow in rSERVICES.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152
Differential Revision: https://secure.phabricator.com/D11752
Summary: Ref T7234. I didn't know about this spot in D11750.
Test Plan: ..the next diff really makes this work for the T7234 scenario.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7234
Differential Revision: https://secure.phabricator.com/D11751
Summary:
Ref T7234. Turns out some search engines are context specific such that they can't be bubbled up to a dashboard panel generically. The example in question is an Instance Members search, where the instance must be specified and is done so in normal codepaths but the dashboard panel stuff has no way of doing that. Ergo, just turn off these sorts of panels.
Note this code just makes it so we can turn off these sorts of panels but does not do any of that.
Test Plan:
made sure all the queries still showed up
otherwise, next diff
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7234
Differential Revision: https://secure.phabricator.com/D11750
Summary:
Ref T7152. Ref T1139. This updates Phabricator so third-party libraries can translate their own stuff. Also:
- Hide "All Caps" when not in development mode, since some users have found this a little confusing.
- With other changes, adds a "Raw Strings" mode (development mode only).
- Add an example silly translation to make sure the serious business flag works.
- Add a basic British English translation.
- Simplify handling of translation overrides.
Test Plan:
- Flipped serious business / development on and off and saw silly/development translations drop off.
- Switched to "All Caps" and saw all caps.
- Switched to Very English, Wow!
- Switched to British english and saw "colour".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152, T1139
Differential Revision: https://secure.phabricator.com/D11747
Summary:
Ref T7184. I managed to write a phantom setup issue which fails normally and succeeds when looked at carefully, so clicking "you have open issues..." always cleared them. This made it very difficult to figure out what the problem was.
Show issue keys in the "title" attribute to make this sort of thing easier to deal with.
Test Plan: Moused over "You have issues..." text, saw issue key, quickly fixed issue with new information.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7184
Differential Revision: https://secure.phabricator.com/D11743
Summary:
Ref T7152. This substantially completes the upstream login flow. Basically, we just cookie you and push you through normal registration, with slight changes:
- All providers allow registration if you have an invite.
- Most providers get minor text changes to say "Register" instead of "Login" or "Login or Register".
- The Username/Password provider changes to just a "choose a username" form.
- We show the user that they're accepting an invite, and who invited them.
Then on actual registration:
- Accepting an invite auto-verifies the address.
- Accepting an invite auto-approves the account.
- Your email is set to the invite email and locked.
- Invites get to reassign nonprimary, unverified addresses from other accounts.
But 98% of the code is the same.
Test Plan:
- Accepted an invite.
- Verified a new address on an existing account via invite.
- Followed a bad invite link.
- Tried to accept a verified invite.
- Reassigned an email by accepting an unverified, nonprimary invite on a new account.
- Verified that reassigns appear in the activity log.
{F291493}
{F291494}
{F291495}
{F291496}
{F291497}
{F291498}
{F291499}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152
Differential Revision: https://secure.phabricator.com/D11737
Summary:
Ref T7152. Ref T3554.
- When an administrator clicks "send invites", queue tasks to send the invites.
- Then, actually send the invites.
- Make the links in the invites work properly.
- Also provide `bin/worker execute` to make debugging one-off workers like this easier.
- Clean up some UI, too.
Test Plan:
We now get as far as the exception which is a placeholder for a registration workflow.
{F291213}
{F291214}
{F291215}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3554, T7152
Differential Revision: https://secure.phabricator.com/D11736
Summary:
Ref T7152. This implements the administrative UI for the upstream email invite workflow.
Pieces of this will be reused in Instances to implement the instance invite workflow, although some of it is probably going to be a bit copy/pastey.
This doesn't actually create or send invites yet, and they still can't be carried through registration.
Test Plan:
{F290970}
{F290971}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152
Differential Revision: https://secure.phabricator.com/D11733
Summary: Ref T2783. This cleans up some more of the direct VCS access calls. If the repository is local, this boils down to an in-process call. If not, it uses Conduit to make an intracluster request.
Test Plan: Used `reparse.php --message <commit> --trace` to observe cluster request.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11253
Summary:
Fixes T6484. I primarily need this to synchronize device public keys in the Phabricator cluster so the new stuff in T2783 works.
Although, actually, maybe I don't really need it. But I wrote it anyway and it's desirable to have sooner or later.
Test Plan: Ran method.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6484
Differential Revision: https://secure.phabricator.com/D11163
Summary: Groups setup issues into Important, PHP, MySQL, and Base for easier parsing on initial installations.
Test Plan:
Test my internal server and various issues.
{F289699}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7207
Differential Revision: https://secure.phabricator.com/D11726
Summary: Rename `DifferentialChangesetParser::WHITESPACE_IGNORE_FORCE` to `DifferentialChangesetParser::WHITESPACE_IGNORE_ALL` to better reflect reality.
Test Plan: Viewed a diff with various settings for the "Whitespace changes" option.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11730
Summary: This is correct, but the root cause of the issue isn't very clear to me.
Test Plan: Poked around various pages which filter objects.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11727
Summary: Rename `DifferentialChangesetParser::WHITESPACE_IGNORE_ALL` to `DifferentialChangesetParser::WHITESPACE_IGNORE_MOST`.
Test Plan: Browsed a diff with a few different settings for "Whitespace changes".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11715
Summary:
Ref T7152. This builds the core of email invites and implements all the hard logic for them, covering it with a pile of tests.
There's no UI to create these yet, so users can't actually get invites (and administrators can't send them).
This stuff is a complicated mess because there are so many interactions between accounts, email addresses, email verification, email primary-ness, and user verification. However, I think I got it right and got test coverage everwhere.
The degree to which this is exception-driven is a little icky, but I think it's a reasonable way to get the testability we want while still making it hard for callers to get the flow wrong. In particular, I expect there to be at least two callers (one invite flow in the upstream, and one derived invite flow in Instances) so I believe there is merit in burying as much of this logic inside the Engine as is reasonably possible.
Test Plan: Unit tests only.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152
Differential Revision: https://secure.phabricator.com/D11723
Summary:
Fixes T7092. When you name project "Foo" which has primary hashtag "foo" to "Foobar", post this patch the hashtag "foo" gets added as a secondary hashtag. Also makes sure we don't normalize the hashtags in the query function as the wikimedia folks were hitting an issue around capitalization on the hashtag.
Note that T6909 remains "broken" in that you get an error that you can't do that, though if you just omit the additional hashtag it would work fine. I think if a fix is necessary here the best bet would be to simply detect this particular scenario and let things proceed; its a bit tricky though since its about two transactions about to be applied and how they interact with one another...
Test Plan: Made project "Foo" which has primary hashtag "foo". Renamed it to "Foobar" and verified "foo" was added as a secondary hashtag and "foobar" was the primary hashtag. Renamed it again to "Foo" and noted that the hashtags all ended up correct.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7092, T6909
Differential Revision: https://secure.phabricator.com/D11697
Summary:
This makes thumbnail URIs work on instanced, CDN'd installs like Phacility cluster instances.
Some of these transforms can proabably be removed, but the underlying code to generate the transform should be cleaned up too and we have some other tasks filed elsewhere about this anyway.
Test Plan: CDN'd local install now loads thumbnails properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11719
Summary: Fixes T7153.
Test Plan:
used `bin/auth trust-oauth-client` and `bin/auth untrust-oauth-client` to set the bit and verify error states.
registered via oauth with `bin/auth trust-oauth-client` set and I did not have the confirmation screen
registered via oauth with `bin/auth untrust-oauth-client` set and I did have the confirmation screen
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7153
Differential Revision: https://secure.phabricator.com/D11724
Summary: I //think// Maniphest has switched to real edges now.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11716
Summary: I think this is safe to remove now.
Test Plan: WIP
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11717
Summary: Adds core and apps grouping to configuration options, makes it somewhat easier to browse config options.
Test Plan: Set each option, review list. Breakdown is nearly 50/50 apps/core.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11722
Summary: Ref T7210. Not sure if this fixes things, but it's definitely //an// issue.
Test Plan:
- Not able to reproduce issue locally yet.
- These get into the map now, at least?
- Saw `.woff2` URIs transform in CSS.
- Loaded a `.woff2` file.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7210
Differential Revision: https://secure.phabricator.com/D11720
Summary:
The generated HTML is like `<p>some text <div …>…</div> more text</p>`, and HTML `<p/>` tags may not contain block content like `<div/>` tags. Browsers actually parse this as if it was `<p>some text </p><div …>…</div> more text<p></p>` (sic).
The layout CSS class already has `display: inline` set, but this is not sufficient. Browser's HTML parser doesn't care what CSS rules will be applied, it only deals with the meanings of tags.
Fixes T7201.
Test Plan:
Verify that the following displays the image inline:
`some text {Fnnn,layout=inline} more text`
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Projects: #remarkup
Maniphest Tasks: T7201
Differential Revision: https://secure.phabricator.com/D11706
Summary: Adds option for setting large text instead of icons. Adds success state.
Test Plan:
Built some more examples.
{F286388}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11710
Summary: Super duper sized panels for singluar actions.
Test Plan:
UIExamples, will need more testing in Phacility.
{F286098}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11709
Summary:
Ref T7208. Now that we have approvals (new installs are safe by default), take those into account when generating this warning.
Try to soften the warning to cover the case discussed in T7208, hopefully without requiring additional measures.
Test Plan:
{F286014}
{F286015}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7208
Differential Revision: https://secure.phabricator.com/D11708
Summary: Uses more standard boxes for display, and icons!
Test Plan:
Test with all enabled, all disabled, and a mix.
{F285945}
{F285946}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11707
Summary: Ref T7153. The "whoami" scope should be default and always on, because otherwise we can't do anything at all. Also, if a client doesn't want a certain scope, don't bother asking the user for it. To get there, had to add "scope" to the definition of a client.
Test Plan: applied the patch to a phabricator "client" and a phabricator "server" as far as oauth shenanigans go. Then I tried to login / register with oauth. If the "client" was configured to ask for "always on" access I got that in the dialogue, and otherwise no additional scope questions were present. Verified scope was properly granted in either case.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7153
Differential Revision: https://secure.phabricator.com/D11705
Summary: Ref T7153. I am not sure if this is 100% correct because sometimes you have to POST vs GET and I don't know if the redirect response will / can do the right thing? I think options to fix this would be to 1) restrict this functionality to JUST the Phabricator OAuth provider type or 2) something really fancy with an HTTP(S) future. The other rub right now is when you logout you get half auto-logged in again... Thoughts on that?
Test Plan: setup my local instance to JUST have phabricator oauth available to login. was presented with the dialog automagically...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7153
Differential Revision: https://secure.phabricator.com/D11701
Summary: Fixes T7142. Make old permission mean "make (non-bot) users" and then nuance the UI for those administrators who can make bot accounts.
Test Plan: loaded up admin a with full powers and admin b with restricted powers. noted admin a could make a full user. noted admin b could not make a full user. noted admin b got an error even via clever uri hacking.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7142
Differential Revision: https://secure.phabricator.com/D11702
Summary: `ssh-keygen` declines to run on a too-public key. Write the correctly-restricted key a little earlier in the workflow.
Test Plan:
```
epriestley@orbital ~/dev/phabricator $ chmod 644 ~/dev/core/conf/keys/daemon.key
epriestley@orbital ~/dev/phabricator $ ./bin/almanac register --private-key ~/dev/core/conf/keys/daemon.key --identify-as local.phacility.net --device daemon.phacility.net --force --allow-key-reuse
Installing public key...
Installing private key...
Installing device ID...
HOST REGISTERED This host has been registered as "local.phacility.net" and a trusted keypair has been installed.
epriestley@orbital ~/dev/phabricator $
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11700
Summary:
I'm hitting this in the cluster and couldn't figure it out after staring at it for a couple minutes. Produce a better error.
This dumps a hash of each configuration key value which is set to a non-default value into the daemon log. This is much more compact than the full config, and doesn't spread secrets around, so it seems like a good balance between providing information and going crazy with it.
Test Plan: {F284139}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11699
Summary: This got clobbered in D11547. Revive the code but move it up from the base class to the PeopleList controller which is presumably all the main "admin" views. Fixes T7181.
Test Plan: Saw the button once more on /people/...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7181
Differential Revision: https://secure.phabricator.com/D11698
Summary:
Fixes T7169. We just weren't doing a policy-aware query. Basic idea here is that if you set an app to be visible only to specific users, those specific users are the only ones who should be able to authorize it.
In the Phacility cluster, this allows us to prevent users who haven't been invited from logging in to an instance.
Test Plan:
- Tried to log into an instance I was not a member of.
- Logged into an instance I am a member of.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7169
Differential Revision: https://secure.phabricator.com/D11696
Summary: Fixes T7118. This does the basic "filter the list" thing, though it ends up being a little manual since I guess this hasn't come up before? There is also potential weird behavior if the user was using an app and lost access to it - they will have nothing selected on edit - but I think this is actually correct behavior in this circumstance.
Test Plan:
used a user who couldn't get access to the "quick create" apps and noted that the dropdown list on dashboard panel create was missing the expected engines
ran `arc unit --everything` to verify abstract method implemented everywhere
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7118
Differential Revision: https://secure.phabricator.com/D11687
Summary: Fixes T7164. Adds some details about how the statuses will show up in the UI.
Test Plan: Read the text
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T7164
Differential Revision: https://secure.phabricator.com/D11686
Summary: Fixes T7117. The slightly icky part is we just build the menu items up 2x because there's no way to tell you wont be able to make a menu item unless you try to make them all and come up with nada.
Test Plan: created a user and denied them access to every application in the quick create menu. observed the "+" icon disappearing from the nav, correctly. used a different, unrestricted user and the menu showed up and worked
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T7117
Differential Revision: https://secure.phabricator.com/D11684
Summary: So I derped and missed the %s inside the `UPDATE` query (previously only fixing the `INSERT` query). This changes `%s` to `%B` for the update logic as well.
Test Plan: Patched it in production and saw the offending build run all the way through without UTF8-related exceptions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11669
Summary:
This allows us to CDN the cluster.
General problem is that we can't easily give each instance its own CDN URI (`giraffe.phcdn.net`) in Cloudfront, because it requires that you enumerate all aliases (and there's a limit of 100) and depends on SNI (a newish feature of SSL which allows one server to serve multiple certificates, but which doesn't have full support everywhere yet).
It's //possible// that we could eventually work around this, or use Cloudflare instead (which has a different model that seems like a slightly easier fit for CDN-domain-per-instance), but I don't want to sink a ton of work into this and want to keep things on AWS insofar as we reasonably can.
The easiest way to fix this is just to put the instance identity into URIs, then read it out when handling CDN requests. This has no effect on installs without cluster instance configuration, which is all of them except ours.
It's also slightly desirable to share this stuff, since we get to share the cache for static resources, which are always identical across instances.
So requests go from the Cloudfront gateway ("xyz.cloudfront.com") to the LB with a hard-coded instance name ("cdn.phacility.com"), which gets them routed to a balanced web machine. The web machine picks the correct instance name out of the URI, acts as that instance, and does the correct thing.
The messiest part of this is that we need "cdn.phacility.com" to be a real instance so it can serve static resources, but that's not a big deal. We have a few other hard-codes which have to be real resources for now, like we must have a merchant named "Phacility".
Test Plan:
- Viewed files with `security.alternate-file-domain` off (i.e., no file tokens).
- Viewed pages and files with `security.alternate-file-domain` on. Saw correct resource behavior, @isntance generation of URIs, and correct token redirect behavior for files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11668
Summary: Fixes T7094 (last of many revisions). Its important to do this filtering ASAP so that users can't deduce the identify of an unknown / invisible project.
Test Plan: executed a query for tasks in project foo using user bar. using user foo, lock user bar out of project foo. reissued the query and saw "no data" as well as "restricted project" in the project typeahead.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11660
Summary: Ref T7094. This is a bit involved and should be tackled as a separate effort. The good news is policy still saves the day here but (back to the bad news) its a bad user experience.
Test Plan: NA, just a comment
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11658
Summary: Ref T7094#94295.
Test Plan: noted the absence of the TODO comment in the diff
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11656
Summary: Ref T7094. Switch to OmnipotentUser policy-based query since this is usually done offline, etc.
Test Plan: pretty simple code change so I just have my fingers crossed while I am typing this
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11655
Summary: Ref T7094. I guess theoretically someone could be making a commit and have just lost access to the revision and thus this could link this commit to that revision, but this all seems far fetched an weird? We also don't necessarily have the commit author's true identity since commit parsing can be a little funky to begin with. Anyhoo, functionally, this makes things no worse, but I am removing the TODO that would make us look at this in a fun way.
Test Plan: `bin/repository reparse --owners rXvalidhash` and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11654
Summary: Ref T7094. This makes the underlying class take a $user parameter, and then the worker just hands it an omnipotent user. Said underyling class is the benefactor of a small re-factor, dropping one query per-use, though the single query that now remains is policy-based so maybe its a wash or even worse. Still, gotta love one less query.
Test Plan:
a little tricky to test so some extra thought instead
basic acceptance test with `bin/repository reparse --change rValidHashHere` -- it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11653
Summary: Moving towards a consisent 'if header, show in object box' style around Phabricator.
Test Plan:
Grep for uses of RevisionList and make sure double boxes arent set, browse Differential, various searches, a revision, and a commit.
{F282113}
{F282114}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11651
Summary:
Fixes T2380. Fixes T2382. Users should really configure this, but when we had a warning before a lot of users had trouble with it.
- Tout performance benefits.
- Document easy setup via CDN.
- We have an "Ignore" button now for users who really don't care.
Test Plan:
- Set up `admin.phacility.com` through AWS CloudFront (need a few changes to handle instances to put it on the cluster in general).
- Set up `secure.phabricator.com` through CloudFlare (almost; waiting for DNS).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T2382, T2380
Differential Revision: https://secure.phabricator.com/D11649
Summary: Ref T7094. We basically need to make sure folks can see repositories before making owners packages about code within. This cleans up things a little bit by moving a bunch of logic out of the storage class and into an editor class.
Test Plan: made a package and it worked! deleted a package and it worked! discovered buggy behavior in more complicated edits and filed T7127; note this bug exists before and after this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11652
Summary:
Ref T7123. Two general issues:
For proxied repositories, we currently throw a ConduitClientException, vs ConduitException for local repositories. This is inconsistent and we should fix it, but I also want to examine the use of try-the-call-and-throw at these sites since it may be something we can update. In particular, trying a call that we know will always fail is now more expensive (in proxied repositories) than it used to be.
Here, we try-and-throw for merges, but they're //never// supported in Subversion. Just don't bother trying.
Test Plan: Browsed a SVN repository with proxying set up, got a clean commit page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7123
Differential Revision: https://secure.phabricator.com/D11646
Summary:
Fixes T7122. The way this query works is a little surprising:
- If executed as `withRepositoryIDs(...)`, it assumes you are passing one //or more// repository IDs, so it will never resolve ambiguous identifiers (e.g., "123" instead of "rSVN123").
- If executed as `withRepository(...)`, it knows you are passing exactly one repository and will use that to imply context and resolve these identifiers correctly.
This isn't very obvious from the API, but I'm not sure how to make it more clear.
(Making `withRepositoryIDs()` do the `withRepository(...)` thing if only one ID was passed in would mean its behavior varied if you passed 1 vs 2 repository IDs, which seems worse / morse surprising.)
Test Plan: Various subversion UIs no longer fail to look up commits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mormegil, epriestley
Maniphest Tasks: T7122
Differential Revision: https://secure.phabricator.com/D11645
Summary: Minor, adds border, reduces greys, etc.
Test Plan:
View a number of config issues, see new colors.
{F282035}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11650
Summary:
This is a pain to test, but we do a lot of needless "X committed thing (authored by X)" right now.
I think that's because we compare two handle links here, and they're never the same, even if they're both links to the same object.
Instead, compare the author and committer more carefully.
Test Plan: Will do it live.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11635
Summary: Ref T7094. I am not sure when this text is legitimately exposed to users - they should be getting an error about not being able to see the object before they get an error about not being able to see a given transaction... That said, I think this text is logically correct at least.
Test Plan: read the text
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11632
Summary: should just be "withIDs" Ref T7094
Test Plan: submitting this very diff!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11633
Summary: This got updated recently but isn't quite correct.
Test Plan: Called `arcanist.projectinfo` using the name of a proejct with a repository association.
Reviewers: btrahan
NOTE: Cowboy committing this since it breaks `arc diff`.
Summary: We probably can't land this yet, since `arc tasks` still uses `maniphest.find` and `arc close` still uses `differential.getrevision`. We should clean those up and wait at least 30 days before committing this (maybe).
Test Plan: Saw setup issues for `maniphest.find` and `differential.getrevision` calls.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, joshuaspence, FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D6333
Summary: Ref T7094. We already had and were mostly using "needProfileImage" on the people query class. Only real trick in this diff is deleting a conduit end point that has been marked deprecated for the better part of 3 years.
Test Plan: clicked around the people action and profiles and calendars loaded nicely.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11630
Summary: Not too shabby - just convert some raw queries to the policy queries. Ref T7094.
Test Plan: NA 'cuz releeph
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11591
Summary: Ref T7094. This one is really straight-forward since $this->actor is always populated and the right thing to do here.
Test Plan: used the ole thinking noodle since testing email w/ attachments is really hard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11589
Summary: Ref T7094. This loadRepository() method bypassed policy unnecessarily. kill it.
Test Plan: basically un-tested since arcanist projects are deprecated and the main callsites were in releeph. conduit end point still works though!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11586
Summary: Revamps Profile to be like Projects, a mini portal and side nav with icons.
Test Plan: Viewed my own profile, as well as others. Test seeing my commits, tasks, diffs, and upcoming events. Checked mobile navigation.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11547
Summary: Use `PhutilXHPASTBinary` methods instead of `xhpast_parse` functions. Depends on D11517.
Test Plan: N/A, this is a direct swap.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11612
Summary: This sets an icon for each config, makes it easier to scan.
Test Plan:
Reload Config page, see all new icons
{F281089}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11619
Summary: Clean up the error view styling.
Test Plan:
Tested as many as I could find, built additional tests in UIExamples
{F280452}
{F280453}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11605
Summary: Ref T6881. This won't do much of interest on third party installs yet, but it's stable and we don't need to hold it back any longer.
Test Plan: Ran `phd start`, saw the trigger daemon start up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11603
Summary: Ref T6881. If we can't automatically bill an invoice, send the account owners a mail explaining why and asking them to pay it.
Test Plan: {F279596}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11602
Summary:
Ref T6881.
- Fix dead links.
- Let implementations provide more information.
- Provide more information to implementations.
Test Plan: Links work, invoices show billing periods, fewer "Subscription 6" crumbs, all is well in the world.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11601
Summary:
Ref T6881.
- Allow users to set a default payment method for a subscription, which we'll try to autobill (not all payment methods are autobillable, so we can't require this in the general case, and a charge might fail anyway).
- If a subscription has an autopay method, try to automatically bill it.
- Otherwise, we'll send them an email like "hey here's a bill, it couldn't autopay for some reasons, go pay it and fix those if you want".
- (That email doesn't exist yet but there's a comment about it.)
- Also some UI cleanup.
Test Plan:
- Used `bin/phortune invoice` to autobill myself some fake test money.
{F279416}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11596
Summary: Self-explanatory. Also made a few methods `final`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11598
Summary: Ref T7094. The class DiffusionRequest has other public methods which use getUser() in an unguarded way. Code inspection of the call sites for loadCommit() also leads me to believe the $user is properly set.
Test Plan: clicked around diffusion a bunch and everything seemed to work okay. (happy to test any particular esoteric endpoints that come to mind)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11585
Summary:
Ref T6881. This is basically just some UX.
Right now, if we invoice you, you can //technically// pay it but since we don't tell you about it and don't show it in the UI you'd have to guess the ID by manipulating the URI. We should probably be at least a little more aggressive about billing.
In the common case when we generate a cart/order, we don't show it to the user or merchant in Phortune until the user takes a payment action (basically, Phortune doesn't recognize the cart until you actually check out with it). In the current use case in Fund (and other reasonable use cases) an un-acted-upon cart hasn't been ordered yet, and is just a place for the application to store state as it hands off the workflow to Phortune.
Even if we had a real "Shop for physical goods" app, I think the same rule would apply -- the application itself would probably track and show your current cart, but it wouldn't make sense to put it into your order history in Phortune until you actually buy it.
Since invoices from subscriptions are essentially identical to not-yet-ordered-carts, that mean they also did not show up in the UI (although I think this is also desirable).
This change carves out a place for them:
- Add an "invoices" section with unpaid invoices.
- The UI shows that you have unpaid invoices.
- Invoices have a slightly different rendering, inclduing an alluring "Pay Now" button.
Some considerations:
- One thing I'm vaguely thinking about is the possibilty that users may be able to invoice one another directly, eventually. For example, we might invoice a contracting client.
- Considering this, I thought about making these carts have a special status like `STATUS_DUE`, which replaces `STATUS_READY`, or a flag like `isInvoice`.
- However, this approach was pretty involved and made the //billing// logic more complicated, so I backed off. The ultimate approach here puts more of the complexity into the display logic, which feels better to me.
- We might need an `isInvoice` flag eventually, but `subscriptionPHID` is a reasonable stand-in for now.
- The OrderTable serving double duty for rendering subscriptions feels a little muddy, but I think splitting it into two highly-redundant classes would be worse.
Test Plan:
{F279348}
{F279349}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11593
Summary: The method is actually named `DivinerAtomRef::newFromDictionary`.
Test Plan: `./bin/diviner generate --publisher DivinerStaticPublisher` worked a bit better.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11590
Summary: Allow the `DivinerPublisher` subclass to be specified via `./bin/divner generate --publisher ...`. In particular, this allows use of the (mostly broken) `DivinerStaticPublisher`.
Test Plan: Ran `./bin/diviner generate --publisher DivinerStaticPublisher`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11588
Summary: Minor tidying and modernizing a few things.
Test Plan: Ran `./bin/diviner atomize` and `./bin/diviner generate`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11587
Summary: Ref T7094.
Test Plan: couldn't really test this - how does one get symbols going nowadays given they are acanist project based?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11584
Summary:
Ref T6881. This generates a product, purchase and invoice for users, and there's sort of some UI for them. Stuff it doesn't do yet:
- Try to autobill when we have a CC;
- actually tell the user they should pay it;
- ask the application for anything like "how much should we charge", or tell the application anything like "the user paid".
However, these work:
- You can //technically// pay the invoices.
- You can see the invoices you paid in the past.
Test Plan: Used `bin/phriction invoice` to double-bill myself over and over again. Paid one of the invoices.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11580
Summary: Ref T7094. Could just delete this end point too I guess? Needed to add "withCommitPHIDs" to the differentialrevisionquery to get this done.
Test Plan: used diffusion.getcommits from conduit console and got a sensible result for a query for two commits, one with a diff and one without.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11581
Summary:
Ref T6881. This adds the worker, and a script to make it easier to test. It doesn't actually invoice anything.
I'm intentionally allowing the script to double-bill since it makes testing way easier (by letting you bill the same period over and over again), and provides a tool for recovery if billing screws up.
(This diff isn't very interesting, just trying to avoid a 5K-line diff at the end.)
Test Plan: Used `bin/phortune invoice ...` to get the worker to print out some date ranges which it would theoretically invoice.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11577
Summary:
Ref T6881.
- Add a subscription detail page.
Minor cosmetics:
- Fix glyph, from "X" (old "X marks the spot" icon) to "diamond" (new gem icon).
- Name the initial account "Default Account" instead of "Personal Account", since this seems more general.
Test Plan:
{F278623}
And I got two full days to test that Jan 30/31 -> Feb 28 billing logic!
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11576
Summary:
Ref T6881. This still doesn't "work" in any reasonable sense of the word, but gets us a bit further.
I'll build out the Phortune UI a little bit next, then look at implementing the Worker to do actual billing.
Test Plan:
- Allocated an instance and saw a Subscription generate properly.
- Saw subscription show up in the Phortune UI, albeit in a very limited way.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11575
Summary:
Ref T7094. We should do a policy query on the files IMO because there exists a scenario where the file gets locked down directly. This requires being a bit more disciplined about setting user, which in turn requires deciding whether or not to show edit / reply links as a separate piece of logic, not conditional on user presence.
This is not the best code but I don't think it gets worse with this and is just some other nuance in any larger cleanup we take on someday.
Test Plan: looked at a revision and noted inline comments rendered correctly with reply / edit actions. looked at a diff standalone and noted no reply / edit actions as expected. looked at a "details" link on a transaction and it rendered correctly. looked at a diff in phriction of page edits and it looked good. grepped around and verified the remaining callsite in diffusion already has the setUser call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11579
Summary: Fixes T1476. The body of the email should be just the output of some diff command.
Test Plan:
git diff master > text.txt; ./bin/mail receive-test --to <configured-diff-create-address> < text.txt; a diff was successfully created...! email generated had a working link to the diff.
./bin/mail receive-test --to <configured-diff-create-address> < README.md; a diff was not created as expected...! email generated had a sensical error message, telling me that the mail body should have been generated via a diff command
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: johnny-bit, Korvin, epriestley
Maniphest Tasks: T1476
Differential Revision: https://secure.phabricator.com/D11574
Summary: Main plan is to give conversations in Conpherence or Durable Column a different, lighter, chatty feel like Phriction.
Test Plan:
Tested a couple of threads and remarkup styles.
{F278086}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11562
Summary: Fixes T6819. This isn't as useful as you might think and has one horribly buggy behavior - if you edit an object which has a description and a projects field, you can be unable to remove the associated project as the automagic association from the description kicks in. Further, since we've added the ability for applications to create multiple email addresses AND herald can react to those emails - say by programmatically adding projects - the known needs for this feature are basically 0. If this proves to be false we can maybe add some other syntax for these mentions - see T6819 for ideas / discussion.
Test Plan: removed a project from a maniphest task while still mentioning it in the description and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6819
Differential Revision: https://secure.phabricator.com/D11573
Summary: Fixes T3404 (post D11565), fixes T5952. This infrastructure has been getting deployed against Maniphest and its time to get these other two applications going on it.
Test Plan: created an email address for paste and used `./bin/mail receive-test` ; a paste was successfully created
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952, T3404
Differential Revision: https://secure.phabricator.com/D11570
Summary: Ref T3404. The only mildly sketchy bit is these codepaths all load the application email directly, by-passing privacy. I think this is necessary because not getting to see an application doesn't mean you should be able to break the application by registering a colliding email address.
Test Plan:
Tried to add a registered application email to a user account via the web ui and got a pretty error.
Ran unit tests.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404
Differential Revision: https://secure.phabricator.com/D11565
Summary: due to typehints, passing null is going to barf here. Ref D11564, ref T5039.
Test Plan: made an edit to a task from the web ui and it didnt fatal
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11571
Summary:
Hit this locally, with an error like:
> Version <empty string> is older than 1.9, the minimum supported version.
(Where `<empty string>` was just the empty string, not literally the text `<empty string>`.)
Be more careful about parsing versions, and parse the newer string.
Test Plan: Got "unknown version" with intentionally-broken test data, then clean readout.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11558
Summary: Fix 'No Conpherences' layout, add 'Recent' label to list.
Test Plan: test with and without a list of threads.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11569
Summary:
Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???"
Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective.
Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11564
Summary: This adds a check to make sure the credential exists when loading it in the Drydock SSH interface. This effectively turns a fatal error (calling a method on a non-object) into a catchable exception.
Test Plan: Had a badly configured resource, saw the exception appear instead of daemon fataling.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11530
Summary: Fixes T7007. Using `%B` permits non-UTF8 data to be appended to Harbormaster build logs. Since we're not really in control of the processes Harbormaster is running remotely, and since they may output invalid UTF8 data, we should store the invalid data instead of failing the build (due to UTF8 exception).
Test Plan: @epriestley said this was the right fix, though I haven't tested it on our production system which actually exhibits the issue yet.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7007
Differential Revision: https://secure.phabricator.com/D11532
Summary: Fixes T7034. Like HTTP, proxy requests to the correct host if a repository has an Almanac service host.
Test Plan: Ran VCS requests through the proxy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034
Differential Revision: https://secure.phabricator.com/D11543
Summary: Ref T5039. This will be necessary for Herald integration so users can make rules like "if app email is one of x, y, or z add projects foo, bar, and metallica." I think its best to do an actual typeahead here -- users select full email addresses -- rather than support prefix, suffix, etc stuff on the email address. I think the latter approach would yield lots of confusion, as well as prevent us from (more) easily providing diagnostic tools about what happened when and why.
Test Plan: hacked a maniphest tokenizer to use this new datasource and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11546
Summary: In Maniphest, we say "X closed <task> by committing <commit>". In Differential, we currently say "X closed <revision> by commit <commit>", which sounds nongrammatical to me.
Test Plan: grammar'd
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11544
Summary: Ref T5952. This adds support for a "default author" and deploys it on Maniphest.
Test Plan: used augmented (by this diff) bin/mail receive-test to test creation via an application email with a default author configured and no author specified. a task was created with the author as the default author i configured.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952
Differential Revision: https://secure.phabricator.com/D11446
Summary:
Ref T7034.
In a cluster environment, when a user connects with a VCS request over SSH (like `git pull`), the receiving server may need to proxy it to a server which can actually satisfy the request.
In order to proxy the request, we need to know which repository the user is interested in accessing.
Split the SSH workflow into two steps:
# First, identify the repository.
# Then, execute the operation.
In the future, this will allow us to put a possible "proxy the whole thing somewhere else" step in the middle, mirroring the behavior of Conduit.
This is trivially easy in `git` and `hg`. Both identify the repository on the commmand line.
This is fiendishly complex in `svn`, for the same reasons that hosting SVN was hard in the first place. Specifically:
- The client doesn't tell us what it's after.
- To get it to tell us, we have to send it a server capabilities string //first//.
- We can't just start an `svnserve` process and read the repository out after a little while, because we may need to proxy the request once we figure out the repository.
- We can't consume the client protocol frame that tells us what the client wants, because when we start the real server request it won't know what the client is after if it never receives that frame.
- On the other hand, we must consume the second copy of the server protocol frame that would be sent to the client, or they'll get two "HELLO" messages and not know what to do.
The approach here is straightforward, but the implementation is not trivial. Roughly:
- Start `svnserve`, read the "hello" frame from it.
- Kill `svnserve`.
- Send the "hello" to the client.
- Wait for the client to send us "I want repository X".
- Save the message it sent us in the "peekBuffer".
- Return "this is a request for repository X", so we can proxy it.
Then, to continue the request:
- Start the real `svnserve`.
- Read the "hello" frame from it and throw it away.
- Write the data in the "peekBuffer" to it, as though we'd just received it from the client.
- State of the world is normal again, so we can continue.
Also fixed some other issues:
- SVN could choke if `repository.default-local-path` contained extra slashes.
- PHP might emit some complaints when executing the commit hook; silence those.
Test Plan: Pushed and pulled repositories in SVN, Mercurial and Git.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034
Differential Revision: https://secure.phabricator.com/D11541
Summary: Add a setBorder call to CrumbsView to be more deliberate when a border is drawn. Could not find any CSS hacks to set it conditionally CSS.
Test Plan: Browsed every application that called crumbs and make a design decision. Also fixed a few bad layouts.
Reviewers: btrahan, epriestley
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11533
Summary: Swaps out AphrontPanels for ObjectBoxes. I'd like to start reducing the floating object lists around the site for consistency. Also, these should provide more items above the fold.
Test Plan:
Test on my local homepage. Built a fake welcome.html too, though I think that's deprecated.
{F277020}
{F277021}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11529
Summary:
Ref T2086. Ref T7014. With the persistent column, there is significant value in retaining chrome state through navigation events, because the user may have a lot of state in the chat window (scroll position, text selection, room juggling, partially entered text, etc). We can do this by capturing navigation events and faking them with Javascript.
(This can also improve performance, albeit slightly, and I believe there are better approaches to tackle performance any problems which exist with the chrome in many cases).
At Facebook, this system was "Photostream" in photos and then "Quickling" in general, and the technical cost of the system was //staggering//. I am loathe to pursue it again. However:
- Browsers are less junky now, and we target a smaller set of browsers. A large part of the technical cost of Quickling was the high complexity of emulating nagivation events in IE, where we needed to navigate a hidden iframe to make history entries. All desktop browsers which we might want to use this system on support the History API (although this prototype does not yet implement it).
- Javelin and Phabricator's architecture are much cleaner than Facebook's was. A large part of the technical cost of Quickling was inconsistency, inlined `onclick` handlers, and general lack of coordination and abstraction. We will have //some// of this, but "correctly written" behaviors are mostly immune to it by design, and many of Javelin's architectural decisions were influenced by desire to avoid issues we encountered building this stuff for Facebook.
- Some of the primitives which Quickling required (like loading resources over Ajax) have existed in a stable state in our codebase for a year or more, and adoption of these primitives was trivial and uneventful (vs a huge production at Facebook).
- My hubris is bolstered by recent success with WebSockets and JX.Scrollbar, both of which I would have assessed as infeasibly complex to develop in this project a few years ago.
To these points, the developer cost to prototype Photostream was several weeks; the developer cost to prototype this was a bit less than an hour. It is plausible to me that implementing and maintaining this system really will be hundreds of times less complex than it was at Facebook.
Test Plan:
My plan for this and D11497 is:
- Get them in master.
- Some secret key / relatively-hidden preference activates the column.
- Quicksand activates //only// when the column is open.
- We can use column + quicksand for a long period of time (i.e., over the course of Conpherence v2 development) and hammer out the long tail of issues.
- When it derps up, you just hide the column and you're good to go.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2086, T7014
Differential Revision: https://secure.phabricator.com/D11507
Summary:
Fixes T7019. In a cluster environment, pushes currently fail because the commit hook can't identify the instance.
For web processes, the hostname identifies the instance -- but we don't have a hostname in the hook.
For CLI processes, the environment identifies the instance -- but we don't have an environment in the hook under SVN.
Promote the instance identifier into the upstream and pack/unpack it explicitly for hooks. This is probably not useful for anyone but us, but the amount of special-purpose code we're introducing is very small.
I poked at trying to do this in a more general way, but:
- We MUST know this BEFORE we run code, so the normal subclassing stuff is useless.
- I couldn't come up with any other parameter which might ever be useful to pass in.
Test Plan: Used `git push` to push code through proxied HTTP, got a clean push.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11495
Summary:
Ref T7019. When we receive a `git clone https://` (or `git push` on HTTP/S), and the repository is not local, proxy the request to the appropriate service.
This has scalability limits, but they are not more severe than the existing limits (T4369) and are about as abstracted as we can get them.
This doesn't fully work in a Phacility context because the commit hook does not know which instance it is running in, but that problem is not unique to HTTP.
Test Plan:
- Pushed and pulled a Git repo via proxy.
- Pulled a Git repo normally.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11494
Summary:
Ref T7019. Ref T7034. In both proxying cases, we want to proxy the request but can not do so over Conduit.
Split the URI resolution apart from Conduit client construction so we can just pull an SSH or HTTP/S URI out of the repository without getting an entire Conduit client.
Test Plan: Browsed around a service-hosted repository. This diff has no behavioral changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034, T7019
Differential Revision: https://secure.phabricator.com/D11491
Summary:
Ref T6881. This roughs in the major objects, support classes, and controllers.
- Show subscriptions on account detail.
- Browse all account subscriptions.
- Link to active subsciptions from merchant detail.
Test Plan: Clicked around in the UI. There's no way to create subscriptions yet, so I basically just kicked the tires on this. I probably missed a few things that I'll clean up in followups.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11482
Summary:
Ref T7055. Apparently we just never had one? I feel like I'm crazy. But I can't find any trace in the logs.
I'm actually not 100% sold on this being better because it's a color glyph on OSX and those feel a little out of place / tacky to me compared to the black-and-white ones. So I'd be fine with just leaving it off, too. Clearly not important if no one noticed it until I caught it in T7055.
Test Plan: {F276917}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7055
Differential Revision: https://secure.phabricator.com/D11524
Summary: Unused at this point
Test Plan: Grep
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11506
Summary: Adds in the sidenav
Test Plan: Click on sidenav, see it persist
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11526
Summary: With the new magic controller switcher, these links are needed.
Test Plan: Look at list of Projects
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11510
Summary: Adds it back
Test Plan: Give token, view story
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11498
Summary: Removes the 1x application icons, and uses the fonticons instead. Feed was only known location.
Test Plan:
feed, dashboards, grep for use
{F275636}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11496
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary:
Ref T2783. I think this served two purposes:
- Improving performance in cases where we "know" a repository is local.
- Preventing loops.
It is now obsolete:
- After D11476, refs can almost always resolve on a fast path.
- As T2783 moves forward, we can usually no longer know when a repository is local without actually looking it up -- almost everything is allowed to run anywhere.
- The cluster behavior in D11475 now prevents loops.
Test Plan: `grep`, browsed around. This didn't really do much of anything anymore.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11477
Summary:
Ref T2783. With service-oriented calls, we take a larger performacne hit than necessary resolving refs.
Instead of resolving refs over the wire, try to resolve them from the database first. This can resolve almost all refs (commit hashes, branch and tag names).
This can't resolve weird refs like `master~50`, and obviously can't resolve invalid refs. In those cases we'll go back to the old logic, call `diffusion.resolverefs`, and end up with the right result.
Test Plan:
- Browsed repositories in Diffusion.
- Verified that service repositories no longer make unnecessary `diffusion.resolverefs` calls for common refs (branch names, commit hashes).
- Resolved refs like `master~50`, saw call to underlying VCS and correct result.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11476
Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.
This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.
In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).
Test Plan:
- With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
- With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11475
Summary: Ref T7020. I need this elsewhere, and it's relatively internal anyway.
Test Plan: Browsed around my local, cluster-configured install and saw everything working fine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11474
Summary: Fixes T7021. When I moved around all the timeline stuff I guess I didn't find this "corner" case, which is wildly common in the post-commit review workflow that we don't use.
Test Plan: pre-patch I could reproduce the issue and post patch I could not. The reproduction case is to have a commit with inline comments and then enough subsequent comments to have a "show older" UI. clicking "show older" now works!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7021
Differential Revision: https://secure.phabricator.com/D11479
Summary: Fixes T7011. Recent refactoring here caused us to begin ignoring URI parameters like `commit`. Most controllers take parameters as a `dblob`, which was still parsed properly.
Test Plan:
- Editing different commits actually edits the desired commits.
- Browsed around some `dblob` pages and verified they still work properly.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7011
Differential Revision: https://secure.phabricator.com/D11473
Summary:
Ref T5833. In some cases, we need to know if an Almanac device is the localhost or not, so we can either handle or forward the request.
To accomplish this, write a device ID when running `bin/almanac register`.
Using `--allow-key-reuse` and `--identify-as`, multiple devices are permitted to //authenticate// as one device but //identify// as different devices. In the Phacility cluster, this allows all the `repoXXX` machines to have one keypair (making key management much easier) but still work as separate devices. This is an advanced feature; normal installs with 1-3 hosts would just generate a key + device per host and identify/authenticate as the same device.
Test Plan: Ran commands with lots of flags like `PHACILITY_INSTANCE=local sudo -E ./bin/almanac register --device daemon.phacility.net --private-key ~/dev/core/conf/keys/daemon.key --force --allow-key-reuse --identify-as local001.phacility.net`. Got a good result from `AlmanacKeys::getDeviceID()` afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D11452
Summary: Fixes T6890. This doesn't feel like a perfect solution, but I can't think of any cases in which this will produce the wrong result either.
Test Plan: Ran `./bin/diviner generate` and checked the generated documentation for `PhabricatorCommonPasswords::loadWordlist()`. The return type was corrected shown as `map<string, bool>`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6890
Differential Revision: https://secure.phabricator.com/D11469
Summary: As suggested in T6950, add the method description to the response from `conduit.query`.
Test Plan: Called `echo '{}' | arc call-conduit conduit.query` and verified that the response contained the method description.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11467
Summary: Fixes T6950. Adds the return type of Conduit API methods to the `conduit.query` call.
Test Plan: Called `echo '{}' | arc call-conduit conduit.query` and verified that the return types were present in the response.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6950
Differential Revision: https://secure.phabricator.com/D11466
Summary:
Fixes T6858. We shouldn't create mentions for dependent diffs.
NOTE: This won't fix the issue for existing revisions (which have the mentions edge), but I think that this is harmless.
Test Plan: Added `Depends on Dxxx` to a differential summary. Saw a `josh added a dependent revision` transaction, but no explicit mention.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6858
Differential Revision: https://secure.phabricator.com/D11460
Summary: Ref T5833. This was using the wrong constant, so we weren't validating property.
Test Plan: Tried to create a nameless network and correctly got an error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D11447
Summary: Fixes T6989. Basically return a nice dialogue like we do for "NoEffect" transactions. This is a little prettier than the other dialogue was. Also, stop adding TYPE_EDGE as a transaction type as we end up having it 2x, which then makes the error get validated 2x.
Test Plan: tried to add myself as a reviewer and got a nice error message.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6989
Differential Revision: https://secure.phabricator.com/D11448
Summary:
Ref T6881. I tried to cheat here by not implementing this, but we need it for destroying triggers directly with `bin/remove destroy`, since that needs to load them by PHID.
So, cheat slightly less. Implement PolicyAware but not CursorPagedPolicyAware.
Test Plan:
- Used `bin/remove destroy` to destroy a trigger by PHID.
- Browsed daemon console.
- Ran trigger daemon.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11445
Summary: Ref T5952, T3404. This lays the basic plumbing for how this will work, all the way to deploying on Maniphest. Aside from what is mentioned on T5952, I think page(s) on editing application emails could use a little more helpful text about what's going on, similar to how the config page that's getting deprecated works.
Test Plan: ran migration and noted my create email address migrated successfully. used bin/mail to make a task. added another email and used bin/mail to make a task. deleted an email. edited an email. invoked various error states and they all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404, T5952
Differential Revision: https://secure.phabricator.com/D11418
Summary: Fixes T6963. Long term will likely make this more like other document views, but not worth the time right now since this is only location.
Test Plan: Review Phriction document at desktop and mobile breakpoints. Click menu and see menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6963
Differential Revision: https://secure.phabricator.com/D11420
Summary:
Taking a pass at revamping the edit pages in Projects. Specifically:
- Remove EditMainController
- Move actions from EditMain to Profile
- Move properties from EditMain to Profile
- Move timeline from EditMain to Profile
- Move Open Tasks from Profile to sidenavicon
- Add custom icons and colors to timeline
Feel free to bang on this a bit and give feedback, feels generally correct to me.
Test Plan: Edit everything I could on various projects. Check links, timelines, actions.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11421
Summary:
Ref T6881. Hopefully, this is the hard part.
This adds a new daemon (the "trigger" daemon) which processes triggers, schedules them, and then executes them at the scheduled time. The design is a little complicated, but has these goals:
- High resistance to race conditions: only the application writes to the trigger table; only the daemon writes to the event table. We won't lose events if someone saves a meeting at the same time as we're sending a reminder out for it.
- Execution guarantees: scheduled events are guaranteed to execute exactly once.
- Support for arbitrarily large queues: the daemon will make progress even if there are millions of triggers in queue. The cost to update the queue is proportional to the number of changes in it; the cost to process the queue is proportional to the number of events to execute.
- Relatively good observability: you can monitor the state of the trigger queue reasonably well from the web UI.
- Modular Infrastructure: this is a very low-level construct that Calendar, Phortune, etc., should be able to build on top of.
It doesn't have this stuff yet:
- Not very robust to bad actions: a misbehaving trigger can stop the queue fairly easily. This is OK for now since we aren't planning to make it part of any other applications for a while. We do still get execute-exaclty-once, but it might not happen for a long time (until someone goes and fixes the queue), when we could theoretically continue executing other events.
- Doesn't start automatically: normal users don't need to run this thing yet so I'm not starting it by default.
- Not super well tested: I've vetted the basics but haven't run real workloads through this yet.
- No sophisticated tooling: I added some basic stuff but it's missing some pieces we'll have to build sooner or later, e.g. `bin/trigger cancel` or whatever.
- Intentionally not realtime: This design puts execution guarantees far above realtime concerns, and will not give you precise event execution at 1-second resolution. I think this is the correct goal to pursue architecturally, and certainly correct for subscriptions and meeting reminders. Events which execute after they have become irrelevant can simply decline to do anything (like a meeting reminder which executes after the meeting is over).
In general, the expectation for applications is:
- When creating an object (like a calendar event) that needs to trigger a scheduled action, write a trigger (and save the PHID if you plan to update it later).
- The daemon will process the event and schedule the action efficiently, in a race-free way.
- If you want to move the action, update the trigger and the daemon will take care of it.
- Your action will eventually dump a task into the task queue, and the task daemons will actually perform it.
Test Plan:
Using a test script like this:
```
<?php
require_once 'scripts/__init_script__.php';
$trigger = id(new PhabricatorWorkerTrigger())
->setAction(
new PhabricatorLogTriggerAction(
array(
'message' => 'test',
)))
->setClock(
new PhabricatorMetronomicTriggerClock(
array(
'period' => 33,
)))
->save();
var_dump($trigger);
```
...I queued triggers and ran the daemon:
- Verified triggers fire;
- verified triggers reschedule;
- verified trigger events show up in the web UI;
- tried different periods;
- added some triggers while the daemon was running;
- examined `phd debug` output for anything suspicious.
It seems to work in trivial use case, at least.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11419
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11415
Summary: Ref T6822. This method needs to be `public` because it is called from `PhabricatorApplicationSearchController::buildApplicationMenu()`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11416
Summary: Ref T6822. This method is only called from `PhutilDaemon::execute()` and can be made `protected`.
Test Plan: See D11404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11405
Summary: Ref T6822. This method is only called from within the `PhabricatorWorker::executeTask()` and `PhabricatorWorker::scheduleTask()` methods.
Test Plan: `grep`ped for `->doWork`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11406
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from `LiskDAO::update()`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11409
Summary: Ref T6944. This was not quite implemented correctly in D11387.
Test Plan: Saw no more exceptions about being unable to create `/var/log`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: maxhodak, Korvin, epriestley
Maniphest Tasks: T6944
Differential Revision: https://secure.phabricator.com/D11397
Summary: ...also adds policies on who can view and who can edit an action. Fixes T6949.
Test Plan: viewed a secret through the new UI and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6949
Differential Revision: https://secure.phabricator.com/D11401
Summary: Ref T6962. Mainly accomplished by re-factoring the base editor `buildMailBody` function and then using it differently in the `DifferentialTransactionEditor`.
Test Plan: commented on a revision leaving inline feedback. inspected via bin/mail and it looked good! also made a maniphest comment and checked that email, which still looked good.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6962
Differential Revision: https://secure.phabricator.com/D11402
Summary: Fixes T6959.
Test Plan: When I was ready to test the feature, the "Daemon & Web config" error already showed up, from having added phd.variant-config. I went meta and changed the value of phd.variant-config to have phd.variant-config. The config error disappeared. I then changed the conpherence setting about conpherence email prefix and the error showed up again. Removing the conpherence config setting made the error disappear once more.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6959
Differential Revision: https://secure.phabricator.com/D11399
Summary: Fixes T6944. Attempt to automatically create the log directory for the Aphlict server. If the directory can't be created, throw a helpful exception.
Test Plan:
# Set `notification.log` to `/var/log/aphlict/aphlict.log`.
# Ran `./bin/aphlict debug` and saw an exception (because the user doesn't have permissions to create the `/var/log/aphlict` directory).
# Ran `sudo chmod 777 /var/log`.
# Ran `./bin/aphlict debug` and saw the `/var/log/aphlict` directory created.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6944
Differential Revision: https://secure.phabricator.com/D11387
Summary: I assume we've shown this long enough, plus with redesign it's a good time to remove.
Test Plan: reload page, no link
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11394
Summary: We still seem to reach for this, though may be time to remove Wiki?
Test Plan: view link
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11393
Summary: In Maniphest, we provide an additional caption shortcut if you can create projects, which has no use if you cant. Fixes T6969
Test Plan: Check page with and without a user's capability.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6969
Differential Revision: https://secure.phabricator.com/D11390
Summary: Ref T6971. This fixes the error the user reported. Not sure what's up with the root cause of their issue.
Test Plan: Went to `/auth/config/new/asdfqwer/` and got a 404 instead of an exception.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6971
Differential Revision: https://secure.phabricator.com/D11388
Summary: Fixes T6955.
Test Plan: made an oauth app. made a test authorization. ran bin/remove destroy <phid of oauth client> and there were no errors. verified oauth app and test authorization were both gone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6955
Differential Revision: https://secure.phabricator.com/D11378
Summary: Fixes T6957. If / when a policy object is destroyed, access to an object that uses that policy object is denied.
Test Plan: looked around in the code to fail confident enough to write the summary above
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6957
Differential Revision: https://secure.phabricator.com/D11380
Summary: We could still miss this if the policy had never been customized and we returned early after one of the other checks.
Test Plan:
Works great on instances now.
{F267067}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11374
Summary: Third time lucky... the filename should match the class name now.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11362
Summary: Fast commit. Also forgot to make the config override the existing policy. I *think* this is the right spot and we're good? Ref T6947.
Test Plan: viewed the application settings page for people application and saw the correct overrode setting.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6947
Differential Revision: https://secure.phabricator.com/D11373
Summary: Ref T5833. This doesn't do anything yet, but will allow new instances to automaticaly bind to an open database without anything too hacky.
Test Plan:
Created a service of this type.
{F267059}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D11372
Summary: Fixes T6947
Test Plan:
locked people.create.user and noted the UI only showed a link to the existing policy with no way to edit it.
tried to set the config to all the various bad things and saw helpful error messages telling me what I did wrong.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6947
Differential Revision: https://secure.phabricator.com/D11358
Summary: Not sure this is obvious enough, but maybe future apps will use as well?
Test Plan: test a project with and without a workboard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11371
Summary: Ref T6947.
Test Plan: made the setting say only admin user a and noted admin user b lost access
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4137, T6947
Differential Revision: https://secure.phabricator.com/D11357
Summary: Reduces visual duplication in a few places.
Test Plan: Review pages in sandbox, see image removed.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11354
