Summary:
Ref T5873. This provides paths and line numbers for inline comments.
This is a touch hacky but I was able to keep it mostly under control.
Test Plan:
- Made inline comments.
- Called API, got path/line information.
{F5120157}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873
Differential Revision: https://secure.phabricator.com/D18469
Summary: Minor cleanup, this logic can be simpler. Instead of special-casing inlines as having an effect if the have a comment, just consider any transaction with a comment to have an effect. I'm fairly certain this is always true.
Test Plan: Made inlines, tried to submit empty comments. Behavior unchanged.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18468
Summary:
Ref T5873. See PHI14. I don't want to just expose internal transaction data to Conduit by default, since it's often: unstable, unusable, sensitive, or some combination of the three.
Instead, let ModularTransactions opt in to providing additional data to Conduit, similar to other infrastructure. If a transaction doesn't, the API returns an empty skeleton for it. This is generally fine since most transactions have no real use cases, and I think we can fill them in as we go.
This also probably builds toward T5726, which would likely use the same format, and perhaps simply not publish stuff which did not opt in.
This doesn't actually cover "comment" or "inline comment", which are presumably what PHI14 is after, since neither is modular. I'll probably just put a hack in place for this until they can modularize since I suspect modularizing them here is difficult.
Test Plan: Ran `transaction.search` on a revision, saw some transactions (title and status transactions) populate with values.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5873
Differential Revision: https://secure.phabricator.com/D18467
Summary:
Fixes T12970. This is easier than I expected, and appears to occur in only one place.
This prevents a change from ever generating with an anchor like `#12345678`, which is ambiguous because it may be a comment anchor.
Test Plan: Viewed a revision, saw new `change-xxxyyyzzz` anchors, clicked one, got jumped to the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12970
Differential Revision: https://secure.phabricator.com/D18465
Summary:
Ref T2543. This updates and migrates the status change transactions:
- All storage now records the modern modular transaction ("differential.revision.status"), not the obsolete non-modular transaction ("differential:status").
- All storage now records the modern constants ("accepted"), not the obsolete numeric values ("2").
Test Plan:
- Selected all the relevant rows before/after migration, data looked sane.
- Browsed around, reviewed timelines, no changes after migration.
- Changed revision states, saw appropriate new transactions in the database and timeline rendering.
- Grepped for `differential:status`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18419
Summary:
Ref T2543. Rewrites all the storage to use constants.
Note that transactions still use legacy values, I'll migrate and update them separately.
Test Plan:
- Ran migration.
- Browsed around, changed revision states, viewed dashboard, etc.
- Selected `DISTINCT()` and `GROUP_CONCAT()` of the `status` field in the database, saw sane/expected before and after values.
- Verified that old Conduit methods still return numeric constants for compatibility.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18418
Summary: Ref T2543. All writers now write modern statuses. Make all readers explicit about whether they are reading modern or legacy statuses, so I can swap the storage format.
Test Plan:
- Grepped for `getStatus()`, scanned the list. Other applications have methods with this name so it's possible I missed something.
- Browed around, changed revision statuses.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18417
Summary: Ref T2543. Update these for the modern stuff.
Test Plan: Created a new revision, got a revision in the right state ("Needs Review"). Accepted, planned, requested, abandoned revision; state transitions looked good.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18415
Summary: Ref T2543. Swaps these over to modern constants.
Test Plan: Viewed dashboard, no chagnes to bucketing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18414
Summary: Ref T2543. This cleans up all the "when no one is rejecting/blocking and someone accepted, mark the revision overall as accepted" logic to use more modern status stuff instead of `ArcanistDifferentialRevisionStatus`.
Test Plan:
- Updated revisions, saw them go to "Needs Review".
- Accepted, requested changes to revisions.
- Updated one with changes requested, saw it go to "needs review" again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18413
Summary:
Ref T2543. This cleans up a couple of remaining rough edges:
- We could do an older TYPE_ACTION "close" via the daemons.
- We could do an older TYPE_ACTION "close" via `arc close-revision`, explicitly or implicitly in `arc land`, via API (`differential.close`).
- We could do an older TYPE_ACTION "rethink" ("Plan Changes") via the API, via `arc diff --plan-changes` (`differential.createcomment`).
Move these to modern modular transactions, then get rid of all the validation and application logic for them. This nukes a bunch of `ArcanistDifferentialRevision::...` junk.
Test Plan:
- Used `bin/repository reparse --message rXYZ...` to reparse a commit, closing a corresponding revision.
- Used `differential.close` to close a revision.
- Used `differential.createcomment` to plan changes to a revision.
- Reviewed transaction log for full "closed by commit" message (linking to commit and mentioning author).
- Grepped for `::TYPE_ACTION` to look for remaining callsites, didn't find any.
- Grepped for `differential.close` and `differential.createcomment` in `arcanist/` to look for anything suspicious, seemed clean.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18412
Summary:
Ref T2543. Converts the TYPE_STATUS transaction (used to render "This revision now requires changes to proceed.", "This revision is accepted and ready to land.", etc) to ModularTransactions.
Also, continue consolidating all the status-related information (here, more colors and icons) into a single place. By the end of this, we may learn that NEEDS_REVIEW uses //every// color.
Test Plan:
Reviewed old status transactions (unchanged) and created new ones (looked the same as the old ones).
(I plan to migrate all of these a few diffs from now, around when I change the storage format.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18410
Summary: Ref T2543. Cleans up some more references to ArcanistDifferentialRevisionStatus, moving toward getting rid of it completely.
Test Plan: Planned changes, requested review, inspected the "close" one since it isn't trivial to trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18408
Summary: Ref T2543. Now that the integer status constants are banished to the internals, we can expose status information from "differential.revision.search".
Test Plan:
Searched for revisions.
{F5093873}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18400
Summary: Ref T2543. I converted this condition the wrong way, missing a `!`. I'll cherry-pick this to `stable`.
Test Plan: No more "Reopen Revision" action available on open revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18399
Summary:
Ref T2543. Ref T10967. This isn't precisely related to "draft" status, but while I'm churning this stuff anyway, get rid of the old double writes to clean the code up a bit.
These were added in T10967 to make sure the migration was reversible/recoverable, but we haven't seen any issues with it in several months so I believe they can now be removed safely. Nothing has read this table since ~April.
Test Plan: Took various review actions on revisions (accept, reject, resign, comment, etc). If this change is correct, there should be no visible effect.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967, T2543
Differential Revision: https://secure.phabricator.com/D18398
Summary:
Ref T2543. I believe there have been no upstream callsites of this method since D1646, in February 2012.
The method works, and we can revert this if needbe, but this seems like a good time to remove support.
Test Plan: Grepped for `differential.find`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18397
Summary: Ref T2543. All callsites are now in terms of `withStatuses()`.
Test Plan:
- Called `differential.query` and `differential.find` from Conduit API.
- Grepped through all `withStatus()` callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18396
Summary: Ref T2543. Several queries want only open revisions. Provide a tailored, non-legacy way to issue that query.
Test Plan: Viewed some of these callsites (e.g., "Similar open revisions affecting these files"), saw only open revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18395
Summary:
Ref T2543. This updates the UI control in the web UI. Also:
- This implicitly makes this queryable with the API (`differential.revision.search`); it previously was not.
- This does NOT migrate existing saved queries. I'll do those in the next change, and hold this until it happens.
- This will break some existing `/differential/?status=XYZ` links. For example, `status=open` now needs to be `status=open()`. I couldn't find any of these in the upstream, and I suspect these are rare in the wild (users would normally link directly to saved queries, not use URI query construction).
Test Plan: {F5093611}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18393
Summary:
Ref T2543. This adds a tokenizer, similar to the Maniphest tokenizer, so the hard-coded `<select />` control in Differential ApplicationSearch can be replaced with a more flexible control that handles the addition of new statuses with more grace.
This only adds the new datasource.
Test Plan: Used `/typeahead/class/` to preview the behavior of the new datasource.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18392
Summary: See PHI31. The "Accepted Older Revision" icon is (more reasonably) bluegrey, but that rule spilled over here where it doesn't make much sense. "Requested Changes to Prior Diff" remains in effect across updates, but the coloration implies otherwise.
Test Plan:
"Requested Changes to This Diff" (unchanged):
{F5092019}
"Requested Changes to Prior Diff" (now red, previously bluegrey):
{F5092020}
Note that the icons are different so this is technically colorblind-safe, and it's normally not important to distinguish between these two reds anyway.
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Differential Revision: https://secure.phabricator.com/D18385
Summary:
Ref T2543. Currently, Differential uses a set of hard-coded query filters (like "open" and "closed") to query revisions by status (for example, "open" means any of "review, revision, changes planned, accepted [usually]").
In other applications, like Maniphest, we've replaced this with a low level list of the actual statuses, plus higher level convenience UI through tokenizer functions. This basically has all of the benefits of the hard-coded filters with none of the drawbacks, and is generally more flexible.
I'd like to do that in Differential, too, although we'll need to keep the legacy maps around for a while because they're used by `differential.find` and `differential.getrevision`. To prepare for this, pull all the legacy stuff out into a separate class. Then I'll modernize where I can, and we can get rid of this junk some day.
Test Plan: Grepped for `RevisionQuery::STATUS`. Ran queries via Differential UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18343
Summary:
Ref T2543. Add `isPublished()` to mean: exactly the status 'closed', which is now interally called 'published', but still shown as 'closed' to users.
We have some callsites which are about "exactly that status", vs "any 'closed' status", e.g. including "abandoned".
This also introduces `isChangePlanned()`, which felt less awkward than `isChangesPlanned()` but more consistent than `hasChangesPlanned()` or `isStatusChangesPlanned()` or similar.
Test Plan: `grep`, loaded revisions, requested review.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18341
Summary:
Ref T2543. Further consolidates status management into DifferentialRevisionStatus.
One change I'm making here is internally renaming "CLOSED" to "PUBLISHED". The UI will continue to say "Closed", at least for now, but this should make the code more clear because we care about "is closed, exactly" vs "is any closed status (closed, abandoned, sometimes accepted)". This distinction is more obvious as `isClosed()` vs `isPublished()` than, e.g., `isClosedWithExactlyTheClosedStatus()` or something. I think "Published" is generally more clear, too, and more consistent with modern language (e.g., "pre-publish review" replacing "pre-commit review" to make it more clear what we mean in Git/Mercurial).
I've removed the IN_PREPARATION status since this was just earlier groundwork for "Draft" and not actually used, and under the newer plan I'm trying to just abandon `ArcanistDifferentialRevisionStatus` entirely (or, at least, substantially).
Test Plan:
- Viewed revisions.
- Viewed revision list.
- Viewed revisions linked to a task in Maniphest.
- Viewed revision graph of dependencies in Differential.
- Grepped for `COLOR_STATUS_...` constants.
- Grepped for removed method `getRevisionStatusIcon()` (no callsites).
- Grepped for removed method `renderFullDescription()` (one callsite, replaced with just building a `TagView` inline).
- Grepped for removed method `isClosedStatus()` (no callsites after other changes).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18340
Summary:
Ref T2543. These are currently numeric values, like "0" and "3". I want to replace them with strings, like "accepted", and move definitions from Arcanist to Phabricator.
To set the stage for this, reduce the number of callsites where Phabricator invokes `ArcanistDifferentialRevisionStatus`.
This is just the easy ones. I'll hold this until the release cut.
Test Plan:
- Called `differential.find`.
- Called `differential.getrevision`.
- Called `differential.query`.
- Removed all reviewers from a revision, saw warning.
- Abandoned the no-reviewers revision, no more warning.
- Attached a revision to a task to get it to show the state icon with the status on a tooltip.
- Viewed revision bucketing on dashboard.
- Used `bin/search index` to reindex a revision.
- Hit the "Land Revision" endpoint.
I didn't explicitly test these cases:
- Doorkeeper Asana integration, since setup takes a thousand years.
- Disambiguation logic when multiple hashes match, since setup is also very involved.
- Releeph because it's Releeph.
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18339
Summary: Fixes T12952. This never work AFAIK, so resolves this mis-information. See T4411 for follow up.
Test Plan: Click on policy for a diff, no longer see text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12952
Differential Revision: https://secure.phabricator.com/D18349
Summary:
Reverts D18276. See PHI18 for discussion. The additional rules here (roughly, "only show the first successful operation") didn't actually work out for the other types of operations.
This is all just figuring out a stopgap, T12935 and other changes should eventually provide real pathways here.
Test Plan: Straight revert.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18281
Summary:
See PHI18. Third parties can currently define other types of Drydock operations (like "Merge Check" or "Cherry-Pick") but we won't show them in the UI.
This is a simple change which improves third-party support for now. These kinds of operations generally make sense in the upstream, but the pathways to support are longer.
Test Plan:
- Verified that there are no other types of repository operation which we'd want to exclude in the upstream today by reviewing the "Repository Operation" subclasses.
- Will click some buttons in production to make sure this works.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18276
Summary: Cleans up colors, removes commit hash and links the text instead. Also unsure how valuable "lint" column is here, but left it. I'd maybe like to understand that workflow since it just seems like clutter overall. Also Fixes T12905
Test Plan:
Review Phabricator, hg, and a few other test repositories locally. Holler if anything here seems bad, but this feels easier to read and use to me.
{F5038425}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12905
Differential Revision: https://secure.phabricator.com/D18189
Summary:
When users use the web UI to enter text like "Reviewers: x" into the "Summary" or "Test Plan", we can end up with an ambiguous commit message.
Some time ago we added a warning about this to the "Summary" field, and //attempted// to add it to the "Test Plan" field, but it actually gets called from the wrong place.
Remove the code from the wrong place (no callers, not reachable) and put it in the right place.
This fixes an issue where users could edit a test plan from the web UI to add the text "Tests: ..." and cause ambiguities on a later "arc diff --edit".
Test Plan: {F5026603}
Reviewers: chad, amckinley
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18175
Summary: Ref T12845. This moves the "enum" and "string" types to the new code.
Test Plan: Set, deleted, and tried to set invalid values for various enum and string config values (header color, mail prefixes, etc) from the CLI and web.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18156
Summary: This was accidentally caught in the crossfire in D18150. This is stable enough to formalize instead of adding with an event hook.
Test Plan: Looked at a candidate revision, saw "Land Revision" appear in UI again.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18154
Summary:
Fixes T12869. This is a very old, pre-Drydock chunk of code from D7486 and some followups.
It does three things:
- "Land to Hosted Git": Obsoleted by Drydock, has been commented out in HEAD for a very long time with no complaints. Disabled by D8719 in 2014.
- "Land to Hosted Mercurial": Could be obsoleted by Drydock with a fairly small amount of work, but currently has no replacement. Unclear if this sees any real use. Not actually disabled at HEAD.
- "Land to GitHub": Use GitHub OAuth credentials to land to GitHub. This is sort of theoretically useful and has no analog today. Disabled by D13022 in 2015.
This stuff was largely disabled a long time ago and we haven't seen users hitting issues with it. This could all be moved to an extension today if anyone still relies on it.
Test Plan: Grepped for removed classes, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12869
Differential Revision: https://secure.phabricator.com/D18150
Summary:
Fixes T8909. Ref T12733.
UI attempts to follow the mock, but is a bit rough since PHUIXButtonView without text in this menu gets weird spacing, we don't have circular buttons yet, and PHUIXActionView without an icon also gets odd spacing.
Test Plan: {F5003125}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733, T8909
Differential Revision: https://secure.phabricator.com/D18128
Summary:
Ref T12733. This paves the way for a separate "hide" operation which completely hides things.
(I didn't extend this to the server side because that would require schema changes and the new "hide" state is client-only.)
Test Plan: Collapsed and expanded inlines, viewed tooltips.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18126
Summary: Adds a very basic list of all inline comments, threaded, and their status. Kept this a little simpler than the mock, mostly because sorting here feels a little strange given threads would be all over the place. Not sure sorted is needed in practice anyways. I'd probably lean towards just adding a JS checkbox to hide certain rows if needed in the future.
Test Plan:
Test various commenting structures:
- Leave Comment
- Update Diff
- Leave new comment
- Reply to comment
- Reply to comment as revision author
- Mark items as done
- Update diff again
{F4996915}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18112
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.
Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18077
Summary: Ref T12733. Completely removes the objectives UI.
Test Plan:
- Grepped for `objective`, etc.
- Browsed revisions, no JS errors / broken stuff.
- (If I missed anything, it's likely to turn up in followup changes.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18043
Summary:
Fixes T12757. Here's a simple repro for this:
- Add a package you own as a reviewer to a revision you're reviewing.
- Open two windows, select "Accept", don't submit the form.
- Submit the form in window A.
- Submit the fomr in window B.
Previously, window B would show an error, because we considered accepting on behalf of the package invalid, as the package had already accepted.
Instead, let repeat-accepts through without complaint.
Some product stuff:
- We could roadblock users with a more narrow validation error message here instead, like "Package X has already been accepted.", but I think this would be more annoying than helpful.
- If your accept has no effect (i.e., everything you're accepting for has already accepted) we currently just let it through. I think this is fine -- and a bit tricky to tailor -- but the ideal/consistent beavior is to do a "no effect" warning like "All the reviewers you're accepting for have already accepted.". This is sufficiently finnicky/rare (and probably not terribly useful/desiable in this specific case)that I'm just punting.
Test Plan: Did the flow above, got an "Accept" instead of a validation error.
Reviewers: chad, lvital
Reviewed By: chad, lvital
Subscribers: lvital
Maniphest Tasks: T12757
Differential Revision: https://secure.phabricator.com/D18019
Summary:
Ref T12733.
- While editing a comment, show a pink star ({icon star, color=pink}) with a tooltip.
- Slight UI tweaks, including draft comments getting an indigo pencil ({icon pencil, color=indigo}).
Test Plan: {F4968470}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17977
Summary: See D17955.
Test Plan: Loaded a revision, no longer saw annotations with prototypes off. Still saw annotations with prototypes on.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17983
Summary:
Minor UI tweaks:
- Use the dynamic icon for each file (e.g., image, text), not a hard-coded icon.
- Render the path (less important) in grey and the filename (more important) in black.
Test Plan: {F4966176}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17957
