Summary:
Ref T7447. Fixes T7600. This likely needs significant adjustment, but implements content-aware comment porting for line changes.
Specifically, this moves lines around to adjust their position considering added and removed lines between the diffs and across rebases.
It does not try to do any actual content (line against line) matching.
Test Plan:
- Unit tests.
- Poking around in the web UI seems to generate mostly reasonable-ish results?
- This may be a huge step backward in some cases that I just haven't hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7600, T7447
Differential Revision: https://secure.phabricator.com/D12741
Summary: Ref T7776. This could get better, but I think I got most of the big stuff. It's ~4x faster now.
Test Plan:
Before:
{F393338}
After:
{F393339}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7776
Differential Revision: https://secure.phabricator.com/D12730
Summary: Ref T7447. At least some users dislike this feature so strongly that they'd prefer not to have it at all.
Test Plan: Viewed ghosts; toggled preference, no more ghosts.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12704
Summary: See D12702.
Test Plan: Made something a link and clicked it, seemed to work OK.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim, epriestley
Differential Revision: https://secure.phabricator.com/D12703
Summary: Closes T7928, E{id} is available via global search.
Test Plan: Create calendar event, search for its monogram in global search, event should be accessible.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7928
Differential Revision: https://secure.phabricator.com/D12581
Summary: Seems reasonable? At least, it always matches however a user might think about documents (app or document). Unclear if "Diffusion" for example, are actually needed.
Test Plan: tested searching for "phriction", "wiki", "document", etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12577
Summary: "Authors" and "Subscribers" convert easily without any extra work.
Test Plan: Used both fields; used functions and normal tokens.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12556
Summary:
See M1433. Fixes T7266. Fixes T4475. Ref T7314.
Future work/notes/etc:
- Write the User Guide (see TODO).
- This might needs some design tweaks -- I think it's functionally almost-equivalent to the mock, but the UI isn't quite the same.
- (Mobile design is a touch off-looking I think?)
- When you use a custom query, the duplicate "magnifying glass" icons are a little weird. Maybe change one or the other.
- Maybe worth adding an "Open Documents in Current Application" option? Planning to wait for feedback on that.
- Need a Quicksand integration to change the current application at some point.
- Searching in "Current Application" from, e.g., the 404 page just searches all documents. Current plan is to just document this behavior, since the icon is a pretty good callout and it seems plausible that this is intuitive enough that users won't have a hard time with it.
Test Plan:
New dropdown:
{F379150}
Device-ish:
{F379151}
Normal search (current application, from maniphest, selects tasks):
{F379153}
Application search from non-application:
{F379154}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: johnny-bit, epriestley
Maniphest Tasks: T7266, T7314, T4475
Differential Revision: https://secure.phabricator.com/D12509
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 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. 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.
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: 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. 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 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. 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. 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. 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:
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. 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: 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 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. 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. 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. 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 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 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 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 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: 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 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:
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. 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 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:
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: 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 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:
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: 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: 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 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: 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 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: 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:
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: 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: 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: 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: 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: 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: 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:
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: 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: 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 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 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: 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: 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: 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:
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: 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 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 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: Ref T5752, moves mobile action menus to the object box instead of crumbs.
Test Plan: View action menus at tablet, desktop, and mobile break points. Verify clicking buttons works as expected opening menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5752
Differential Revision: https://secure.phabricator.com/D11340
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within `PhabricatorController` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11241
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplicationSearchEngine` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11242
Summary: CLosed is a pretty important state and black tends to blend in a bit. This bumps to an alternate color to improve ability to scan and know state of objects.
Test Plan:
Review a number of closed objects. I will follow up with another diff on 'Archived' colors.
{F261895}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11222
Summary: Fixes T6608, though I'll also clean up the comment for PhutilStringTruncator in another diff. If I understand correctly, before T1191, MySQL column length was by character count and post T1191 its by byte count. Ergo, most of these changes are going from codepoint -> bytes. See test plan for complete list of what was and was not done.
Test Plan:
Thought very carefully about each callsite and made changes as appropos. "Display" means the string is clearly used for display-only purposes and correctly uses "glyph" already.
grep -rn PhutilUTF8StringTruncator *
applications/calendar/query/PhabricatorCalendarEventSearchEngine.php:217: ->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
applications/chatlog/controller/PhabricatorChatLogChannelLogController.php:111: $author = id(new PhutilUTF8StringTruncator()) -- display
applications/conduit/method/ConduitConnectConduitAPIMethod.php:62: $client_description = id(new PhutilUTF8StringTruncator()) -- was codepoint, changed to bytes
applications/conpherence/view/ConpherenceFileWidgetView.php:22: ->setFileName(id(new PhutilUTF8StringTruncator()) -- display
applications/differential/controller/DifferentialDiffViewController.php:65: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/event/DifferentialHovercardEventListener.php:69: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/parser/DifferentialCommitMessageParser.php:144: $short = id(new PhutilUTF8StringTruncator()) -- was glyphs, made to bytes
applications/differential/view/DifferentialLocalCommitsView.php:80: $summary = id(new PhutilUTF8StringTruncator()) -- display
applications/diffusion/controller/DiffusionBrowseFileController.php:686: id(new PhutilUTF8StringTruncator()) -- display
applications/feed/story/PhabricatorFeedStory.php:392: $text = id(new PhutilUTF8StringTruncator()) -- display, unless people are saving the results of renderSummary() somewhere...
applications/harbormaster/storage/build/HarbormasterBuild.php:216: $log_source = id(new PhutilUTF8StringTruncator()) -- was codepoints now bytes
applications/herald/storage/transcript/HeraldObjectTranscript.php:55: // NOTE: PhutilUTF8StringTruncator has huge runtime for giant strings. -- not applicable
applications/maniphest/export/ManiphestExcelDefaultFormat.php:107: id(new PhutilUTF8StringTruncator()) -- bytes
applications/metamta/storage/PhabricatorMetaMTAMail.php:587: $body = id(new PhutilUTF8StringTruncator()) -- bytes
applications/people/event/PhabricatorPeopleHovercardEventListener.php:62: id(new PhutilUTF8StringTruncator()) -- display
applications/phame/conduit/PhameCreatePostConduitAPIMethod.php:93: id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/pholio/storage/PholioTransaction.php:300: id(new PhutilUTF8StringTruncator()) -- display
applications/phortune/provider/PhortuneBalancedPaymentProvider.php:147: $charge_as = id(new PhutilUTF8StringTruncator()) -- bytes
applications/ponder/storage/PonderAnswerTransaction.php:86: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:267: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:276: id(new PhutilUTF8StringTruncator()) -- display
applications/repository/storage/PhabricatorRepositoryCommitData.php:43: $summary = id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:20: $data->setAuthorName(id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/slowvote/query/PhabricatorSlowvoteSearchEngine.php:158: $item->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php:317: $host = id(new PhutilUTF8StringTruncator()) -- bytes
view/form/control/AphrontFormPolicyControl.php:61: $policy_short_name = id(new PhutilUTF8StringTruncator()) -- glyphs, probably display only
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6608
Differential Revision: https://secure.phabricator.com/D11219
Summary: Show the full unit test name, including the namespace. Depends on D11208.
Test Plan: Inspected the "Table of Contents" of a diff created //with// D11208 and //without// D11208 applied.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11209
Summary: The default behavior was inadvertedly changed in D11074. This restores the original behavior.
Test Plan: Added a project reviewer to a diff, saw no inverse transaction recorded.
Reviewers: Krenair, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11181
Summary: Modernize remaining edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Browsed around and performed various actions include subscribing, unsubscribing and watching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11116
Summary: I was going to fix the variable name as it violates convention, but it is not used anyway.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11139
Summary: This class is no longer required after D10869.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11154
Summary:
These didn't get translated quite right:
- We need to use `$total_count` because some languages have different words for 1, 2-3, and 4+ things (for example). So the strings might translate as:
- alincoln added a reviewer-one ...
- alincoln added reviewers-few ...
- alincoln added reviewers-many ...
- That is, while English has only "reviewer" and "reviewers", other languages have more plural forms, and "reviewer", "reviewers-few" and "reviewers-many" may be completely different words.
- In English, because we know we always have 2+ in this branch and the only special word is for 1, we can just drop this.
- Anyway, the %4$s stuff is counting assuming that $total_count is included in the string, so these were a off by one.
- See also D11160.
There a probably a couple more of these, but they should be easy enough to hunt down as they crop up.
Test Plan: Saw nice strings instead of empty strings, or invalid strings (after D11160).
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11162
Summary:
Removes an unused PhabricatorFeedStory Parameter from all getTitleForFeed() and getApplicationTransactionTitleForFeed() functions.
Ref D11088 Ref T6545
Test Plan: ran all unit tests and viewed some dashboard feeds
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11146
Summary: Modernize Differential edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Krenair, epriestley
Differential Revision: https://secure.phabricator.com/D11074
Summary: Modernize Legalpad edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan:
# Created a Herald rule to require legal signatures on all diffs.
# Created a new diff.
# Saw the transaction string appear correctly.
I wasn't able to check the inverse transaction because there is none. Also, I couldn't see any text on the feed (presumably, transactions authored by Herald do not generate feed items)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Krenair, chad, epriestley
Differential Revision: https://secure.phabricator.com/D11082
Summary: Fix a few minor lint issues.
Test Plan: Ran `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11059
Summary: Fixes T6790. Turn the old method into "new" (old signature) and "newEphemeral". Deploy "newEphemeral" as many places as possible; basically places we are not in the Differential application *and* have no intentions of ever saving the diff. These callsites are also all places we are just trying to get some changesets at the end of the day.
Test Plan: set differential application policy to 'administrators only'. viewed a commit in diffusion and it worked without any errors! i'm just using my thinkin' noodle on the other code paths.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6790
Differential Revision: https://secure.phabricator.com/D11020
Summary: Fixes T6595. This diff has two issues as is... 1) the differential data fetching is pretty cheesey, but it looks like we can't just issue three separate databases to get the right data? 2) the translations break, since I am turning this into a string (and not an int) so the whole pluralization bit fails. I think 1 is okay as is and 2 needs to be fixed though I am not sure how to best do that...
Test Plan: loaded home page and it looked nice...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6595
Differential Revision: https://secure.phabricator.com/D10979
Summary: This adds back the title to the header link and scans through the codebase for instances where
Test Plan: Tested as many ObjectItemLists as I could find (each app homepage), there may be outliers, but can resolve those individually.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10961
Summary: Ref T3669. Probably. Adds a yellow warning at the top of the Diff View and makes the comment draft icon yellow on lists of revisions.
Test Plan:
Test a diff with many warnings, see warning. Test a diff with draft comments, see warning. Test new icon in list view.
{F230133}
{F230134}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3669
Differential Revision: https://secure.phabricator.com/D10789
Summary: Fixes T6699. We need to "loadInlineComments" consistently, though for an unexpected reason - this mutates the $changesets to include all $changesets that have an associated inline comment, which is necessary to make them render properly.
Test Plan: Took a diff with inline comments and updated it, noting the inline comments disappeared. applied this patch and the inlines reappeared.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6699
Differential Revision: https://secure.phabricator.com/D10935
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary:
Ref T4712. Specifically...
- Differential
- needed getApplicationTransactionViewObject() implemented
- Audit
- needed getApplicationTransactionViewObject() implemented
- Repository
- one object needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true)
- Ponder
- BONUS BUG FIX - leaving a comment on an answer had a bad redirect URI
- both PonderQuestion and PonderAnswer needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on both "history" controllers
- left a "TODO" on buildAnswers on the question view controller, which is non-standard and should be re-written eventually
- Phortune
- BONUS BUG FIX - fix new user "createNewAccount" code to not fatal
- PhortuneAccount, PhortuneMerchant, and PhortuneCart needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on Account view, merchant view, and cart view controller
- Fund
- Legalpad
- Nuance
- NuanceSource needed PhabricatorApplicationTransactionInterface implemented
- Releeph (this product is kind of a mess...)
- HACKQUEST - had to manually create an arcanist project to even be able to make a "product" and get started...!
- BONUS BUG FIX - make sure to "setName" on product edit
- ReleephProject (should be ReleepProduct...?), ReleephBranch, and ReleepRequest needed PhabricatorApplicationTransactionInterface implemented
- Harbormaster
- HarbormasterBuildable, HarbormasterBuild, HarbormasterBuildPlan, and HarbormasterBuildStep all needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) all over the place
Test Plan: foreach application, viewed the timeline(s) and made sure they still rendered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10925
Summary: I think this was a "hacked" sub thing that never got updated when we switched to a real editor? I am not 100% sure how these methods are used, so please let me know if I should expand my test plan. Fixes T6659.
Test Plan: made a diff from the web ui, looked up the phid from mysql, ran bin/remove destroy <phid>, and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6659
Differential Revision: https://secure.phabricator.com/D10911
Summary: Fixes T6658.
Test Plan: made a diff with no repository and default policy and it worked!
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6658
Differential Revision: https://secure.phabricator.com/D10910
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.
Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6152
Differential Revision: https://secure.phabricator.com/D10875
Summary: Fixes T6200. Ref T6237. When creating a diff from the web view, allow the user to select the repository at that time. When viewing a diff that has no associated revision and then creating a revision, pass along the repository phid to the create revision controller. Within the create revision controller, default the repository selector to this repository phid. Finally, in the editor, stop aggressively resetting the repository phid for every TYPE_UPDATE; rather, do so if its not a new object -- the diff should reign supreme in that case -- or if there's no repository -- let the diff be the guide.
Test Plan:
- made a diff with an associated repo, made a revision from the diff, saw the associated repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo and it stuck on save!
- made a diff with an associated repo, made a revision from the diff but changed the repo to nothing and it stuck on save!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6200
Differential Revision: https://secure.phabricator.com/D10872
Summary: Ref T6237. This sets us up for some future work like T6152, T6200 and generally cleaning up this workflow a bit. Tried to do as little as possible so not exposing transaction view yet. (Though that timeline is going to be a little funky in the common case of just the lone create transaction.)
Test Plan: made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237
Differential Revision: https://secure.phabricator.com/D10869
Summary: The shield is just confusing. In one case it doesn't work, and in the other case it just shows you a copy of the file you can see just below except in red. Fixes T4599, T1211. Note T1211 proposed not showing the "move away" file **at all** but I think removing the shield fixes the source of confusion. The code here is a bit if / else if / else if... heavy but this is logically sound.
Test Plan: made a diff where i moved a file then edited it in the new location. viewed diff, saw confusing shield, dropped caches, applied patch, viewed diff and saw no shield. made a diff where I moved a file and didn't edit in new location and saw similar shield disappearness.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T1211, T4599
Differential Revision: https://secure.phabricator.com/D10865
Summary: This upgrades 1up view from "does not work" back to "barely works".
Test Plan: view diff, 1up and 2up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10854
Summary: Fixes T3942, turns the load links into buttons.
Test Plan: Set my limit to 1, test page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3942
Differential Revision: https://secure.phabricator.com/D10775
Summary: Fixes T6343. Grepped for all callsites and added addLinkSection where needed.
Test Plan: Tested Differential, Maniphest, Conpherence, Ponder and Macro. Inspect HTML mail for anchor tags. Inspect text mails for non-disruption.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: talshiri, Korvin, epriestley
Maniphest Tasks: T6343
Differential Revision: https://secure.phabricator.com/D10762
Summary: Ref T6345, This adds more consistent color choices to match how Phabricator generally works across Differential/Diffusion per user statuses.
Test Plan: Review a few Audits in my sandbox.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T6345
Differential Revision: https://secure.phabricator.com/D10726
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary: pre-patch, we match on things like https / http and port... just match domains. Fixes T5693.
Test Plan: arc diff -> arc land and the diff was closed correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5693
Differential Revision: https://secure.phabricator.com/D10701
Summary:
I am not sure how valuable this is *as is* - I think it needs different explanations for what happened in mercurial or subversion? I do not know what those explanations are.
Made an error in D10485 - the $hashes that were saved is an array of objects, so it ends up turning into garbage via the wonders of serialization and de-serialization. Fix that by explicitly saving the tree hash.
I would like to make this work for the other VCS types we support, add the "undo / nope" button and call it fixed.
Ref T3686.
Test Plan: clicked "explan why" and saw why
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5693, T3686
Differential Revision: https://secure.phabricator.com/D10489
Summary: Default $phids to array() and update it if getValue() has something pertinent... Fixes T6292.
Test Plan: just used the ole logic noodle on this one.
Reviewers: chad, epriestley
Reviewed By: chad, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6292
Differential Revision: https://secure.phabricator.com/D10697
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit.
Test Plan:
- Published a Differential feed story into Asana with comment text.
- Pulbished a Diffusion feed story into Asana with comment text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10584
Summary: Fixes T6184. On a Revision page we don't show the date as an important piece of information, so it's also not likely useful on a Hovercard (and confusing as to what the date means).
Test Plan: Hover over a linked Diff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6184
Differential Revision: https://secure.phabricator.com/D10579
Summary: Fixes T6176. Language here is a bit awkard but I wanted to use the verb "removed" *and* still have the object first, so I ended up adding the before details parenthetically.
Test Plan: story no longer fatal'd in my feed
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: epriestley, Korvin
Maniphest Tasks: T6176
Differential Revision: https://secure.phabricator.com/D10549
Summary:
Ref T1191. This lays some groundwork for generating the expected schemata, so we can compare them to the actual schemata and produce a meaningful diff.
- In general, each application will subclass `PhabricatorConfigSchemaSpec` and provide a definition of the tables it expects.
- This class has helper methods to mostly-automatically build table definitions for Lisk and (in the future) edges.
- When building expected schema, we specify a "data type", like "epoch". This is the type of data the application stores in the column, from the application's point of view. The SchemaSpec converts this into the best avilable storage type: for example, "text" will translate to `utf8mb4` if it's availalbe, or `binary` if not. This gives us a layer of indirection to insulate us from craziness.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10497
Summary: Fixes T5536. Some bonus pht in there.
Test Plan: made a diff hovered over the stars and saw my new text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5536
Differential Revision: https://secure.phabricator.com/D10487
Summary:
Implements a new transaction - still TYPE_ACTION - but using a new DifferentialAction::ACTION_COMMIT_CLOSE. Augment rendering as necessary to display this new transaction. Saves enough information so T3686 is possible but stops short of implementing a popup to display this information. Fixes T5875. Ref T3686.
One small display oddity - this new transaction now renders at the top of the transaction group whereas when it was a comment it was on the bottom. I think this is basically okay but if not how fix? (Playing with the "strength" of these actions will mess up the email too?)
Test Plan: made a diff X that fixed task Y. committed. checked diff X, task Y, and the commit pages for proper transactions and all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3686, T5875
Differential Revision: https://secure.phabricator.com/D10485
Summary: Fixes T4036. Now if you say something on diff X like "This reminds me of Tx and Dy and commitHashFoo and Px." each of those objects gets a little visible transaction that the mention occurred. No feed, email, or notifications.
Test Plan: made a comment like above and verified transactions. also submitted a diff that "Fixes Tx" and Tx did not get the transaction as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T4036
Differential Revision: https://secure.phabricator.com/D10451
Summary:
Ref T6013. A very long time ago, edges were less clearly low-level infrastructure, and some user-aware stuff got built around edge edits.
This was kind of a mess and I eventually removed it, during or prior to T5245. The big issue was that control flow was really hard to figure out as things went all the way down to the deepest level of infrastructure and then came back up the stack to events and transactions. The new stuff is more top-down and generally seems a lot easier and cleaner.
Consequently, actors are no longer required for edge edits. Remove the parameter.
Test Plan: Poked around; ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10412
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary: Fixes T5915. Occasionally, users derp up and diff private key material. Adding a pre-write Herald phase enables configuration of a partial layer of protection that will reject these changes before they hit disk, provided they can be detected by, e.g., filename.
Test Plan:
- Added a rule with checks on every field, verified they looked fine in the transcript.
- Created some revisions to test those changes (I have a bunch of revision rules locally).
- Verified rejects don't write transcripts to the database.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5915
Differential Revision: https://secure.phabricator.com/D10305
Summary: Ref T5894. We have a couple more similar cases. Make them all do a decision-based redirect for now.
Test Plan: Did "View Raw File" and such, and also made sure thumbnails still work.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10301
Summary:
When enabled, this will show the full history of review comments in an
email-compatible threading-view.
Test Plan: Sending emails with the option on and off.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10146
Summary:
Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Test Plan:
Been running this in our deployment for a few months now.
====Well behaved clients====
- Gmail
- Mail.app
====Bad clients====
- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.
====Need testing====
- Outlook (Windows + Mac)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: webframp, taoqiping, chad, epriestley, Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D9375
Summary: Fixes T2101. When viewing an image change, show image dimensions, MIME type, and filesize.
Test Plan:
{F190189}
{F190190}
very utility
such wow
Reviewers: mailson, btrahan, chad
Reviewed By: chad
Subscribers: epriestley, Korvin, aran
Maniphest Tasks: T2101
Differential Revision: https://secure.phabricator.com/D5206
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.
This has zero impact on the UI or behavior.
Test Plan:
- Checked/unchecked some options, saved form.
- Swapped back to `master` and saw exactly the same values.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10238
Summary:
- Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
- Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
- Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.
Test Plan:
- Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
- Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
- Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5771, T4864, T5851
Differential Revision: https://secure.phabricator.com/D10221
Summary: This fix is wrong - should be load and not get - but moreover this is actually correctly set as the reply handler is instantiated inside the DifferentialRevisionMailReceiver correctly; $this->getExclude was correct. Ref T5185.
Test Plan: this shall stop the fatal in production.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10101
Summary: Ref T5185. By code inspection, I am pretty sure before this patch it was doing a set of a get on itself which does nothing. Now, being careful not to break Facebook we get the proper exclusion phids. I am pretty sure the folks in T5185 are experiencing this in Differential only.
Test Plan: Get some folks on T5185 to play with this
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10087
Summary:
This Fixes T5737. Apparently the functionality to search by different
statuses in differential was already there, but the options weren't
exposed in the frontend. I can't think of any reason why this should've
been the case, so I just added the other options.
Test Plan: Tested against some local diffs to match new query option.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5737
Differential Revision: https://secure.phabricator.com/D10076
Summary: Instead of implementing the `getCapabilityKey` method in all subclasses of `PhabricatorPolicyCapability`, provide a `final` implementation in the base class which uses reflection. See D9837 and D9985 for similar implementations.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D10039
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Provide an implementation for the `getName` method rather than automagically determining the application name.
Test Plan: Saw reasonable application names in the launcher.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10027
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: Instead of implementing the `getTypeConstant` method in all subclasses of `PhabricatorPHIDType`, provide a `final` implementation in the base class which uses reflection. See D9837 for a similar implementation.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9985
Summary: Ref T5655. The `PhabricatorDestructibleInterface` interface is misspelled as `PhabricatorDestructableInterface`. Fix the spelling mistake.
Test Plan: `grep`. Seeing as this interface is fairly recent, I don't expect that this would cause any widespread breakages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9988
Summary: Ref T4420. These are used for some stuff like "reviewer".
Test Plan:
- Edited "reviewers" in differential edit.
- Edited "reviewers" in differential search.
- Edited "reviewers" in Differential "add reviewers..." action on detail page.
- Edited a "reviewers" field in a herald rule.
- Edited "owner" in owners search.
- Edited "primary owner", "owners" on owners edit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9887
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary:
Ref T5245. See some discussion in D9838.
When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once.
To do this, allow edge types to `shouldWriteInverseTransactions()`. When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log.
Test Plan: Attached and detached revisions from tasks, saw transactions appear on both sides of the operation.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9839
Test Plan: Queried a revision that had a repository attached, got the PHID; queried one that didn't, got null.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9928
Summary: Ref T4420. Update "projects" source.
Test Plan:
- Edited projects on a Differential revision.
- Edited projects on a commit.
- Edited projects on a repository.
- Edited projects in feed search.
- Edited projects in a Herald rule field.
- Edited projects in a Herald rule action.
- Edited projects in Maniphest batch editor.
- Edited projects on Maniphest task.
- Edited projects in "Associate Projects..." action in Maniphest.
- Edited projects on Maniphest search in "all projects", "any project" and "not projects" fields.
- Edited projects on a Paste.
- Edited projects on a Pholio mock.
- Edited projects on a custom policy rule.
- Edited projects on a Ponder question.
- Edited projects on a Diffusion search query.
- Edited projects on a global search query.
- Edited projects on a slowvote.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9884
Summary:
Ref T4420.
- Allow tokenizers to accept either a `Datasource` object (new style) or a URI (old style).
- Read URI and placeholder text from object, if available.
- Swap the "repositories" datasource (which seemed like the simplest one) over to the new stuff.
- Tweak/update the repo tokens a little bit.
Test Plan:
- Used tokenizer in Herald, Differential (search), Differential (edit), Push Logs.
- Grepped for other callsites.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9874
Summary:
Fixes T3732. Ref T1205. Ref T3116.
External accounts (like emails used as identities, Facebook accounts, LDAP accounts, etc.) are stored in "ExternalAccount" objects.
Currently, we have a very restrictive `CAN_VIEW` policy for ExternalAccounts, to add an extra layer of protection to make sure users can't use them in unintended ways. For example, it would be bad if a user could link their Phabricator account to a Facebook account without proper authentication. All of the controllers which do sensitive things have checks anyway, but a restrictive CAN_VIEW provided an extra layer of protection. Se T3116 for some discussion.
However, this means that when grey/external users take actions (via email, or via applications like Legalpad) other users can't load the account handles and can't see anything about the actor (they just see "Restricted External Account" or similar).
Balancing these concerns is mostly about not making a huge mess while doing it. This seems like a reasonable approach:
- Add `CAN_EDIT` on these objects.
- Make that very restricted, but open up `CAN_VIEW`.
- Require `CAN_EDIT` any time we're going to do something authentication/identity related.
This is slightly easier to get wrong (forget CAN_EDIT) than other approaches, but pretty simple, and we always have extra checks in place anyway -- this is just a safety net.
I'm not quite sure how we should identify external accounts, so for now we're just rendering "Email User" or similar -- clearly not a bug, but not identifying. We can figure out what to render in the long term elsewhere.
Test Plan:
- Viewed external accounts.
- Linked an external account.
- Refreshed an external account.
- Edited profile picture.
- Viewed sessions panel.
- Published a bunch of stuff to Asana/JIRA.
- Legalpad signature page now shows external accounts.
{F171595}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3732, T1205, T3116
Differential Revision: https://secure.phabricator.com/D9767
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: This supplements the footer warning and makes it more visible for authors.
Test Plan: {F173277}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9794
Summary:
Ref T5495. We currently show one warning in revision headers, about not having any reviewers.
I want to add a second warning (for missing Legalpad signatures). At least one install would like to add custom warnings (see T5495) which are so specific that we can't reasonably cover them in the upstream.
Generalize these header warnings by moving them to CustomField, so I can implement the Legalpad stuff without making a mess and the install in T5495 can use an extension.
Test Plan:
Hit all three header states, they look exactly like they did before this change:
{F173265}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5495
Differential Revision: https://secure.phabricator.com/D9793
Summary: Fixes T5503. We incorrectly render an encoding note for empty files. Only render an encoding note for text changes with at least one hunk.
Test Plan:
- Viewed empty file, no note.
- Viewed nonempty file with altered encoding, saw note.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5503
Differential Revision: https://secure.phabricator.com/D9780
Summary:
Ref T3116. Add a Herald action "Require legal signatures" which requires revision authors to accept legal agreements before their revisions can be accepted.
- Herald will check which documents the author has signed, and trigger a "you have to sign X, Y, Z" for other documents.
- If the author has already signed everything, we don't spam the revision -- basically, this only triggers when signatures are missing.
- The UI will show which documents must be signed and warn that the revision can't be accepted until they're completed.
- Users aren't allowed to "Accept" the revision until documents are cleared.
Fixes T1157. The original install making the request (Hive) no longer uses Phabricator, and this satisfies our requirements.
Test Plan:
- Added a Herald rule.
- Created a revision, saw the rule trigger.
- Viewed as author and non-author, saw field UI (generic for non-author, specific for author), transaction UI, and accept-warning UI.
- Tried to accept revision.
- Signed document, saw UI update. Note that signatures don't currently //push// an update to the revision, but could eventually (like blocking tasks work).
- Accepted revision.
- Created another revision, saw rules not add the document (since it's already signed, this is the "no spam" case).
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: asherkin, epriestley
Maniphest Tasks: T1157, T3116
Differential Revision: https://secure.phabricator.com/D9771
Summary: Ref T5137. A slight modification to D9609, such that the repository is always included in Differential emails. Otherwise "Accepted", "Closed" and "Requested Changes To" emails don't include the repository.
Test Plan: Not tested.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5137
Differential Revision: https://secure.phabricator.com/D9728
Summary:
Ref T5446.
- For all callsites which do not specify a value, set `false` explicitly.
- Make `true` the default.
Test Plan: Used `grep`, then manually went through everything.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5446
Differential Revision: https://secure.phabricator.com/D9687
Summary:
add looksoon call after every attempt at landing.
This includes failed attempts, to elevate "not a fast-forward" issues, although there are probably smarter things to be done about that.
Test Plan: Land, look at logs.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9518
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.
Test Plan: (See screenshots...)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T832, T4045, T5179
Differential Revision: https://secure.phabricator.com/D9294
Summary: Ref T1049. This moves the declaration of build variables onto HarbormasterBuildableInterface, allowing new classes implementing HarbormasterBuildableInterface to declare their own variables.
Test Plan: Implemented it on another class, saw the build variables appear.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D9618
Summary: Ref T5137. Listing the repository in Differential emails makes it easy to filter.
Test Plan: Eye-ball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: young_hwi, epriestley, Korvin
Maniphest Tasks: T5137
Differential Revision: https://secure.phabricator.com/D9609
Summary: Ref T4986. Instead of requiring users to know the name of an application search engine class, let them select from a list.
Test Plan:
Created a new panel.
{F165468}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9500
Summary: Fixes T5309. Modernize this callsite to use ChangesetQuery and pick up attached objects.
Test Plan: Clicked "Download Raw Diff" in Differential.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5309
Differential Revision: https://secure.phabricator.com/D9461
Summary: This trailing whitespace is meaningful for these files. Also, exclude test data from linting.
Test Plan: Ran unit tests.
Reviewers: hach-que, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9462
Summary: Fixes T5303. Individual diffs can have public access policies.
Test Plan: Viewed a public diff while logged out.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5303
Differential Revision: https://secure.phabricator.com/D9452
Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.
Test Plan: Will run `arc unit` on another host.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9443
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: This went smoother than expeced. Makes the rounded Card the default, also tweaked selected state a little.
Test Plan:
Test UIExamples, Maniphest, Home, Differential, Harbormaster, Audit. Everything seems normal
{F163971}
{F163973}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9408
Summary:
Can't say I know what I'm doing here, but this fixes an the upgrade-scope flow for landing-to-github.
Without this change, it looks like the submit button makes the browser (Chrome and msie) make the call in the background, instead of hijacking the window.
With it, it works like it should.
Test Plan: try to land with weak token, click "Refresh..", see GitHub button.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9407
Summary: Fixes T5262. This branch is overzealous, and causes us to fail to load changeses if `metamta.differential.unified-comment-context` is off. It was on for me locally for testing, which is why I missed this.
Test Plan: No more exception.
Reviewers: chad
Reviewed By: chad
Subscribers: richardvanvelzen, epriestley
Maniphest Tasks: T5262
Differential Revision: https://secure.phabricator.com/D9376
Summary: Fix size and spacing of file icons in diffs, update with new types, consistency.
Test Plan: Tested a diff in differential
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9372
Summary: Ref T4045. Ref T5179. Send all new writes into the modern store.
Test Plan:
- Created a diff.
- Verified it went to the modern store.
- Destroyed a revision, verified hunks were destroyed.
- Also unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9293
Summary: Ref T4045. Ref T5179. When saving a modern hunk, deflate it if we have the function and deflating it will save a nontrivial number of bytes.
Test Plan:
- Used `bin/hunks migrate` to move some hunks over, saw ~70-80% compression on most standard hunks.
- Viewed changesets using compressed hunks.
- Profiled `gzinflate()` and verified the cost is trivial (<< 1ms) at least for normal diffs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9292
Summary:
Ref T4045. Ref T5179. While we'll eventually need to force a migration, we can let installs (particularly large installs) do an online migration for now. This moves hunks to the new storage format one at a time.
(Note that nothing writes to the new store yet, so this is the only way to populate it.)
WARNING: Installs, don't run this yet! It won't compress the data. Wait until it can also do compression.
Test Plan: Added a `break;` after migrating one row and moved a few rows over. Spot checked them in the database and viewed the affected diffs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9291
Summary:
Ref T4045. Ref T5179. Hunk storage has two major issues:
- It's utf8, but actual diffs are binary.
- It's huge and can't be compressed or archived.
This introduces a second datastore which solves these problems: by recording hunk encoding, supporting compression, and supporting alternate storage. There's no actual compression or storage support yet, but there's space in the table for them.
Since nothing actually uses hunk IDs, it's fine to have these tables exist at the same time and use the same IDs. We can migrate data between the tables gradually without requiring downtime or disrupting installs.
Test Plan:
- There are no writes to the new table yet.
- The only effect this has is making us issue one extra query when looking for hunks.
- Observed the query issue, but everything else continue working fine.
- Created a new diff.
- Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9290
Summary: Ref T4045. Ref T5179. This removes all non-Query hunk loads.
Test Plan:
- Viewed revisions.
- Viewed standalone changesets.
- Viewed raw old/new files.
- Viewed vs diffs.
- Enabled inline comments in mail and sent some transactions with inlines.
- Called `differential.getrawdiff`.
- Grepped for `loadHunks()`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9289
Summary: Ref T5179. Ref T4045. I want to move all hunk loads into DifferentialHunkQuery so I can make it do magical things where hunks come from multiple places, handle non-utf8 encodings properly, handle compression, archive into Files, and so on.
Test Plan: Viewed some revisions. Called `differential.getrawdiff`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045, T5179
Differential Revision: https://secure.phabricator.com/D9287
Summary:
Ref T5179. Currently, all the changeset rendering logic is in the "populate" behavior, and a lot of it comes in via configuration and is hard to get at.
Instead, surface an object which can control it, and which other behaviors can access more easily.
In particular, this allows us to add a "Load/Reload" item to the view options menu, which would previously have been very challenging.
Load/Reload isn't useful on its own, but is a step away from "Show whitespace as...", "Highlight as...", "Show tabtops as...", "View Unified", "View Side-By-Side", etc.
Test Plan:
- Viewed Differential.
- Viewed Diffusion.
- Viewed large changesets, clicked "Load".
- Used "Load" and "Reload" from view options menu.
- Loaded all changes in a large diff, verified "Load" and TOC clicks take precedence over other content loads.
- Played with content stability stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5179
Differential Revision: https://secure.phabricator.com/D9286
Summary: I think this is the direction the language has been moving? Maybe this will train me that "CCs" are called "Subscribers". (I actually don't love this wording change, but consistency is good?)
Test Plan: {F163255}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9367
Summary:
This does some backend cleanup of the tile stuff, and some general cleanup of other application things:
- Users who haven't customized preferences get a small, specific set of pinned applications: Differential, Maniphest, Diffusion, Audit, Phriction, Projects (and, for administrators, Auth, Config and People).
- Old tile size methods are replaced with `isPinnnedByDefault()`.
- Shortened some short descriptions.
- `shouldAppearInLaunchView()` replaced by less ambiguous `isLaunchable()`.
- Added a marker for third-party / extension applications.
Test Plan: Faked away my preferences and viewed the home page, saw a smaller set of default pins.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9358
Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.
Open to feedback.
Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)
{F160052}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5176
Differential Revision: https://secure.phabricator.com/D9297
Summary:
Fixes T5041. Pretty sure this is the issue: if a diff contains a large number of identical lines longer than 30 characters, we end up paying O(N^2) for each set.
Instead, when N > 16, opt to pay 0.
Test Plan: Added a test which dropped from ~100s to ~0 after changes (this diff includes a reduced-strenght version of the test, since parsing a 4,000 line diff is a little bit pricey).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5041
Differential Revision: https://secure.phabricator.com/D9178
Summary:
Updates policy, headers, typeaheads to FA over policy icons
Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late
Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names
Browsed numerous places, checked new dropdowns, see pudgy people.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4739
Differential Revision: https://secure.phabricator.com/D9179
Summary:
Fixes T4898. After we increased the strictness of the `%s` conversion, most `serialize()` output is rejected from the cache.
Drop the cache, change the column type to latin1_bin, and then use `%B` to mark the data as binary during query construction.
Test Plan: Viewed Differential, saw cache fills.
Reviewers: btrahan, spicyj
Reviewed By: spicyj
Subscribers: epriestley
Maniphest Tasks: T4898
Differential Revision: https://secure.phabricator.com/D9171
Summary: Changes to using FontAwesome
Test Plan:
Testing UIExamples and each of the pages (except releelph)
{F155942}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9157
Summary: Ref T5058. The use of "enum" is confusing; we mean "choose one of these specific string constants". Make this more clear.
Test Plan: Viewed each call from the web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5058
Differential Revision: https://secure.phabricator.com/D9127
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.
Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9088
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary: See <http://fab.wmflabs.org/T88>. While this issue is on MW's side, these links are kind of ugly and have more readable alternate forms now. Update them to use proper modern forms.
Test Plan: See inlines.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9042
Summary:
See <https://github.com/facebook/phabricator/issues/596>
Broadly, Facebook would like to bring Pull Requests from GitHub into Phabricator.
In the long term we can do this properly via Doorkeeper/Nuance, but that's probably a ways off. This seems like a reasonable low-budget compromise for now.
I'm a little hesitant to add a ton of parameters to this call, but `repositoryPHID` seems pretty reasonable, and is notable because it also controls default policies.
Test Plan:
- Created a diff with no repositoryPHID.
- Created a diff with a repositoryPHID.
- Verified it carried over when the diff was used to create a revision.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: ptarjan, jamesgpearce, epriestley
Differential Revision: https://secure.phabricator.com/D9023
Summary: Ref T4986. These are mostly mechanical now, I skipped a couple of slightly tricky ones. Still a bunch to go.
Test Plan:
For each engine:
- Viewed the application;
- created a panel to issue the query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9017
Summary:
Ref T4119. This is ugly for now, but technically works.
The comment area and transaction log don't realy know about each other, so for the moment the linking is a bit manual. Differential/Maniphest are special cases anyway.
Test Plan: {F149992}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8957
Summary:
Fixes T4941. If a diff has had trailing whitespace stripped, we will fail to handle empty lines correctly (previously, these lines had a leading space when the original tool emitted them).
(This probably stopped working around the time we began retaining newlines.)
Test Plan: The diff in T4941 now parses and renders correctly.
Reviewers: asherkin, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4941
Differential Revision: https://secure.phabricator.com/D8968
Summary:
Fixes T4917. Currently, if a user doesn't have access to, e.g., Phriction, they still get a checkbox in the search results to search for Wiki Documents. Those results will be filtered anyway, so this is confusing at best.
Instead, bind PHID types to applications. This is a relatively tailored fix; some areas for potential future work:
- Go through every PHID type and bind them all to applications. Vaguely nice to have, but doesn't get us anything for now.
- If no searchable application is installed, we don't show you an error state. This isn't currently possible ("People" is always installed) but in the interest of generality we could throw an exception or something at least.
- The elasticserach thing could probably constrain types to visible types, but we don't have a viewer there easily right now.
Test Plan: Uninstalled Phriction, saw the checkbox vanish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4917
Differential Revision: https://secure.phabricator.com/D8904
Summary: See <http://fab.wmflabs.org/T226>. The summary for this option is confusing, because "true" means sticky but the wording implies "true" means non-sticky.
Test Plan:
- Looked at the option in summary view.
- Reviewed related text, none of the other copy here seems confusing or ambiugous to me.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: qgil, epriestley
Differential Revision: https://secure.phabricator.com/D8884
Summary: Fixes T4899. Action strengths got lost somewhere along the way; actions like "Accepted" should be stronger than "Changed Subscribers".
Test Plan: Verified things sort as expected now, with major actions at the top.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4899
Differential Revision: https://secure.phabricator.com/D8857
Summary: Some actions (notably, `!accept`) require more information than we currently load.
Test Plan: Piped in some `!accept` mail using `bin/mail receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8844
Summary: Fixes T4606. Also shortens two unusual type names which are currently inconsistent.
Test Plan: Expanded advanced search.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4606
Differential Revision: https://secure.phabricator.com/D8853
Summary:
If you create a diff with no hunks (e.g., it adds a single empty file), we never attachHunks() so we throw on getHunks().
Instead, make sure changesets get hunks attached if they expect it.
Test Plan: Created a new diff with a single empty file in it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: zeeg, epriestley
Differential Revision: https://secure.phabricator.com/D8842
Summary: These are a little easier on the eyes.
Test Plan:
Reject an epriestley diff.
{F146851}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8841
Summary:
Ref T4866. I did a fancy version of this but it looks pretty bad/confusing so here's a simple version.
Fancy-but-whack version:
{F146847}
Test Plan: This version is like that, but just always uses `fa-user`.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4866
Differential Revision: https://secure.phabricator.com/D8840
Summary: center aligns the icons in the fill area, removes some of the positioning jank. Also set new icons for maniphest custom.
Test Plan: test desktop and mobile layouts, tested thin pins for proper centering.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4866
Differential Revision: https://secure.phabricator.com/D8839
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.
Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8827
Summary:
Ref T4830. A few methods, like `conduit.ping`, are callable without authentication, so this even has some use cases. Also:
- Make some Differential stuff a little more consistent.
- Use slightly more modern rendering.
- Deprecate the status-oriented `user` calls; these will be replaced by Calendar methods.
Test Plan: Browsed console as logged out / logged in users.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4830
Differential Revision: https://secure.phabricator.com/D8826
Summary: Ref T3718. Move from unbatched / ad-hoc loading to standard stuff for handles.
Test Plan: Looked at some requests and saw no changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D8810
Summary: This `%d` should be a `%s`, since the `PhutilNumber` value may get formatted according to locale settings.
Test Plan: will make @zeeg
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D8814
Summary:
Ref T1049. When Harbormaster tests pass, don't bother sending an email about it.
(I tried to implement this earlier but didn't test it entirely properly, and we needed a little more code.)
Test Plan: Used `bin/harbormaster build` to build some junk, got no email about passes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D8813
Summary: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.
Test Plan: Looked at a revision, task, paste, macro, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8812
Summary:
Ref T4810. Ultimate goal is to let Harbormaster post a "build passed/failed" transaction. To prepare for that, implement `PhabricatorApplicationTransactionInterface` in Differential.
To allow Harbormaster to take action on //diffs// but have the transactions apply to //revisions//, I added a new method so that objects can redirect transactions to some other object.
Test Plan:
- Subscribed/unsubscribed/attached/detached from Differential, saw transactions appear properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4810
Differential Revision: https://secure.phabricator.com/D8802
Summary: Ref T4809. This saves us a few round trips to find a Buildable, and generally makes the notion of "active" more explicit (i.e., not just the diff with the largest ID). In the future, we may let you revert to previous diffs, which would make the "largest number" rule not always correct.
Test Plan: Ran `differential.query`, got sensible results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8800
Summary:
Ref T3551. Releeph has old-style `loadX()` methods; get rid of one of them.
Differential has a couple of copies of this too, clean them up.
Test Plan:
- Viewed various differential revisions (with and without projects).
- Viewed and edited Releeph products.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8768
Summary:
Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.
This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.
Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8765
Summary: Ref T4045. These three methods are fairly copy-pastey. Provide a more formal DifferentialHunk API for querying various types of line ranges.
Test Plan: Used test console to verify that "added content", "removed content", and "changed content" rules still produce the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8764
Summary: Fixes T4774. With the new code and configuration instructions downplaying the role of arcanist project we weren't writing affected paths at all! I had this issue on my installation - no affected paths were written. We seem to always have the repository now though if we can see it, so not too bad of a fix.
Test Plan: updated a diff and was able to browse in diffusion.
Reviewers: epriestley, bitglue
Reviewed By: epriestley, bitglue
Subscribers: bitglue, epriestley, Korvin
Maniphest Tasks: T4774
Differential Revision: https://secure.phabricator.com/D8757
Summary: Fixes T2328. Note the audit part is fixed now.
Test Plan: Tried to reproduce the audit issue by raising my own commit as a problem; it showed up before code changes! Made a diff with my self as author and reviewer; it showed up as expected
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2328
Differential Revision: https://secure.phabricator.com/D8755
Summary: We make a silly query for every commit if you copy/paste a diff.
Test Plan: Copy/pasted diffs now render in fewer than 30 seconds.
Reviewers: btrahan, spicyj
Reviewed By: btrahan, spicyj
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8758
Summary:
Fixes T4759.
Turns out Chrome on windows doesn't really like the word joiner character. We'll switch back to zwsp but make it `position: absolute;` so it doesn't turn into a line break.
Test Plan: Looked at diffs in IE9 and Chrome Windows. Made sure copying still works as expected.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4759
Differential Revision: https://secure.phabricator.com/D8727
Summary: Ref T182. This feature rarely/never works and is on the balance enormously confusing to users (see <https://github.com/facebook/phabricator/issues/566>). If installs have somehow made it sort of work, they can comment this line out for now until we have time to make this work more reasonably.
Test Plan: Looked at a revision in Differential.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D8719
Summary:
This text is overly repetitive and is not super important. Keeps the other states. Also
- Easier to parse reviewers now
- Mobile is less janky
Test Plan:
reload my list of diffs
{F138756}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8707
Summary:
Ref T4371. Ref T4699. Fixes T3994.
Currently, we're very conservative about sending errors back to users. A concern I had about this was that mistakes could lead to email loops, massive amounts of email spam, etc. Because of this, I was pretty hesitant about replying to email with more email when I wrote this stuff.
However, this was a long time ago. We now have Message-ID deduplication, "X-Phabricator-Sent-This-Mail", generally better mail infrastructure, and rate limiting. Together, these mechanisms should reasonably prevent anything crazy (primarily, infinite email loops) from happening.
Thus:
- When we hit any processing error after receiving a mail, try to send the author a reply with details about what went wrong. These are limited to 6 per hour per address.
- Rewrite most of the errors to be more detailed and informative.
- Rewrite most of the errors in a user-facing voice ("You sent this mail..." instead of "This mail was sent..").
- Remove the redundant, less sophisticated code which does something similar in Differential.
Test Plan:
- Using `scripts/mail/mail_receiver.php`, artificially received a pile of mail.
- Hit a bunch of different errors.
- Saw reasonable error mail get sent to me.
- Saw other reasonable error mail get rate limited.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3994, T4371, T4699
Differential Revision: https://secure.phabricator.com/D8692
Summary: This ensures that two comments by the same author on the same line are sorted properly.
Test Plan: Before this patch, made two comments that appeared in the wrong order. With this patch, they sort correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8697
Summary: Make the actions appear in crumbs on mobile
Test Plan: Test action list on a mobile diff layout
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4730
Differential Revision: https://secure.phabricator.com/D8691
Summary: This should prevent long lines from making the code width different between files, which can be annoying. (And of course, it stops long lines from making a giant scrollbar too.)
Test Plan:
Loaded this diff in Chrome, Firefox, IE9, and IE8:
{F137505}
(That's a screenshot from Chrome, but it looks about the same in the other browsers.)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D8686
Summary:
This does two things
- Modernizes Table of Contents
- Makes Differential reasonable on mobile
I say resonable, as you still have to scroll horizontal to see the entire diff. This is minor as the rest of the page is 100x more useful. A 1-up view would be preferred, but this is still an improvement.
Test Plan: Used iOS simulator for browsing diffs.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8681
Summary: ...also link to commits we know about in "Local Commits" and "Revision Update History" tables. Fixes T4585.
Test Plan: made a repo. made a diff (foo) and committed it (bar). made a new diff that was comprised of two local commits. noted links to (bar) in various commit hashes as expected
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T4585
Differential Revision: https://secure.phabricator.com/D8679
Summary:
Currently, users get an error when making any changes to this field if they don't have a linked JIRA account.
Instead:
- We should only raise an error if they're trying to //add// issues, and only on the new issues. It's always fine to remove issues, and existing issues the author can't see are also fine.
- When we can't add things because there's no account (vs because there's a permissions error or they don't exist), raise a more tailored exception.
Test Plan:
- As JIRA and non-JIRA users, made various edits to this field.
- Got appropriate exceptions, including better tailoring.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Differential Revision: https://secure.phabricator.com/D8676
Summary: When trying to render "BRANCH", we need the active diff. Load it
in general since it seems reasonable for custom fields to expect it to
exist during mail rendering.
Summary: Ref T418. Fixes T4642. The "changes since last update" and "branch" fields got dropped; restore them in a general, field-driven way.
Test Plan:
- Created a revision, got relevant sections in mail.
- Commented on a revision, got relevant sections in mail.
- Updated a revision, got relevant sections in mail.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: spicyj, epriestley
Maniphest Tasks: T418, T4642
Differential Revision: https://secure.phabricator.com/D8657
Summary: Fixes T4683. This was just a missing method implementation. Also provide a couple of translation things.
Test Plan:
- Created a revision from the command line with a nonempty `JIRA Issues:` line, via `arc diff`.
- Looked at the translation strings.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4683
Differential Revision: https://secure.phabricator.com/D8656
Summary:
Fixes T3202. This fixes a couple of workflow issues:
- Accepted Revision -> Request Review. Currently this stays "accepted" due to sticky rules being too aggressive, but should transition to "needs review".
- Accepted Revision -> Plan Changes -> Request Review. Currently this stays "accepted". I think this behavior is correct, and have retained it. (In this case, you don't update the revision, you just "undo" your plan changes.) You can "Request Review" again to get back to "Needs Review".
Then implements a "sticky accept" switch:
- When off, updates downgrade accepts.
- When off, "request review" always downgrades accepts.
Test Plan:
- Went through all (I think?) of the plan changes / request review / accept / update workflows, with sticky accept on and off.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3202
Differential Revision: https://secure.phabricator.com/D8614
Summary:
Fixes T4636. Currently, we copy fields from the diff to the revision during the external effect phase, but there's no guarantee that we persist the object after this phase.
(In practice, when Herald rules trigger they cause the object to persist on this install, which is why we don't see this issue.)
Instead, move the field copies to the internal phase, where persistence is guaranteed.
Also consolidate some of the diff loading.
Test Plan: Ran `arc diff`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T4636
Differential Revision: https://secure.phabricator.com/D8610
Summary:
Fixes T4636. If a user manually deletes a "repository" setting from a revision, Herald attempts to resolve it. Instead, Herald should now just trust Differential. Generally, the new logic is:
- When diffs are created, figure out repository information.
- When revisions are updated, copy info from diffs.
- Everywhere else, just trust the revision field.
Test Plan:
- Created revisions.
- Used Herald to dry-run revisions before and after a manual edit to remove the repository setting.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4636
Differential Revision: https://secure.phabricator.com/D8576
Summary:
After "reject; plan changes; request review", revisions go back to "needs revision". Instead, they should remain in "needs review" (the reviewers need to review comments on the "request review", in the normal case). Generally, "request reivew" should act a lot like "update", just not actually change the diff.
To accomplish this, downgrade reviewers on "request review" to "rejected older", just like we would on an update.
Test Plan: Did "reject; plan; request", revision ended in "needs review". Rejected it into "needs revision"; updated it into "needs review".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: dctrwatson, epriestley
Differential Revision: https://secure.phabricator.com/D8558
Summary:
- Point them at the new Diviner.
- Make them a little less cumbersome to write.
Test Plan: Found almost all of these links in the UI and clicked them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8553
Summary:
Ref T4481. Summary is optional, but we currently always render it.
We previously rendered TEST PLAN. I wanted to see if anyone missed it. I miss it a little bit, and it sounds like @spicyj misses it. Restore it.
Test Plan:
$ ./bin/mail show-outbound --id 15232
...
BODY
epriestley created this revision.
epriestley added reviewers: The Bureaucracy, duck.
epriestley added a subscriber: duck.
TEST PLAN
more j
REVISION DETAIL
http://local.aphront.com:8080/D1042
AFFECTED FILES
number_j.txt
CHANGE DETAILS
Index: number_j.txt
===================================================================
--- number_j.txt
+++ number_j.txt
@@ -137,3 +137,4 @@
j
j
j
+j
To: epriestley, duck, Sebastiangarcia, Ahmedsmoore, nathanhthomas, chewnicorn
Cc: duck
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley, spicyj
Maniphest Tasks: T4481
Differential Revision: https://secure.phabricator.com/D8531
Summary: Herald returns a map of `phid => true`. This is unintuitive and should probably be cleaned up eventually.
Test Plan: With a "Send an email to" rule, updated a revision and saw no error in error log.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8527
Summary: Fixes T4601. The "Differential Revision" field needs to be present in the "editable" version of the message so that `--verbatim` works correctly. Some day all of this might get rewritten to be a little easier to follow, maybe, but keep things working properly for now.
Test Plan: Used `arc diff`, `arc diff --edit`, `arc diff --verbatim`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4601
Differential Revision: https://secure.phabricator.com/D8526
Summary: In the Message parser, we read this field and expect to get an array of PHIDs out of it. Currently, we get a string. Instead, get an array of PHIDs.
Test Plan: Wrote a message like "Fixes Tnnn" with "Reviewed by: duck", and saw no more parse error during message parsing.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8510
Summary: Similar to D8491, some of my new exceptions are a bit too aggressive. See IRC. This one's hitting an edit workflow with 'revisionID' onboard somehow.
Test Plan: Not entirely sure how to hit this, but it won't throw anymore.
Reviewers: btrahan, dctrwatson, zeeg
Reviewed By: zeeg
Subscribers: zeeg, aran, epriestley
Differential Revision: https://secure.phabricator.com/D8514
Summary: For some actions (like accept) we need to load reviewer authority so we can figure out if the actor can act on behalf of project reviewers, etc.
Test Plan: Will make @dctrwatson do it.
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
Subscribers: chad, aran, dctrwatson, epriestley
Differential Revision: https://secure.phabricator.com/D8505
Summary:
Two issues:
- Herald is currently overwriting accepts and rejects with "blocking reviewer". Just stop it from doing that.
- When you update an accepted revision, we put it back in "needs review", then return it to "accepted", generating an extra transaction. Instead, don't.
Test Plan:
- Updated a revision with an accepting, herald-based blocking project reviewer. Reviewer was still accepting.
- Updated an accepted revision, didn't get an extra transaction.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8506
Summary:
Ref T4585.
- Use modern UI kit.
- Make mobile-ish.
- Fix a couple minor things.
Test Plan:
{F127155}
{F127156}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: aran, epriestley, chad
Maniphest Tasks: T4585
Differential Revision: https://secure.phabricator.com/D8504
Summary:
This is a bit messy, but not tooo bad:
- In general, stop the author from being added as a reviewer.
- In the specific case that "self accept" is enabled, allow it. This is easier than trying to special case it.
- When commandeering, we make the author a reviewer and make the actor the author, but these happen after validation. At validation time, it looks like we're making the author a reviewer. Just special case this.
- Provide a slightly nicer message when trying to add yourself from `arc`. We hit the Transactions message anyway, but it's not formatted as cleanly.
- Don't try to add the author via Herald.
Test Plan:
- Edited a revision with author = reviewer, got stopped.
- Commandeered revision.
- Updated from `arc`.
- Updated in general.
- Fired a "add author as reviewer" Herald rule without things breaking.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8496
Summary: This got dropped in the ApplicationTransactions stuff.
Test Plan: Created a new revision.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8495
Summary:
Fixes two issues with Differential:
- New reviewers on initial diff were being created into a `null` state.
- The `"="` edge update was overwriting accepted/rejected statuses. This could maybe be more nuanced in the long run, but I've just made it update correctly for now.
Test Plan:
- Created and updated a revision, paying attention to reviewer statuses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8494
Summary: Hit this issue in D8485. I think reviewedByPHID changes should appear in application transactions.
Test Plan: Would like to deploy this and try updating D8485 again.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8491
Summary: The JIRA field is currently always enabled. This isn't correct; it
should be disabled if there's no JIRA provider.
We also use the old set of reviewers to compute mail delivery. Instead, reload
the correct set of reviewers before moving on after finalizing transactions.
Summary:
Ref T2222.
- Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
- Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
- Moves Releeph churn field off `DifferentialCommentQuery`.
- Removes dead code in `DifferentialRevisionViewController`.
- Removes `DifferentialException` (no references).
- Removes `DifferentialRevision->loadComments()` (no callsites).
- Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
- Removes `DifferentialCommentQuery` (all callsites updated).
Test Plan: Mostly a lot of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8476
Summary: Ref T2222. Makes the "lint/unit errors" warnings work again.
Test Plan: Viewed some revisions with and without these warnings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8475
Summary: Ref T2222. The unit and lint fields still have one piece of functionality that I need to port, but everythign else is obsolete.
Test Plan: Lots of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8474
Summary: Ref T2222. There's some magic here, just port it forward in a mostly-reasonable way. This could use some refinement eventually.
Test Plan: Pushed commits with "Fixes" and "Ref" language, used `reparse.php` to trigger the new code. Saw expected updates in Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8471
Summary:
Ref T2222. This has some minor functionality regressions:
- The plain diff page no longer shows unit/test status. I want to give diffs separate custom fields for this.
- It was technically possible to shove more data on the list view, although this doensn't affect the default config.
Test Plan: Looked at list view, diff detail view. Grepped for changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8470
Summary: Ref T2222. These no longer have an effect, and are obsoleted by `differential.fields`.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8468
Summary:
Ref T2222. Brings the major mail features (affected files, patches) forward.
This drops some of the minor integrations which just show object state (like "Maniphest Tasks") since I think they're not very important. I'll put them back if users miss them.
Test Plan: Sent mail with inline/attached patches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8459
Summary: Ref T2222. Fully modernizes these tips. No callsites remain for the old methods.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8457
Summary: Ref T2222. This has no callsites and no functionality not present in the TransactionEditor.
Test Plan: awwyiss
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8456
Summary: Ref T2222. We have two tables (one for hashes; one for paths) which were unevenly updated before. Now, update them consistently in the TransactionEditor.
Test Plan: Created a revision, saw it populate this information.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8455
Summary:
Ref T2222. DifferentialRevisionEditor has no remaining callsites, but it has a bit of functionality which still needs to be ported forward. I'm going to rip it apart piece by piece.
This removes the willWriteRevision/didWriteRevision hooks. They are completely encapsulated by transactions now, except for a unique piece of branch/task logic, which I migrated forward.
Test Plan:
- Lots of `grep`.
- Created a new revision on branch `T25`, saw it associate with the task.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8454
Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.
This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.
Test Plan: Used `arc diff --create` to create several revisions with different data.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8452
Summary: Ref T2222. Ref T3794. Medium term goal is to remove `DifferentialRevisionEditor`. This removes one of two callsites.
Test Plan: Used `arc diff --edit` to repeatedly update a revision, making changes to various fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3794, T2222
Differential Revision: https://secure.phabricator.com/D8451
Summary: Ref T2222. Ref T3886. This is a little early for general use, but the message parse/generate stuff requires CustomFields and FieldSpecifications to be closely aligned, so this provides at least a plausbile approach for any installs that run into trouble.
Test Plan: Viewed config; reordered fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222, T3886
Differential Revision: https://secure.phabricator.com/D8450
Summary: Ref T2222. Ref T3886. Converts parsing and construction of commit messages to be driven by CustomField.
Test Plan:
This is a huge, messy change. I've made an effort to test it exhasutively, but suspect I probably missed a few behaviors. Roughly:
- Enumerted all current fields (fields implementing `shouldAppearOnCommitMessage()`) and tried to test them one by one.
- Used `arc diff --edit` repeatedly to manipulate each field (this workflow hits both the parse and construct steps).
- Used `arc amend --show` to examine construct output (this does not activate the "edit" mode).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8449
Summary:
Ref T2222. Currently, Differential has a fairly hairy piece of logic to parse object lists, like `Reviewers: alincoln, htaft`. Extract, generalize, and cover this.
- Some of the logic can be simplified with modern ObjectQuery stuff.
- Make `@username` the formal monogram for users.
- Make `list@domain.com` the formal monogram for mailing lists.
- Add test coverage.
Test Plan:
- Ran unit tests.
- Called `differential.parsecommitmessage` with a bunch of real-world inputs and got sensible results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8445
Summary: Ref T2222. We have a hunk of logic that purely does text parsing here; separate it and get coverage on it.
Test Plan:
- Ran new unit tests.
- Used `differential.parsecommitmessage`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8444
Summary: Ref T2222. These no longer have any callsites. Also got rid of a little bit of other code which also no longer has callsites.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8443
Summary: Ref T2222. Update this callsite; pretty straightforward.
Test Plan: Used Conduit to take actions and saw their effects in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8442
Summary: Ref T2222. When we discover a commit associated with a revision, close it using modern transactions.
Test Plan: {F123848}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8441
Summary: Ref T2222. Moves this instance of CommentEditor to TransactionEditor.
Test Plan: Used `bin/mail receive-test` to test receiving comment mail and action mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8427
Summary: Ref T2222. For now, I'm just dropping this rather than updating it since I'll need to come back here later for `DifferentialRevisionEditor` anyway, and no users rely on this functionality.
Test Plan: Static checks; this isn't user-facing.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8426
Summary: Ref T2222. Straightforward update to new stuff.
Test Plan:
- Tried to close an uncloseable revision.
- Closed a closeable revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8425
Summary: Ref T2222. Primary goal is to remove this callsite for `DifferentialCommentEditor`, but rather than updating it I'm just nuking this method since it's been deprecated for more than a year (more than two years?)
Test Plan: Reloaded Conduit method list.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8424
Summary:
Ref T2222. Ref T4484. See D8404 for discussion.
When a revision is updated with the new Editor, apply Herald rules. Additionally, apply them in a modern way which generates transactions.
Test Plan: {F122299}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T2222, T4484
Differential Revision: https://secure.phabricator.com/D8405
Summary: Ref T2222. Ref T4481. This fixes the issue where "Plan Changes" could immediately trigger a state change (e.g., back to accepted) because of state-based transitions out of the NEEDS_REVISION state.
Test Plan: Planned changes an "accepted" revision, it didn't immediately return to being accepted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222, T4481
Differential Revision: https://secure.phabricator.com/D8403
Summary:
Ref T2222. Ref T4481. Specifically:
- When a revision is updated, change all "Reject" reviewers to "Reject Prior".
- Change status to "Needs Review".
- Update the state logic to account for this properly.
Test Plan:
- Created a revision as user A, with B as a reviewer.
- Rejected as B.
- Updated the revision as A.
- Saw revision in "needs review" state, with B as a "Rejected Prior" reviewer.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4481, T2222
Differential Revision: https://secure.phabricator.com/D8402
Summary:
Fixes T4550 by changing supportsFeed to shouldPublishFeedStory, so things can be more granular like that are with mail. Attempts to fix things generally too, filtering out xactions that have no business in feed, etc.
Also return an updated Task HTML representation on drag and drop moves, etc. This is important so if the priority changes you can see it reflected in the UI.
Test Plan: dragged tasks around. observed no feed stories on subpriority drags. observed feed stories and updated color bars on stories that changed priority
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4550
Differential Revision: https://secure.phabricator.com/D8399
Summary:
Ref T2222.
- Restore mail tags for ApplicationTransactions mail.
- Restore subject line verbs.
- Denormalize line count and repository PHID.
- Fix an issue with the mailgun adapter where headers weren't attached properly.
Test Plan: Sent some mail, verified it had correct subjects and tags.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8378
Summary: Ref T2222. Remove the old controller and swap in the new ApplicationTransactions one.
Test Plan: Made a pile of edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8377
Summary:
Ref T2222. Make the "EditPro" controller accommodate diff updates, and support the transaction type. This one is pretty straightforward.
Also make `revisionPHID` in the comments table nullable to fix the "Edit" action.
Test Plan:
- Created new revision.
- Updated revision.
- Tried to do some invalid stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8376
Summary:
Ref T2222. Differential has certain "words of power" (like `Ref T123` or `Depends on D345`) which should expand into a separate transaction when they appear anywhere in text.
Currently, they're respected in only some fields. I'm expanding them to work in any remarkup field, including comments and inline comments.
This partially generalizes transaction expansion/extraction in comments. Eventually, I'll probably implement some very soft sort of reference edge for T4036, maybe.
Test Plan: {F119368}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8369
Summary: Better aligns the text area when leaving an inline comment. Also, phts
Test Plan: reload page, view new padding.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8370
Summary: Ref T2222. This probably doesn't get everything, but should improve many of the newer transactions.
Test Plan: Looked at feed after making some edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8368
Summary: Ref T2222. This should help new mail thread properly with old mail.
Test Plan: Will push.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8367
Summary: Ref T2222. This updates the new JIRA field to be editable.
Test Plan: Used `/editpro/` to edit associated JIRA issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8364
Summary: Ref T2222. This will probabaly have a few rough edges too, but seems to work well.
Test Plan:
- Made a bunch of comments while building this.
- Made some new comments.
- Verified that the Asana/JIRA integration is only a little bit janky, not completely broken.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8362
Summary: Ref T2222. This will probably have some rough edges for a bit (e.g., weird cases I didn't remember or think of), but there's no change to the underlying data and we can easily revert if things get too messy.
Test Plan: Looked at a variety of revisions and saw sensible output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8361
Summary:
Ref T2222. These are pretty straightforward.
For these fields and a few others, the existing code shows the value for the "current/manual" diff (i.e., the diff selected in the diff selection table), not the "active" diff (i.e., the most recent diff attached to the revision). I'm going to drop that for now (always showing the most recent diff instead) and then reevaluate it once we're switched over. In 95% of cases these are the same, anyway.
Test Plan: Looked at fields; this diff changes nothing on its own.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8359
Summary:
Ref T2222. Ref T3886. Gets the storage-based fields working.
This requires future changes to actually do anything, all this code is inactive.
Test Plan: {F118863}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8357
Summary: Ref T2222. Ref T3886. Differential has a legacy storage table for auxiliary fields; move the data to modern storage.
Test Plan:
- Ran migration.
- Verified fields still worked properly afterward (view, edit, etc).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T2222
Differential Revision: https://secure.phabricator.com/D8355
Summary:
Ref T2222. This isn't complete and doesn't change runtime behavior yet (the new fields are not glued to the interface), but implements many fields.
(The remaining fields have something weird going on with them, for the most part.)
Test Plan:
With additional changes, rendered most fields sensibly:
{F118834}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8354
Summary:
The intent of getOrBuildEngine() is to save some boilerplate in cases where we're just using a standard engine, but it didn't get cached so we'd rebuilt it over and over again.
This was especially bad in Differential with a large number of inlines.
Test Plan: "Query" tab of services is no longer quite so ridiculous in Differential.
Reviewers: btrahan, dctrwatson
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8352
Summary:
Currently, the linter raises `XHP29` warnings for these files because they are not abstract or final.
I guess there are two possibly solutions, either making the classes final or marking them as `@concrete-extensible`. Given that there are no subclasses of these classes in the `phabricator`, `arcanist` and `libphutil` repositories... I opted to declare the classes as final.
Test Plan:
The following linter warnings are gone:
```
>>> Lint for src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
3 /**
4 * @group aphront
5 */
>>> 6 class AphrontDefaultApplicationConfiguration
7 extends AphrontApplicationConfiguration {
8
9 public function __construct() {
>>> Lint for src/applications/differential/mail/DifferentialReplyHandler.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
1 <?php
2
>>> 3 class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
4
5 private $receivedMail;
6
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8347
Summary:
Ref T2222. Ref T3886. Ref T418. A few changes:
- CustomField can now index into global search.
- Use CustomField fields instead of older custom fields for Differential global search. (This slightly breaks any custom fields which exist, but they are presumably very rare, and probably do not exist; this break is also very mild.)
- Automatically perform CustomField and Subscribable indexing on applicable object types.
Test Plan: Used `bin/search index` to reindex a bunch of stuff, then searched for it. Debug-dumped abstract documents to inspect them.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886, T2222
Differential Revision: https://secure.phabricator.com/D8346
Summary: Ref T2222. This enriches mail a little bit to get these rendering pretty much like they do now.
Test Plan: {F118255}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8343
Summary:
Ref T2222. This requires one new trick:
- When merging edge transactions which both add/update an edge, the Editor gets to control how the edge data is merged.
Specifically, we pick the "strongest" state to keep, so "accept + comment" leaves you with an accept instead of a comment.
Test Plan: Accepted, commented on, and comment + accepted revisions. Added some debugging dumps to verify that the merging was getting hit and working correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8340
Summary:
Ref T2222. This doesn't feel super clean, but doesn't feel too bad either.
Basically, Differential transactions can have secondary state-based effects (changing the overall revision status) when reviewers resign, are removed, accept, or reject revisions.
To deal with this in ApplicationTransactions, I did this:
- `applyFinalEffects()` can now alter the transaction set (notably, add new ones). This mostly matters for email, notifications and feed.
- In Differential, check for an overall revision state transition in `applyFinalEffects()` (e.g., your reject moving the revision to a rejected state).
- I'm only writing the transaction if the transition is implied and indirect.
- For example, if you "Plan Changes", that action changes the state on its own so there's no implicit state change transaction added.
The transactions themselves are kind of fluff, but it seems useful to keep a record of when state changes occurred in the transaction log. If people complain we can hide/remove them.
Test Plan: {F118143}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8339
Summary: Ref T2222. This mostly makes Accept/Reject work. The big missing piece is that overall revision status does not yet update properly. I need to think about how I want that to work a little bit more.
Test Plan: Accepted and rejected some stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8333
Summary: Ref T2222. This is obsolete and no longer used. We could deduce it from transactions or commits in modern Phabricator if we wanted it. We may implement a more general mechanism for T4434.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8330
Summary:
Ref T2222. This introduces two small new concepts:
- `expandTransactions()`: allows a transaction to expand into several transactions. For example, "resign" adds a "remove reviewers" transaction.
- We have some other cases which could use this, but currently hard-code things outside of the `Editor`.
- One example is that in Maniphest, closing a task implies claiming it if it is unowned.
- `setIgnoreOnNoEffect()`: The whole Editor can be set to continue or stop if any transactions have no effect, but this allows the behavior to be refined at the individual transaction level. This is primarily to make the UX less confusing, so the user gets only a single relevant error instead of one for each expanded transaction.
Otherwise, this is pretty straightforward.
Test Plan:
Rigged comment form to use SavePro controller, enabled resign action, then tried to resign from a bunch of stuff.
{F117743}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8328
Summary: Ref T2222. Implements the simpler actions (abandon, reclaim, close, reopen, plan changes, request review) in a transactional way with validation and effect checks.
Test Plan:
- Rigged submissions to point at the Pro controller.
- Rigged dropdown to have all these options all the time.
- Tried to apply about 20-30 of these operations to various revisions and always got the expected result (success, error, or no-op).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8307
Summary: Ref T2222. Makes the "pro" controller work with inlines.
Test Plan: Added a bunch of inlines and saved them with the "pro" controller.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8306
Summary: Ref T2222. Adds a mostly-functional "Pro" comment controller. This does the core stuff, but does not yet do actions (accept, reject, etc.) or inline comments.
Test Plan: Changed the `if (false)` to an `if (true)`, then made some comments, etc. This is normally unreachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8304
Summary: Ref T2222. Adds basic support for email.
Test Plan: Received an email via `/editpro/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8303
Summary: Ref T2222. Currently this is a giant header box thing. Move it into the ObjectBox.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8301
Summary:
Ref T3886. Broadly, fields break down into two types right now: fields which store data on the object (like `DifferentialTitleField`) and fields which store data in custom field storage.
The former type generally reads data from the object into local storage prior to editing, then writes it back afterward. Currently, this happens in `didSetObject()`.
However, now that we load and set objects from ApplicationTransactionQuery, we'll do this extra read-field-values on view interfaces too. There, it's unnecessary and sometimes throws data-attached exceptions.
Instead, separate these concepts, and do all the read-from-object / read-from-storage in one logical chunk, separate from `didSetObject()`.
Test Plan:
- Edited Differential revision.
- Edited Maniphest task.
- Edited Project.
- Edited user profile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8299
Summary: Report from @zeeg, I think this is the root issue. Currently, if a project is CC'd we'll write "CC: projectname", but should write "CC: #projectname".
Test Plan: Verified that we now write "CC: #projectname".
Reviewers: btrahan, zeeg
Reviewed By: zeeg
CC: zeeg, aran
Differential Revision: https://secure.phabricator.com/D8296
Summary:
Ref T2222. Differential has custom code for managing subscriptions, but no longer requires it.
The one trick is that we don't have a hook for loading related data on the subscriptions workflow right now. Just glue that in for the moment; it's relatively harmless, and once Diffusion converts we'll have more context on how to best surface it properly.
Test Plan: Subscribed and unsubscribed from a revision. Viewed different revisions and saw correct subscription state.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8293
Summary: Ref T3886. Now that a custom field can emit a core transaction, just emit a subscribers transaction.
Test Plan: {F116014}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8289
Summary:
Ref T3886. Ref T418.
- Adds "View Policy" and "Edit Policy" fields.
- Allows CustomFields to produce arbitrary types of transactions, so these fields can produce standard view/edit policy transactions and get all the strings and validation associated with them.
Test Plan: {F116001}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T3886
Differential Revision: https://secure.phabricator.com/D8287
Summary: Ref T3886. Moves some of the "required" logic to the base class ("DifferentialCoreField") so Title and Test Plan can share it.
Test Plan: Edited revisions using "pro" editor, saw test plan transactions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8285
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.
Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.
Test Plan:
{F115918}
- Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T418
Differential Revision: https://secure.phabricator.com/D8284
Summary:
Ref T3886.
- Adds "Summary" field.
- Adds "CoreField" for fields stored on the actual object, to reduce code duplication a bit for the main fields.
Test Plan: {F115902}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8283
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8294
Summary:
Ref T3496. Currently, we call loadAssets() on each revision table, which invokes a new revision query and a pile of subqueries.
Instead, add `needFlags()` and `needDrafts()` to `RevisionQuery`. Some day these could perhaps be more generic.
Test Plan:
- Viewed home, differential, etc., no longer saw 9203809238 queries being run for no reason.
- Drafts and flags still appear properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8277
Summary:
Ref T3886. I spent a few hours trying to make `DifferentialFieldSpecification` extend `PhabricatorCustomField` so I could be more blunt in my approach here and just swap the whole thing over in one go (more or less like I did with Maniphest) but we have a ton of custom fields and things felt really shaky and the change was enormous and hard to keep track of.
Instead, I'm going to do this more gradually and go field-by-field. This implements a CustomField version of the "Title" field.
(There are no links to this in the UI.)
Test Plan:
{F115353}
{F115354}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8276
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.
Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8275
Summary:
Ref T1812. This cleans up most of the easy hard-coded references to specific statuses:
- The "fixes" language moves into ManiphestTaskStatus.
- Add a method to list open statuses.
- Add a method to test if a status is open.
- Add a method to get default status for new tasks.
Test Plan: Browsed around, lint, grep, created, filtered and updated tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8264
Summary: Fixes T3872. Ref T1812. Ref T3886. Modernize the "closes x as y" string parser, and use all the new parsers instead of the old ones.
Test Plan: Made a commit full of a pile of these trigger strings, then used `scripts/repository/reparse.php --message` to reparse it. Verified that parses came back as expected using a bunch of `var_dump()`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1812, T3872, T3886
Differential Revision: https://secure.phabricator.com/D8263
Summary:
Ref T3886. See D8261. This brings the "reverts x" phrase to modern infrastructure. It isn't actually called by the real parser yet, I'm going to do that in one go at the end so I can test everything more easily.
This had unit tests; port most of them forward.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8262
Summary:
Ref T3886. Ref T3872. Ref T1812. We have several parsers which look for textual references to other objects, like:
Closes Tx.
Depends on Dy.
Reverts Dz.
Currently, these are pretty hard coded, don't get all the edge cases right, and don't generalize well. They're also implemented in the middle of Differential's field code. So I want to:
- Share more code so that, e.g., "Tx, Ty" always works (only some rules support it right now);
- fix bugs in the parser, like T3872;
- make this a modular, extensible process which runs against custom fields, not a builtin part of fields;
- make the internals more flexible to accommodate custom stuff like T1812.
This implements the "Verbs optional-noun Object, Optional Other Objects optional-as-something." grammar in a general way so subclasses can just plug in their keywords. Runtime code doesn't touch this yet.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3872, T1812, T3886
Differential Revision: https://secure.phabricator.com/D8261
Summary:
Ref T2222. I want to stage a "later" patch to drop this column, but get rid of the last few references to it.
One of these methods has no callers, and the other stuff I've updated to use the modern fields.
Test Plan: Created some inlines, hit "edit", submitted them, `grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8240
Summary: Ref T2222. Use new code for rendering. Delete `DifferentialRevisionCommentView`, which has no remaining callsites.
Test Plan: Went through all the different actions and verified the previews rendered correctly. Reloaded page to test draft behavior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8236
Summary:
Ref T4420. Tokenizers currently operate in "preload" or "ondemand" modes. In the former mode, which is default, they'll try to load the entire result list when a page loads.
The theory here was that this would slightly improve the experience for small installs, and once they got big enough they could switch to "ondemand". In practice, several issues have arisen:
- We generally don't have a good mechanism for telling installs that they should tweak perf config -- `metamta.send-immediately` is the canonical example here. Some large installs are probably affected negatively by not knowing to change this setting, and having settings like this is generally annoying.
- We have way way too much config now.
- With the advent of ApplicationSearch, pages like Maniphest make many redundant loads to prefill sources like projects. Most of the time, this data is not used. It's far simpler to switch everything to ondemand than try to deal with this, and dealing with this would mean creating two very complex divergent pathways in the codebase for a mostly theoretical performance benefit which only impacts tiny installs.
- We've been using `tokenizer.ondemand` forever on `secure.phabricator.com` since we have many thousands of user accounts, and it doesn't seem sluggish and works properly.
Removing this config is an easy fix which makes the codebase simpler.
I've retained the ability to use preloaded sources, since they may make sense in some cases (in at least one case -- task priorities -- adding a static source pathway might make sense), and they're part of Javelin itself. However, the code will no longer ever go down that pathway.
Test Plan: Used `secure.phabricator.com` for years with this setting enabled.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D8232
Summary: I assume this box is always after timeline
Test Plan: test a diff or two
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8230
Summary: Ref T2222. Restore this funky is-visible / inline-is-elsewhere logic.
Test Plan: Updated a revision, saw all the inlines render properly when looking at various diffs and versus-diffs. Clicked inline links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8224
Summary: Ref T2222. Once these are live, yell if any of them seem off. I tried to mostly stay consistent-ish with what we had before.
Test Plan: Looked at a bunch of revisions and saw more detailed, colorful transactions.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8223
Summary: Ref T2222. These don't work yet. We just have to copy a couple fields, but let's sort that out later since this is purely a new feature.
Test Plan: Looked at a revision, no edit links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8222
Summary: Ref T2222. This is a `tmp.differential`-only issue. Inline comment transactions now have content, so we treat them like body text. We also render them separately as inline text. This produces mail where inlines are rendered twice.
Test Plan: Sent myself mail, saw only one copy of inlines.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8216
Summary:
Ref T2222. This gets rid of Differential's custom view and uses a standard view instead.
This also mostly fixes the rendering logic for inlines.
This is headed to the `tmp.differential` branch.
Test Plan: {F112696}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8215
Summary: Ref T2222. On the `tmp.differential` branch, we're currently having
issues parsing commits which reference Differential revisions, because the
"user closed this revision (closed by commit xyz)" message is fataling:
[2014-02-13 14:12:36] EXCEPTION: (PhutilProxyException) Error while
executing task ID 345358 from queue. {>} (AphrontQueryException)
#1048: Column 'contentSource' cannot be null
Specifically, the MessageParser pathway for CommentEditor doesn't set a content
source. Make sure CommentEditor always sets a content source.
(This is also causing a buildup of diffs on D8212 and D8211.)
Auditors: btrahan
Summary:
Ref T2222. This is the big one.
This migrates each `DifferentialComment` to one or more ApplicationTransactions (action, cc, reviewers, update, comment, inlines), and makes `DifferentialComment` a double-reader for ApplicationTransactions.
The migration is pretty straightforward:
- If a comment took an action not otherwise covered, it gets an "action" transaction. This is something like "epriestley abandoned this revision.".
- If a comment updated the diff, it gets an "updated diff" transaction. Very old transactions of this type may not have a diff ID (probably only at Facebook).
- If a comment added or removed reviewers, it gets a "changed reviewers" transaction.
- If a comment added CCs, it gets a "subscribers" transaction.
- If a comment added comment text, it gets a "comment" transaction.
- For each inline attached to a comment, we generate an "inline" transaction.
Most comments generate a small number of transactions, but a few generate a significant number.
At HEAD, the code is basically already doing this, so comments in the last day or two already obey these rules, roughly, and will all generate only one transaction (except inlines).
Because we've already preallocated PHIDs in the comment text table, we only need to write to the transaction table.
NOTE: This significantly degrades Differential, making inline comments pretty much useless (they each get their own transaction, and don't show line numbers or files). The data is all fine, but the UI is garbage now. This needs to be fixed before we can deploy this to users, but it's easily separable since it's all just display code.
Specifically, they look like this:
{F112270}
Test Plan:
I've migrated locally and put things through their paces, but it's hard to catch sketchy stuff locally because most of my test data is nonsense and bad migrations wouldn't necessarily look out of place.
IMPORTANT: I'm planning to push this to a branch and then shift production over to the branch, and run it for a day or two before bringing it to master.
I generally feel good about this change: it's not that big since we were able to separate a lot of pieces out of it, and it's pretty straightforward. That said, it's still one of the most scary/dangerous changes we've ever made.
Reviewers: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8210
Summary: Ref T2222. A few rendering interfaces rely on fishing the revision ID out of a DifferentialComment, but it will only have the PHID soon. Pass in the revision and use it to determine the ID instead.
Test Plan: Browsed, previewed, examined comments. Clicked anchors.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8209
Summary:
Ref T2222. I wiped out the Differential-specific stats page a long time ago, but missed this. It turned up recently in `grep`.
Facts will eventually fill this role; this code is unreachable; it probably doesn't work now and definitely won't work in a day or two after ApplicationTransactions.
Test Plan: Used `grep` to look for callsites.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8208
Summary: Ref T2222. We need this `clone` when constructing the new multi-comments in Differential, or we get double-comments internally. This shows up as emails with double comment text.
Test Plan: Sent some "Accept + comment" emails, only one comment in the body.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8206
Summary:
Ref T2222. Ref T4415. We're still writing Differential subscription stuff into this weird legacy `differential_relationship` table, which is like an edge table but extremely ancient.
Move it into a proper table.
I've removed `withSubscriptions()` from `DifferentialRevisionQuery`. It was weird, doesn't work consistently with other similar filters, and was only used by the API. Now it means "ccs", which is consistent with the ApplicationSearch UI and with Maniphest.
Test Plan:
Without migrating, added and removed subscribers via various workflows. Queried for subscribers. Everything worked as expected.
Ran the migration, verified data survived.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222, T4415
Differential Revision: https://secure.phabricator.com/D8202
Summary:
See D8200. Ref T2222. Instead of writing one comment which can have a ton of different effects, write a series of one-effect comments. These will be easier to convert into ApplicationTransactions.
This has a minor user-facing effect of making these multiple-action comments render separately:
{F111919}
Once the migration completes, they should automatically merge together nicely again.
Test Plan: Made a bunch of comments and took a bunch of actions, all of which worked normally except that they rendered as several things instead of just one.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, FacebookPOC
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8201
Summary:
Ref T2222. Instead of writing one comment which performs both a diff update and adds a comment, write two comments, one for each action. These will translate directly into ApplicationTransactions writes.
This has a small impact on the UX: these updates now render in two rows, instead of one. After T2222, they'll automerge back together.
{F111909}
Test Plan: Updated a revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8200
Summary:
Ref T2222. Currently, one `DifferentialComment` can do a lot of things (add ccs, reviewers, comments, inline comments, and perform state changes). In the future, each `ApplicationTransaction` does only one thing. This is the primary piece of complexity which makes the upcoming migration risky, because each comment needs to migrate into multiple transactions.
I want to mitigate this complexity as much as possible before the migration itself happens. One approach I'm going to use to do that is to start writing one comment per effect now, so the mapping is more direct when the migration itself happens and the write code can be straightforward (one row per save()) after the migration.
This tackles a small piece of that, which is the mail Differential sends. Currently, Differential mail acts on a single comment. Instead, allow it to act on a list of comments, but always give it one comment for now. In the future, we can hand it several comments instead and still get the expected behavior.
This change should have no impact on any application behaviors.
Test Plan:
- Commented;
- commented with inline;
- added reviewers;
- added CCs;
- added CCs via mentions;
- updated revision;
- looked at all the mail, all of which seemed sane.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8199
Summary:
Ref T2222. Currently, `DifferentialComment` stores both (a) the text of comments and (b) various other transaction details. This data needs to map to both Transactions and TransactionComments in the long run. This diff separates out all the data which is bound for the TransactionComment table, so that when we migrate `DifferentialComment` itself it will //only// need to migrate into the Transactions table. This is a much simpler migration than the inline comment one was, partly because it set up infrastructure and partly because the data is less complex.
Basically, I'm just proxying the read/write for the comment text into the other table. All readers already go through the Query class, and there are only three writers (preview, comment, implicit comment on diff update) which are all highly regular and straightforward to test.
We can also back out of this diff very easily: doing double writes cost only one line of code (`$this->content = $content;`) so we have proper double writes and a trivial revert path.
Test Plan:
- Without migrating, added comments and saw them show up.
- Migrated.
- Saw all the old comments, and no damage to the new ones.
- Added new comments.
- Used comment preview.
- Updated a revision to implicitly create an update comment and verified it looked OK.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8196
Summary: I don't think this is too terrible, and makes the future easier? Maybe?
Test Plan: ALLCAPS translation, Viewed a diff, feed, and notifications.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8175
Summary: ...and surface it in all adapters except commit adapters. Values are true or false. Ref T4294
Test Plan: made a herald rule to be cc'd on new tasks. was cc'd on new tasks and not cc'd on updates to existing tasks.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8142
Summary: Ref T4365. Two diffs from now, I'm changing the UI a bit to let you search for closed and unowned documents more explcitly. To support this in ElasticSearch and more easily in MySQL search, make these explicit, positive relationships.
Test Plan: `bin/search index --all`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4365
Differential Revision: https://secure.phabricator.com/D8122
Summary:
Fixes T4066.
add `isActionDisabled()` to DifferentialLandingStrategy, which also explains why it is so.
Make an appropriate pop-up in the controller.
Also make the whole UI "workflow", and convert `createMenuItems()` to `createMenuItem()` (Singular).
Test Plan: Click "Land to..." button in all kinds of revisions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4066
Differential Revision: https://secure.phabricator.com/D8105
Summary: Policies are now fully supported in Differential.
Test Plan: Grepped for other caveats, looks like I've already removed htem all.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D8095
Summary:
Moves away from ArcanistProjects:
- Adds storage for diffs to be directly associated with a repository (instead of indirectly, through arcanist projects). Not really populated yet.
- Drops `parentRevisionID`, which is obsoleted by the "Depends On" edge. This is not exposed in the UI anywhere and doesn't do anything. Resolves TODO.
Test Plan: Ran storage upgrades, browsed around, lots of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8072
Summary: Add the arc project and branch fields in emails for revisions under review. I am not quite sure why we only show them for changes which is already accepted or needs revision. It would be nice to have them for changes under review too.
Test Plan: Create a new revision and check email
Reviewers: epriestley, lifeihuang, JoelB, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8035
Summary: This modularizes the rest of the GC submethods. Turned out there was nothing tricky.
Test Plan: Ran `bin/phd debug garbage` and got reasonable looking behavior and output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7971
Summary: Fixes T4317. Update the "inline comment" control to a RemarkupControl. This could maybe use some padding/spacing/design touches eventually but seems OK for the moment.
Test Plan: {F101825}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4317
Differential Revision: https://secure.phabricator.com/D7969
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.
Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7966
Summary: Cleans up some older layouts to new stuffs.
Test Plan: Test with and without a diff ID.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7949
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.
Test Plan: Test out the forms, see errors without the text.
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran, hach-que
Differential Revision: https://secure.phabricator.com/D7924
Summary:
Via Asana. The tags on Differential mail are wrong in two cases:
- Transactions which submit inline comments but no comment text are not labeled as "comments", but should be.
- Non-close, non-comment transactions are not labeled at all, but should be labeled "other".
Test Plan: Submitted a no-comments, inlines-only transaction and got a message with proper `X-Phabricator-Mail-Tags` header.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7912
Summary: Two basic changes here, first we fixed up the Diffusion headers to roll out more PHUIObjectBoxes. Second we added some specific styles for when Errors are inside an ObjectBox at the first position.
Test Plan: Tested a number of different layouts for browsing respositories as well as wherever I could find cases with PHUIObjectBox Form Errors (see images attached). Still some minor tightening due after this diff, but didnt want to overload it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7914
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.
Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.
Test Plan:
- Reloaded pages.
- Browsed around Differential.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7876
Summary:
If the repo isn't bare, than we need copy it's remote instead of using it.
This will probably not work if an SSH key is provided to phabricator, and in any case you must delete
all workspaces that were already created.
This will make landing those repos slower; I plan to just delete and re-clone all repos on my instance.
It will probably be simpler to just make a bare-repo a requirement of all the git-landing work.
Test Plan: landed from hosted and un-hosted repos, checked git-remote url in each newly cloned workspace.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7592
Summary:
Fixes T4270. When you download raw file content, diffs, and patches we currently give them default (all users) visibility.
Instead, bind them to the repository or revision in question.
(This code could use a bit of cleanup at some point.)
Test Plan: Hit the patch and content download links in Diffusion and the patch download link in Differential, got restricted files with accurate policy bindings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4270
Differential Revision: https://secure.phabricator.com/D7849
Summary:
Ref T1049. Adds `bin/harbormaster` and `bin/harbormaster build` for applying plans from the console. Since this gets `--trace`, it's much easier to debug what's going on.
This doesn't work properly with some of the Drydock steps yet, I need to look at those. I think `setRunAllTasksInProcess` probably obsoletes some of the mechanisms. It might also not work with "Wait for Builds" but I didn't check.
Test Plan: Used `bin/harbormaster` to run a bunch of builds. Ran builds from web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7825
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.
Test Plan:
- This was mostly automated, then I cleaned up a few unusual sites manually.
- Bunch of grep / randomly clicking around.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7787
Summary:
These just got copy/pasted like crazy, the base class has the correct default implementation.
(I'm adding "H" for Herald Rules, which is why I was in this code.)
I also documented the existing prefixes at [[ Object Name Prefixes ]].
Test Plan: Verified base implementation. Typed some object names into the jump nav.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7785
Summary:
Ref T1715. When the user clicks "Download Raw Diff" in Differential, we try to build a diff of exactly what they're seeing. However:
- This doesn't work if any of the changes have multiple hunks, and fixing it seems hard.
- I suspect this diff is never actually useful anyway? And probably kind of confusing in the best case. You can't really apply it to anyhting, since you'd have to apply another diff first.
Instead, just build the right-side diff, which should align well with user expectation and doesn't suffer from the multi-hunk bug.
Some day, we could maybe add some of the fancy options in T1715.
See: <https://github.com/facebook/phabricator/issues/461>
Test Plan: Downloaded a multi-hunk diff, got the original back and applied it cleanly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1715
Differential Revision: https://secure.phabricator.com/D7694
Summary: See D7653. This is exclusively for Asana, who uses Differential for a post-commit, Audit-like workflow but has a small set of requirements for it to be a good fit (just this) and a large set of requirements for Diffusion/Audit to be a good fit.
Test Plan: Set the flag, verified "Accepted" revisions are no longer on the dashboard.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7654
Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.
The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.
To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.
Also delete the very old COMMITTED status, which has been obsolete for over a year.
Test Plan: Browsed around most/all of the affected interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7653
Summary: Fixes T3687. Instead of rendering "JIRA Issues" in Differential using plain links, render them using Doorkeeper tags so they get the nice "enhance with object name" effect.
Test Plan: {F84886}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D7648
Summary:
We've been having trouble with viewing diffs timing out when there's a lot of unit test failures. It was caused by formatting userdata for every single failure. The expensive part of this was actually creating the engine for every result, so moved the construction outside of the loop.
Diffs that timed out (2 min) loading before load in around 6 seconds now.
Test Plan: Loaded diffs that used to time out. Verified that details still looked right when Show Full Unit Test Results Is Clicked.
Reviewers: epriestley, keegancsmith, lifeihuang, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, andrewjcg
Differential Revision: https://secure.phabricator.com/D7581
Summary:
Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.
Fix this in two ways:
# Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
# In this case, load the revision for an omnipotent viewer.
This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.
Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4109
Differential Revision: https://secure.phabricator.com/D7603
Summary:
A usable, Land to GitHub flow.
Still to do:
- Refactor all git/hg stratagies to a sane structure.
- Make the dialogs Workflow + explain why it's disabled.
- Show button and request Link Account if GH is enabled, but user is not linked.
- After refreshing token, user ends up in the settings stage.
Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.
Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.
Test Plan:
Went through these flows:
- load revision in hosted, github-backed, non-github backed repos to see button as needed.
- hit land with weak token - sent to refresh it with the extra scope.
- Land to repo I'm not allowed - got proper error message.
- Successfully landed; Failed to apply patch.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7555
Summary:
Fixes T3034. This is obsoleted by modern policies.
This was written by a Facebook intern and is rarely used -- the Hive install might be the only use in the wild. It has never really worked correctly.
Test Plan: `grep`; browsed Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3034
Differential Revision: https://secure.phabricator.com/D7568
Summary: Fixes T3535. Also, flip flop on that spacing thing and make the spaces purdy
Test Plan: got an arcanist projected phid in the json dict
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3535
Differential Revision: https://secure.phabricator.com/D7565
Summary:
Depends on D7500.
This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).
Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, hwinkel
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7501
Summary: I've kept this as close as possible to the Git version for ease of review and later refactoring of them both together. At minimum, the functions to get the working dir should probably be cleaned up one day.
Test Plan: Landed a revision.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7534
Summary:
Ref T1049. Ref T2222. `DifferentialDiff` does not currently have a PHID, but we need it for Harbormaster and ApplicationTransactions. See some discussion in D7501.
(I split the SQL into two sections so we can't fail in the middle. At some point, I'd like to do a pass on the migration stuff and get this happening automatically, and also simplify the PatchList.)
Test Plan:
- Ran `bin/storage upgrade`.
- Checked for valid PHIDs in the database.
- Used `phid.query` to look up a diff by PHID.
- Created a new diff and verified it got a PHID.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2222, T1049
Differential Revision: https://secure.phabricator.com/D7513
Summary:
ref T182.
Simple approach of clone, patch, push. While waiting for drydock, implement a hackish mutex
setup for the workspace, which should work ok as long as there's only one committer who is
carefull about theses things.
Less obvious note: This is taking the both author and commiter's 'primary email' for the commit -
which might rub some people wrong.
Test Plan:
With a hosted repo, created some diffs and landed them.
Also clicked button for some error cases, got the right error message.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: hach-que, Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7486
Summary: The idea is to have all `phtize` definitions in applications to allow their separation.
Test Plan: Clicked View Options after mangling the translation.
Reviewers: epriestley
Reviewed By: epriestley
CC: btrahan, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7345
Summary: Adds summary (description) and test plan icons to make these area's more unique and differentiated over general sections.
Test Plan: Test a diff, a commit, a task
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7493
Summary:
I pulled these into the property list recently, which made them more consistent, but that dropped "preserve linebreaks". Since these usually come from the CLI, render with linebreaks preserved.
@csilvers, you'll need to `bin/cache purge --purge-remarkup` after this if you want to fix existing revisions.
Test Plan: Made a revision with some poetry, saw poetry preserved.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: csilvers, aran
Differential Revision: https://secure.phabricator.com/D7464
Summary: Updates the review status list to align better inside property lists. Alsu uses the default colors a bit more. This removes an overflow hidden on the value side, but that shouldnt cause any issues, given it has plenty of space.
Test Plan: tested differential and audit, highlighted and not.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7441
Summary: See title. Fixes T1809.
Test Plan:
verified each type that has flaggable interface still can be flagged
verified that new custom query filter works
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D7392
Summary:
Gets rid of some old Differential-specific nonsense and replaces it with general runtime-pluggable Remarkup rules.
Facebook: This removes two options which may be in use. Have any classes being added via config here just subclass the new abstract bases instead. This should take 5 seconds to fix. You can adjust order by overriding `getPriority()` on the rules, if necessary.
Test Plan: See comments.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, andrewjcg, aran
Differential Revision: https://secure.phabricator.com/D7393
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.
This has several parts:
- For PolicyAware queries, provide an application class name method.
- If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
- For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.
Test Plan:
- Added a unit test to verify I got all the class names right.
- Browsed around, logged in/out as a normal user with public policies on and off.
- Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7367
Summary:
Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions.
Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc.
Test Plan: Browsed several revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3415, T2222
Differential Revision: https://secure.phabricator.com/D7375
Summary:
Fixes T3675.
- Maniphest had a couple of old non-event listeners; move them to events.
- Make most of the similar listeners a little more similar.
- Add checks for access to the application.
Test Plan:
- Viewed profile, project, task, revision.
- Clicked all the actions.
- Blocked access to various applications and verified the actions vanished.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3675
Differential Revision: https://secure.phabricator.com/D7365
Summary:
Ref T3675. Some of these listeners shouldn't do their thing if the viewer doesn't have access to an application (for example, users without access to Differential should not be able to "Edit Tasks"). Set the stage for that:
- Introduce `PhabricatorEventListener`, which has an application.
- Populate this for event listeners installed by applications.
- Rename the "PeopleMenu" listeners to "ActionMenu" listeners, which better describes their modern behavior.
This doesn't actually change any behaviors.
Test Plan: Viewed Maniphest, Differntial, People.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3675
Differential Revision: https://secure.phabricator.com/D7364
Summary: Ref T1279. The new stuff seems stable, so stop writes to the old tables.
Test Plan:
- Added and removed reviewers.
- Grepped for `::RELATIONSHIP_TABLE` to verify we really have no more reads.
- Grepped for `::RELATION_REVIEWER`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7360
Summary: Fixes T4001. I broke this some time ago and no one has complained. I don't think it gets much use, and we haven't added it for the newer apps. Just get rid of it rather than adapt the URIs for ApplicationSearch.
Test Plan: Unit tests, sent myself some email.
Reviewers: zeeg, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4001
Differential Revision: https://secure.phabricator.com/D7355
Summary: Fixes T3898. This feature needs generalization at some point, but just unbreak it for now since a surprising number of users like it.
Test Plan: Pressed "z".
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran, spicyj
Maniphest Tasks: T3898
Differential Revision: https://secure.phabricator.com/D7366
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.
The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.
Two risks here:
- I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
- This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.
I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.
Test Plan:
- Before migrating, then after migrating:
- Made a bunch of inlines (drafts, submitted).
- Edited and deleted inlines.
- Verified inlines showed up in preview.
- Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
- Verified that inlines ARE indexed when they're not drafts.
- Verified that drafts inlines make revisions appear as "with draft" in the revision list.
- Made left, right, and draft inlines.
- Migrated (`bin/storage upgrade`).
- Verified that my inlines from before the migration still showed up.
- (Repeated all the stuff above.)
- Manually inspected the inline comment table.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7139
Summary:
I refactored this recently and accidentally dropped the download URI.
Also fix a warning with, e.g., files named `README`.
Test Plan: Clicked a thumb, clicked "Download", got a file.
Reviewers: chad, btrahan, dctrwatson
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7341
Summary: Adds filetype icons, applying to differential file headers. The main issue is with all the lightening, I wanted something to still anchor 'new file' on the page and adding a sharp icons does that pretty well for me. Feedback is cool too.
Test Plan: Add some new icons, test in previous commits.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7320
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.
Some policy objects don't have real PHIDs:
PhabricatorTokenGiven
PhabricatorSavedQuery
PhabricatorNamedQuery
PhrequentUserTime
PhabricatorFlag
PhabricatorDaemonLog
PhabricatorConduitMethodCallLog
ConduitAPIMethod
PhabricatorChatLogEvent
PhabricatorChatLogChannel
Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.
Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.
Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7306
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.
Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7283
Summary: Ref T603. Currently, we hard-code defense against setting policies to "Public" in several places, and special case only the CAN_VIEW policy. In fact, other policies (like Default View) should also be able to be set to public. Instead of hard-coding this, move it to the capability definitions.
Test Plan: Set default view policy in Maniphest to "Public", created a task, verified default policy.
Reviewers: btrahan, asherkin
Reviewed By: asherkin
CC: asherkin, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7276
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.
I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.
Test Plan: Created new revisions from the CLI and conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7269
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.
Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.
That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.
Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.
Also provide more context, and more useful exception messages.
Test Plan:
- See screenshots.
- Also triggered a policy exception and verified it was dramatically more useful than it used to be.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7260
Summary: Ref T1279. Prerequisite for adding icons or other type information to tokenizers, since we don't currently have enough information to prefill them when rendering things from the server side. By passing handles in, the tokenizer can extract type information.
Test Plan:
- Searched by user in Audit.
- Sent Conpherence from profile page.
- Tried to send an empty conpherence.
- Searched Countdown by user.
- Edited CCs in Differential.
- Edited reviewers in Differential.
- Edited a commit's projects.
- Searched lint by owner.
- Searched feed by owner/project.
- Searched files by owner.
- Searched Herald by owner.
- Searched Legalpad by owner.
- Searched Macro by owner.
- Filtered Maniphest reports by project.
- Edited CCs in Maniphest.
- Searched Owners by owner.
- Edited an Owners package.
- Searched Paste by owner.
- Searched activity logs by owner.
- Searched for mocks by owner.
- Edited a mock's CCs.
- Searched Ponder by owner.
- Searched projects by owner.
- Edited a Releeph project's pushers.
- Searched Releeph by requestor.
- Edited "Uses Symbols" for an Arcanist project.
- Edited all tokenizers in main search.
- Searched Slowvote by user.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7248
Summary: Ref T603. We currently bomb out here, but should just continue forward. I'm fairly certain we don't even use this for anything anymore (it has been replaced by "depends on") but need to check that.
Test Plan: Created a new revision with `arc diff`.
Reviewers: ljalonen, btrahan, #blessed_reviewers, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7255
Summary:
Ref T1279. This is a logical change.
- "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
- "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".
Practically, the primary net effect of this is just to make blocking reviews actually block.
This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.
Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7245
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.
Test Plan: Added Herald rules and updated revisions; see screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7244
Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.
This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.
Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7243
Summary: Ref T1279. If you accept a revision, also accept on behalf of all the projects you have authority to accept for.
Test Plan:
- Accepted a revision which I was a reviewer on, saw my own status and an authority project's status change to "Accepted".
- Accepted a revision which I was not a reviewer on, saw my own status be added (as "Accepted") and the project's status update.
Also, see screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7242
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:
- It puts queries very close to the display layer.
- We have to query for each revision if we want to figure out authority for several.
- We need to figure it out in several places, so we'll end up with copies of this logic.
- The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
- We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.
Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.
Test Plan:
- Looked at some revisions, verified the correct lines were highlighted.
- Looked at a revision I created and verified that projects I was a member of were not highlighted.
- With self-accept enabled, these //are// highlighted.
- Looked at a revision I did not create and verified that projects I was a member of were highlighted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7241
Summary: Ref T1279. No logical changes, but cosmetically highlight stuff you have authority for, like we do in Diffusion.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7237
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.
I will probably write some lengthy treatise on how you shouldn't use this rule later.
Implementation is straightforward because Differential previously supported this rule.
This rule can also be used to add project reviewers.
Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7235
Summary: Ref T1279. Show separate sections for "Reviewers" and "Project Reviewers" (Differential) and for "Auditors" and "Package/Project Auditors" (Diffusion/Audit).
Test Plan:
- Looked at a commit. Saw separation.
- Looked at a revision. Saw separation.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7233
Summary:
Ref T1279. Two changes to the search/query for Differential:
- "Reviewers" now accepts users and projects.
- "Responsible Users" now includes revisions where a project you are a member of is a reviewer.
Test Plan:
- Searched for project reviewers.
- Verified that the dashboard now shows reviews which I'm only part of via project membership.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7231
Summary:
Ref T1279. No actual logical changes, but:
- You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
- You can now add projects as reviewers from the revision detail typeahead.
- You can now add projects as reviewers from the CLI (`#yoloswag`).
- Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).
I'll separate projects from users in the "Reviewers" tables in the next revision.
Test Plan:
- Added projects as reviewers using the web UI and CLI.
- Used `arc amend --show --revision Dnnn` to generate commit messages.
- Viewed revision with project reviewers in web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7230
Summary: Ref T1279. Updates status to 'accepted' or 'commented' when the user takes those actions.
Test Plan:
- Commented on a revision, got a comment icon.
- Accepted a revision, got an accept icon.
- Commented again, icon stayed as "accept".
- Faked the "old diff" states.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7229
Summary:
Ref T1279. No logical changes, just updates the reviewer display style.
We currently keep track of only "requested changes".
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7228
Summary:
Ref T1279. @champo did a lot of this work already; we've been doing double writes for a long time.
Add "double reads" (reading the edge table as both the "relationship" table and as the "reviewer status" table), and migrate all the data.
I'm not bothering to try to recover old reviewer status (e.g., we could infer from transactions who accepted old revisions) because it wold be very complicated and doesn't seem too valuable.
Test Plan:
- Without doing the migration, used Differential. Verified that reads and writes worked. Most of the data was there anyway since we've been double-writing.
- Performed the migration. Verified that everything was still unchanged.
- Dropped the edge table, verified all reviweer data vanished.
- Migrated again, verified the reviewer stuff was restored.
- Did various cc/reviewer/subscriber queries, got consistent results.
Reviewers: btrahan
Reviewed By: btrahan
CC: champo, aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7227
Summary: Right now emails don't include bookmark info (wasn't added in D2897). Lets include it so it's consistent with the web UI.
Test Plan: Inspected code, made sure it matched web UI code. Verified that web UI with these changes was consistent with rendering before refactoring.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7215
Summary:
Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'.
- There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant.
- The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally.
Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3829
Differential Revision: https://secure.phabricator.com/D7214
Test Plan: Confirm the API returns a single flat result with a unified git diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, charles
Differential Revision: https://secure.phabricator.com/D7199
Test Plan:
Enable inline patches:
```
bin/config set metamta.differential.patch-format 'unified'
bin/config set metamta.differential.inline-patches 100000000
```
Create a new diff and confirm it renders correctly via email.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7198
Summary: I removed the only callsite in D7179, but forgot to remove this code.
Test Plan: Grepped for callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7194
Summary:
- "revision" is misspelled.
- Remove an unused variable.
Test Plan: Used API console to call method.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7184
Summary: See D7162. This was like 99% my fault. Just provide a header; the new ones look pretty reasonable.
Test Plan: Viewed Diffusion change view, no exception.
Reviewers: vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7183
Summary: Ref T603. Clean these up and move them to a single place.
Test Plan:
- Downloaded a raw diff.
- Enabled "attach diffs", created a revision, got an email with a diff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7179
Summary: Ref T603. Swaps out most `PhabricatorFile` loads for `PhabricatorFileQuery`.
Test Plan:
- Viewed Differential changesets.
- Used `file.info`.
- Used `file.download`.
- Viewed a file.
- Deleted a file.
- Used `/Fnnnn` to access a file.
- Uploaded an image, verified a thumbnail generated.
- Created and edited a macro.
- Added a meme.
- Did old-school attach-a-file-to-a-task.
- Viewed a paste.
- Viewed a mock.
- Embedded a mock.
- Profiled a page.
- Parsed a commit with image files linked to a revision with image files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7178
Summary:
Three changes here.
- Add `setActionList()`, and use that to set the action list.
- Add `setPropertyList()`, and use that to set the property list.
These will let us add some apropriate CSS so we can fix the border issue, and get rid of a bunch of goofy `.x + .y` selectors.
- Replace `addContent()` with `appendChild()`.
This is just a consistency thing; `AphrontView` already provides `appendChild()`, and `addContent()` did the same thing.
Test Plan:
- Viewed "All Config".
- Viewed a countdown.
- Viewed a revision (add comment, change list, table of contents, comment, local commits, open revisions affecting these files, update history).
- Viewed Diffusion (browse, change, history, repository, lint).
- Viewed Drydock (resource, lease).
- Viewed Files.
- Viewed Herald.
- Viewed Legalpad.
- Viewed macro (edit, edit audio, view).
- Viewed Maniphest.
- Viewed Applications.
- Viewed Paste.
- Viewed People.
- Viewed Phulux.
- Viewed Pholio.
- Viewed Phame (blog, post).
- Viewed Phortune (account, product).
- Viewed Ponder (questions, answers, comments).
- Viewed Releeph.
- Viewed Projects.
- Viewed Slowvote.
NOTE: Images in Files aren't on a black background anymore -- I assume that's on purpose?
NOTE: Some jankiness in Phortune, I'll clean that up when I get back to it. Not related to this diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7174
Summary: Missed this case in my sandbox
Test Plan: Reload a test diff
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7168
Summary: This adds the 'PHUIObjectBox' to nearly every place that should get it. I need to comb through Diffusion a little more. I've left Differential mostly alone, but may decide to do it anyways this weekend. I'm sure I missed something else, but these are easy enough to update.
Test Plan: tested each new layout.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7162
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.
- Provide as much detail as possible.
- Fix all the strings for i18n.
- Explain special rules to the user.
- Allow indirect policy filters to raise policy exceptions instead of 404s.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7151
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".
This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7150
Summary:
Ref T603. Make Differential behaviors for logged-out and underprivleged users more similar to other apps.
I'm going to drop this "anonymous access" thing at some point, but `reviews.fb.net` actually looks like it's running semi-modern code, so leave it alive until we have a more compelling replacement in the upstream.
Test Plan: As a logged out user, browsed Differential and clicked things and such.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7148
Summary:
Ref T603. When a diff is attached to a revision, try to guess the repository if possible. In cases where we succeed, this automatically gives us intuitive policy behavior (i.e., you can see a revision if you can see the repository the change is against).
I pulled this into a funky little "Lookup" class for two reasons:
- It's used in two places;
- I anticipate that we might need to add some sort of `explainWhy()` method if users find the heuristics confusing.
Test Plan: Created and updated revisions, saw them pick up the correct repository association. Ran Herald dry run against associable and nonassociable revisions, saw correct values populate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7147
Summary: This isn't too useful most of the time since we don't automatically populate this data yet, but works fine.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7144
Summary: Ref T603. I think T2222 is fraught with peril so I'm not going to try to sequence it ahead of T603 for Differential. Provide access to policy controls in Differential's edit view.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7142
Summary: Adds some padding to the right
Test Plan: Looked at a diff
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7143
Summary: Ref T603. Makes the majority of reads policy aware (and pretty much all the important ones).
Test Plan:
- Created a comment with `differential.createcomment`.
- Created a new revision with `arc diff` in order to exercise `differential.creatediff`.
- Created an inline comment with `differential.createinline`.
- Added a comment to a revision.
- Edited an inline comment.
- Edited a revision.
- Wrote "Depends on ..." in a summary, saved, verified link was created.
- Browsed a file in Diffusion.
- Got past the code I changed in the Releeph request thing.
- Edited a Releeph request.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7136
Summary: Ref T603. Moves policy information from a custom field to the header for revisions.
Test Plan: Looked at a revision.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7135
Summary:
Ref T603. Read policies out of policy columns.
When a revision is associated with a repository (which is currently never), require view access on the repository to see the revision (or, require the viewer to be the owner). This is a blanket "do the right thing" rule which should make Differential's default policies align with user expectations.
Future diffs will populate the `repositoryPHID` when a revision is created.
Test Plan: Tooled around Differential. None of this stuff does anything yet, so nothing very exciting happened.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7134
Summary: Ref T603. Paves the way for policy controls.
Test Plan: Ran storage upgrade, bumbled around in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7133
Summary: Ref T603. This swaps almost all queries against the repository table over to be policy aware.
Test Plan:
- Made an audit comment on a commit.
- Ran `save_lint.php`.
- Looked up a commit with `diffusion.getcommits`.
- Looked up lint messages with `diffusion.getlintmessages`.
- Clicked an external/submodule in Diffusion.
- Viewed main lint and repository lint in Diffusion.
- Completed and validated Owners paths in Owners.
- Executed dry runs via Herald.
- Queried for package owners with `owners.query`.
- Viewed Owners package.
- Edited Owners package.
- Viewed Owners package list.
- Executed `repository.query`.
- Viewed "Repository" tool repository list.
- Edited Arcanist project.
- Hit "Delete" on repository (this just tells you to use the CLI).
- Created a repository.
- Edited a repository.
- Ran `bin/repository list`.
- Ran `bin/search index rGTESTff45d13dffcfb3ea85b03aac8cc36251cacdf01c`
- Pushed and parsed a commit.
- Skipped all the Drydock stuff, as it it's hard to test and isn't normally reachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7132
Summary:
Ref T603. Make almost every task read policy-aware. Notable exceptions are:
- Edge editor -- this stuff is prescreened and should be moved to ApplicationTransactions eventually anyway.
- Search/attach stuff -- this stuff needs some general work. The actual list should be fine since you can't pull handles. There may be a very indirect hole here where you could attach an object you can't see (but do know the ID of) to an object you can see. Pretty fluff.
- The "Tasks" field in Differential will let you reference objects you can't see. Possibly this is desirable, in the case of commandeering revisions. Mostly, it was inconvenient to get a viewer (I think).
Test Plan:
- Called `maniphest.info`.
- Called `maniphest.update`.
- Batch edited tasks.
- Dragged and dropped tasks to change subpriority.
- Subscribed and unsubscribed from a task.
- Edited a task.
- Created a task.
- Created a task with a parent.
- Created a task with a template.
- Previewed a task update.
- Commented on a task.
- Added a dependency.
- Searched for "T33" in object search dialog.
- Created a branch "T33", ran `arc diff`, verified link.
- Pushed a commit with "Fixes T33", verified close.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7119
Summary: I'd like to reuse this for other content areas, renaming for now. This might be weird to keep setForm, but I can fix that later if we need.
Test Plan: reload a few forms in maniphest, projects, differential
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7120
Summary:
`getArcanistProjectName()` has some logic which gets messy with the `self::ATTACHABLE` mechanism. This makes `differential.getdiff` and similar Conduit methods throw an exception when querying a diff which doesn't have a project. See <http://pastebin.com/Czzrd0Jz>.
Instead, unconditionally attach a project (possibly `null`) when loading diffs if they need projects.
Test Plan: Ran `differential.getdiff` against a `arc diff --raw` diff with no project, got a result instead of an exception.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, sttwister
Differential Revision: https://secure.phabricator.com/D7101
Summary: Adds status icons and colors to Maniphest and Differential. Also minor tweaks to them in hovercards. Probably some other stuff too.
Test Plan: Test many diff and task states.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7098
Summary:
- Kicks it out to full width.
- More useful header/crumbs/properties/actions (needs some more work).
- Works for public repositories.
- Fix a bug where the "rX" crumb would lose the branch you're on.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7063
Summary:
Ref T603. See inlines for an explanation. The case where I hit this was loading the "Pending Differential Revisions" panel in Diffusion when logged out, after making a repository public.
What happens is that we load 10 revisions (say, D1 .. D10) but the user can't see any of them. We then try to load the next 10, but since the pagination is ordered by date modified, we need to base the next query on the modified date of the last thing we loaded (D10). However, since we use the viewer's policies to load that cursor object, it fails to load, and then we just issue the same query over and over again, loading D1 .. D10 until we run out of execution time.
Test Plan: Interface now loads correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7059
Summary:
If handed a revision ID, we might get more than one result, which causes `executeOne()` to throw. Instead, translate the revision id into a diff ID before querying for the diff.
Also one small consistency change to parameter casing.
Test Plan: Used console to query for a revision with more than one diff using the revision id.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Differential Revision: https://secure.phabricator.com/D7026
Summary: We were returning an array here when previous return was a string.
Test Plan: reload diff
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7025
