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
