Summary:
See <https://twitter.com/HayleyCAnderson/status/988873585363009536>.
Currently, the action dropdown in Differential shows a heavy "X" after "Request Changes" and a heavy checkmark after "Accept Revision".
Although I'm not convinced that the messaging around "Request Changes" is too strong, I do think these marks are out of place in modern Differential. They came from a simpler time when this dropdown had fewer actions, but feel a little weird and inconsistent to me in the modern UI.
Let's try getting rid of them and see how it goes?
Test Plan:
- Viewed these actions in the dropdown, no longer saw the mark icons.
- Grepped for these unicode sequences without getting any other hits.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19405
Summary:
Ref T13130. See PHI483. Currently, "Plan Changes + Draft" uses rules like "Plan Changes", not rules like "Draft", and allows "Accept".
This isn't consistent with how "Draft" and "Accept" work in other cases. Make "Plan Changes + Draft" more like "Draft" for consistency.
Also fix a string that didn't have a natural English version.
Test Plan:
- Added a failing build plan.
- Created a revision.
- Loaded the revision before builds completed, saw a nicer piece of text about "waiting for builds" instead of "waiting for 2 build(s)".
- Builds failed, which automatically demoted the reivsion to "Changes Planned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense (particularly, no "Accept").
- Abandoned the revision to test "Abandoned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense.
- Reclaimed the revision, then used "Request Review" to send it to "Needs Review". Verified that actions made sense and, e.g., reviewers could now "Accept" normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19398
Summary:
Ref T13124. See PHI584. When you create a draft revision and it automatically demotes to "Changes Planned + Draft" because builds fail, let it promote to "Needs Review" automatically if builds pass. Usually, this will be because someone restarted the builds and they worked the second time.
Although I'm a little wary about adding even more state transitions to the diagram in T13110#237736, I think this one is reasonably natural and not ambiguous.
Test Plan:
- Created a failing build plan with a "Throw Exception" step.
- Created a revision which hit the build plan, saw it demote to "Changes Planned" when Harbormaster failed.
- Edited the build plan to remove the "Throw Exception" step, restarted the build, got a pass.
- Saw revision promote again:
{F5526104}
I didn't exhaustively test that the other 40 state transitions still work properly, but I think the scope of this change is small enough that it's unlikely I did much collateral damage.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13124
Differential Revision: https://secure.phabricator.com/D19380
Summary:
Ref T13124. See PHI593.
When you `arc diff` in a Git or Mercurial repository, we upload some information about the local commits in your working copy which the change was generated from.
In the future (for example, with T1508) we may increase the prominence of this feature.
Provide a stable way to read this information back via the API. This roughly mirrors the information we provide about commits in "diffusion.commit.search", although the latter is less fleshed-out today.
Test Plan: Used `differential.diff.search` to retrieve commit information about Git, Mercurial, and Subversion diffs. (There's no info for Subversion, but it doesn't crash or anything.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13124
Differential Revision: https://secure.phabricator.com/D19386
Summary:
Ref T13127. Users with red/green colorblindness may have difficulty using this element in its current incarnation.
We could give it different behavior if the "Accessibility" option is set for red/green colorblind users, but try a one-size-fits-all approach since the red/green aren't wholly clear anwyay.
Test Plan: {F5530050}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13127
Differential Revision: https://secure.phabricator.com/D19385
Summary:
Depends on D19370. See T13124. See PHI549. The particular install in PHI549 migrated a large amount of data via the fallback hunk migration script, which does not compress hunks.
Add a mode to `bin/differential migrate-hunk` that amounts to "compress all the hunks which would benefit from compression".
Test Plan: Ran `bin/differential migrate-hunk` with `--auto`, `--all`, `--to`, `--id`, and `--dry-run` in various mixtures. Forced a bunch of hunks to raw ("byte") format, saw it cleanly upgrade them to compressed ("gzde") format.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19371
Summary:
Depends on D19369. Ref T13120. Add a flag to migrate every hunk.
This isn't terribly useful on its own, but I'm going to add an `--auto` flag next so that you can run `--auto --all` to migrate hunks to the preferred hunk format.
Test Plan: Ran `bin/differential migrate-hunk --all --to text`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120
Differential Revision: https://secure.phabricator.com/D19370
Summary: This is a good spelling, but maybe a better spelling is possible.
Test Plan: hmmm
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19369
Summary:
See PHI573. Ref T13120. Drafts were recently changed so that "draft" and "broadcast" are separate flags, and you can have non-broadcasting revisions in states other than "draft" if builds fail on a draft or you abandon a draft.
However, when draft mode is entered with `arc diff --draft` and you have prototypes off, this flag wasn't being set correctly.
Test Plan: Disabled prototypes, created a revision with `arc diff --draft`, observed that `draft.broadcast` is now correctly `false`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120
Differential Revision: https://secure.phabricator.com/D19360
Summary:
See PHI574. Ref T13120. When you `Ref Txx` or `Fixes Txxx`, we mark it "unmentionable" to prevent the task from generating both a reference and a mention.
If you add a reference to an object (like a commit hash) to a custom remarkup field, there's currently no real way to prevent it from generating a mention, except that you can explicitly mark the PHID as unmentionable on the Editor.
This isn't exactly a first-class feature, but we technically do it in `PhabricatorRepositoryCommitMessageParserWorker`, and it probably doesn't hurt or interfere with anything to support it slightly better.
In Differential, respect any existing value and append new values to it rather than overwriting the value.
Test Plan: Edited a revision summary to include `Ref Txxx`, saw only a reference (not a mention) generate.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120
Differential Revision: https://secure.phabricator.com/D19361
Summary: This reverts D18524. See that revision for discussion.
Test Plan: Viewed home menu, saw application names as menu items.
Differential Revision: https://secure.phabricator.com/D19308
Summary:
Depends on D19296. Ref T13110.
- Remove the "Large Changesets" documentation since we now degrade very large changesets and I don't have any evidence that anyone has ever tried to follow any of the recommendations in this document.
- Remove references to it.
- When an older revision doesn't have denormalized size information on the Revision object itself, don't render a scale element (instead of rendering a bogus one).
- Try to improve terminology consistency around "Large Change" (100-1000 files) vs "Very Large Change" (1000+ files) vs "Enormous Change" (too large to hold in memory).
Test Plan: Viewed revisions; grepped for documentation.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19298
Summary: Depends on D19295. Ref T13110. Degrade the review UX when users try to interact with changes which are too large to receive human review.
Test Plan: Reduced the "very large" limit, browsed some changes, saw various elements degrade.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19296
Summary: Ref T13110. Installs have various reasons for sending unreviewable changes (changes where the text of the change will never be reviewed by a human) through Differential anyway. Prepare for accommodating this more gracefully by building a standalone changeset list page which paginates the changesets.
Test Plan: Clicked the new "Changeset List" button on a revision, was taken to a separate page.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19295
Summary: Ref T13110. See PHI230. Show revision sizes on a roughly logarithmic scale from 1-7 stars. See D16322 for theorycrafting on this element.
Test Plan: Looked at some revisions, saw plausible-looking size markers.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19294
Summary:
See PHI489. Ref T13110. At least for now, this just shows "..." at the end since you can click the revision to see the whole list anyway.
Also remove the older-style external Handle passing in favor of lazy construction via HandlePool.
Test Plan: Viewed revisions, fiddled with the 7 limit, got sensible-seeming "..." behavior.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19293
Summary: Depends on D19289. Ref T13110. This flag has been obsolete for some time and has no callers.
Test Plan: Grepped for `hasReviewTransaction`, no hits.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19290
Summary:
Depends on D19288. Ref T13110. In addition to kicking revisions back to "Changes Planned" when builds fail, notify the author that they need to fix their awful garbage change.
(The actual email could be more useful than it currently is.)
Test Plan: Created a revision with failing remote builds, saw email about the problem generate.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19289
Summary: Depends on D19287. Ref T13110. Currently, "Abandon" and then "Reclaim" moves you out of "Draft" without setting the "Should Broadcast" flag. Keep these revisions in draft instead.
Test Plan: Reclaimed an abandoned + draft revision, got a draft revision instead of a "needs review + nonbroadcast" revision (which isn't a meaningful state).
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19288
Summary:
Depends on D19286. Ref T13110. After builds fail remote builds, put revisions back in the author's queue.
This doesn't actually notify the author quite yet.
Test Plan: Made a failing build plan run on revisions, created a revision, saw it demote after builds failed.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19287
Summary: Depends on D19285. Ref T13110. When you update an "Abandoned + But, Never Promoted" revision or (in the future) a "Changes Planned + But, Never Promoted" revision, return it to the "Draft" state rather than promoting it.
Test Plan: Updated an "Abandoned + Draft" revision, saw it return to "Draft".
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19286
Summary:
Depends on D19284. Ref T13110. It's now possible to get a revision into a "Abandoned + But, Never Promoted From Draft" state. Show this in the header and provide the draft hint above the comment area.
Also, remove `shouldBroadcast()`. The method `getShouldBroadcast()` now has the same meaning.
Finally, migrate existing drafts to `shouldBroadcast = false` and default `shouldBroadcast` to `true`. If we don't do this, every older revision becomes a non-broadcasting revision because this flag was not explicitly set on revision creation before, only on promotion out of draft.
Test Plan: Ran migration; abandoned draft revisions and ended up in a draft + abandoned state.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19285
Summary:
Depends on D19283. Ref T13110. To enable "Changes Planned + But, Still A Draft" and "Abandoned + But, Never Promoted From Draft" states, decouple the "broadcast" flag from the "draft" state.
Broadcast behavior is now based only on the `shouldBroadcast` flag, and revisions in any state may have this flag.
Revisions gain this flag when created as a non-draft, or when they leave the draft state for the first time.
There are probably still some ways you can get the wrong result here -- maybe abandon + update -- but those can be cleaned up as they arise.
Test Plan: Kinda poked it a bit but I'll vet this more heavily at the end of this sequence.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19284
Summary:
Depends on D19282. Ref T13110. I want to introduce "Changes Planned + Still A Draft" and "Abandoned + Still A Draft" states, at a minimum.
I think the "hasBroadcast" flag is effectively identical to a hypothetical "stillADraft" flag, so rename it to "shouldBroadcast" to better match its intended behavior.
This just changes labels, not any behavior.
Test Plan: Grepped for `hasBroadcast` and `HAS_BROADCAST`.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19283
Summary:
Depends on D19281. This increases consistency between build timeline publishing and revision draft promotion.
There's no real behavioral change here (switching how publishing worked already changed the beahvior) but this sends more callsites down the same code paths.
Since the builds we're looking at include completed builds, change the term "active" to "impactful". This describes the same set of builds, but hopefully describes them more accurately.
Test Plan: Created a local revision, saw it plausibly interact with draft status and promote. There are a lot of moving parts here and some stuff may well have slipped through.
Differential Revision: https://secure.phabricator.com/D19282
Summary:
Depends on D19280. Ref T13110. Although Harbormaster cares about all builds, Differential does not practically care about local lint and unit results in determining build status.
In Differential, orient publishing around "remote builds" instead of "builds".
This does not yet change any of the draft logic, it just makes the timeline story use newer logic.
Test Plan: Used `bin/harbormaster publish` (with some guard-clause removal) to publish some buildables to revisions without anything crashing.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19281
Summary:
Ref T13110. Currently, build status is published the same way for every Buildable by the BuildEngine.
I want to change this to delegate publishing to each Buildable, particularly so that Differential may use more detailed rules for handling builds and drafts.
Rather than add additional methods to the existing `BuildableInterface`, add an engine generator method instead. This is a pattern which has seen more use recently (e.g., in Ferret) and lets us pay a little more upfront to pull complex pieces of logic out of the main class and let them use inheritence more easily. If we had Traits that might cover this to some degree.
I'd expect to eventually reduce the size of `BuildableInterface` and move the `CircleCI` and `BuildKite` interfaces so that the `BuildableEngine` implements them instead of the main object.
Here, this new engine does nothing and is never instantiated. In upcoming changes, publishing logic will move into it so that Differential can handle publishing differently.
Test Plan: Ran `arc liberate`, loaded pages, grepped for `BuildableInterface`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19278
Summary:
Ref T13114.
- Followup fix for D19267, which didn't work correctly with //new// revision creation.
- Followup fix for changes in T11015. Some of the querying logic was still handling "/x.y" and "/x.y/" differently. Instead, normalize consistently to "/x.y/"
Test Plan:
- Created a new revision cleanly.
- Created a package owning only a `example.txt` file and saw Differential find it as an owning package in the table of contents.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19268
Summary: Ref T13114. See PHI515. Updating a revision with the same, currently active diff became an error at some point (probably D19175). This is inconsistent; make it an allowable no-op instead.
Test Plan:
- Updated a revision's diff via Conduit.
- Updated to the same diff, no-op.
- Tried to update a different revision, error ("already attached elsewhere").
- Updated with a different diff.
- Tried to update with the original diff, error ("previously attached version").
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19267
Summary:
This change prevents the following error when using PHP 7.2:
```
ERROR 2: count(): Parameter must be an array or an object that implements Countable at [/usr/local/lib/php/phabricator/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php:132]
```
A similar issue was fixed in D18964
Test Plan: Tested in a live system.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19242
Summary:
See PHI457. There's no real reason not to allow this, it just wasn't clear if it was useful. See D18626.
An install had a user `arc diff` and then sprint out the door to take a very long vacation before the builds finished. One failed, so the revision is stuck as a draft forever. This seems like a reasonable motivation for allowing "Commandeer".
Test Plan: Successfully commandeered a draft.
Differential Revision: https://secure.phabricator.com/D19228
Summary:
See PHI431. Ref T13102. An install is interested in a custom "non-sticky" accept action, roughly.
On the one hand, this is a pretty hacky patch. However, I suspect it inches us closer to T731, and I'm generally comfortable with exploring the realms of "Accept Next Update", "Unblock Without Accepting", etc., as long as most of it doesn't end up enabled by default in the upstream.
Test Plan:
- Accepted and updated revisions normally, saw accepts respect global stickiness.
- Modified the "Accept" action to explicitly be unsticky, saw nonsticky accept behavior after update.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19211
Summary: See PHI433. This beefs up reminder texts for drafts a little bit since some users in the wild aren't always seeing/remembering the existing, fairly subtle hints.
Test Plan: Created a reivsion with `--draft`, viewed it, saw richer reminders.
Differential Revision: https://secure.phabricator.com/D19204
Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.
Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.
Maniphest Tasks: T13099, T11664
Differential Revision: https://secure.phabricator.com/D19180
Summary:
Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance.
These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story.
Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time.
Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly".
Maniphest Tasks: T13099, T12787
Differential Revision: https://secure.phabricator.com/D19179
Summary: Depends on D19175. Ref T13099. This fills in "close" and "update" transactions so that they show which commit(s) caused the action.
Test Plan: Used `transaction.search` to query some revisions, saw commit PHID information.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19176
Summary: Ref T13099. Move most of the "Update" logic to modular transactions
Test Plan: Created and updated revisions. Flushed the task queue. Grepped for `TYPE_UPDATE`. Reviewed update transactions in the timeline and feed.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19175
Summary:
See PHI416. If you raise a lint message in a deleted file, we don't render any text on the right hand side so the message never displays.
This is occasionally still legitimate/useful, e.g. to display a "don't delete this file" message. At least for now, show these messages on the left.
Test Plan: Posted a lint message on a deleted file via `harbormaster.sendmessage`, viewed revision, saw file expand with synthetic inline for lint.
Differential Revision: https://secure.phabricator.com/D19171
Summary:
Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead:
- Pick a default width somewhere between the two.
- Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it).
- Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place).
Test Plan:
- Without settings, loaded page: got medium-width bar.
- Dragged bar wide/narrow, toggled on/off with "f", got persistent width.
- Dragged bar wide/narrow, reloaded page, got persistent width.
- Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19129
Summary:
Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work.
For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint".
Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway.
Remove all the aggregation stuff for now, it's not really clear to me what this should look like.
Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19119
Summary: Revisions with blocking reviewers had this stamp built incorrectly, which cascaded into trying to use `array()` as a PHID. Recover so these tasks succeed.
Test Plan: Will deploy production.
Differential Revision: https://secure.phabricator.com/D19082
Summary: Like "Commit Hook" rules, these also fire oddly and don't have an object PHID or a list of transactions.
Test Plan: Verified that "Call Webhooks" was no longer available from Diff rules, but still available from other rule types.
Differential Revision: https://secure.phabricator.com/D19069
Summary:
See PHI346. Ref T13054. If you have prototypes enabled on the server but use `master` / `stable` on the client and run `arc diff --plan-changes`, the transition is rejected because "Draft -> Changes Planned" isn't currently a legal transition.
Allow this transition if not coming from the web UI (to keep it out of the dropdown).
Test Plan:
- Ran `arc diff --plan-changes` on `master`, got a "Changes Planned" revision instead of a validation error.
- Ran `arc diff` without `--plan-changes`, got a draft, verified that "Plan Changes" still doesn't appear in the action dropdown.
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19067
Summary:
Depends on D19065. Ref T13054. Instead of just updating `containerPHID` and hoping for the best, queue a proper BuildWorker to process a "your container has changed, update it" message.
We also need to remove a (superfluous) `withContainerPHIDs()` when loading active diffs for a revision.
Test Plan:
- Without daemons, created a revision and saw builds stick in "preparing" with no container PHID, but also stay in draft mode.
- With daemons, saw builds actually build and get the right container PHID.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19066
Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
Summary: Depends on D19056. Fixes T8475. Ref T13054. Merges "ModernHunk" back into "Hunk".
Test Plan: Grepped for `modernhunk`. Reviewed revisions. Created a new revision. Used `bin/differential migrate-hunk` to migrate hunks between storage formats and back.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19057
Summary: Ref T13054. Ref T8475. This table has had no readers or writers for more than a year after it was migrated to the modern table.
Test Plan: Ran migration, verified that all the data was still around.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19056
Summary: See PHI356. Adds inline comment and done counts to the filetree. Also makes the filetree wider by default.
Test Plan: Fiddled with filetrees in different browsers on different revisions. Added inlines, marked them done/undone.
Differential Revision: https://secure.phabricator.com/D19041
Summary: See PHI356. Makes it easier to pick out change types in the filetree view in Differential.
Test Plan: Created a diff with adds, copies, moves, deletions, and binary files. Viewed in Differential, had an easier time picking stuff out.
Differential Revision: https://secure.phabricator.com/D19040
Summary:
Depends on D19021. Ref T13053. When you "Subscribe", or make some other types of edits, we don't necessarily have reviewer data, but may now need it to do the new recipient list logic.
I don't have a totally clean way to deal with this in the general case in mind, but just load it for now so that things don't fatal.
Test Plan: Subscribed to a revision with the "Subscribe" action.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19022
Summary:
Depends on D19019. Ref T13053. Fixes T12689. See PHI178.
Currently, if `@alice` resigns from a revision but `#alice-fan-club` is still a subscriber or reviewer, she'll continue to get mail. This is undesirable.
When users are associated with an object but have explicitly disengaged in an individal role (currently, only resign in audit/differential) mark them "unexpandable", so that they can no longer be included through implicit membership in a group (a project or package).
`@alice` can still get mail if she's a explicit recipient: as an author, owner, or if she adds herself back as a subscriber.
Test Plan:
- Added `@ducker` and `#users-named-ducker` as reviewers. Ducker got mail.
- Resigned as ducker, stopped getting future mail.
- Subscribed explicitly, got mail again.
- (Plus some `var_dump()` sanity checking in the internals.)
Reviewers: amckinley
Maniphest Tasks: T13053, T12689
Differential Revision: https://secure.phabricator.com/D19021
Summary:
Depends on D19012. Ref T13053. In D19012, I've changed "Thread-Topic" to always use PHIDs.
This change drops the selective on-object storage we have to track the original, human-readable title for objects.
Even if we end up backing out the "Thread-Topic" change, we'd be better off storing this in a table in the Mail app which just has `<objectPHID, first subject we used when sending mail for that object>`, since then we get the right behavior without needing every object to have this separate field.
Test Plan: Grepped for `original`, `originalName`, `originalTitle`, etc.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19013
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.
I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.
In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.
Then allow this header through for "Must Encrypt" mail.
Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19012
Summary:
Ref T13053. Adds revision stamps (status, reviewers, etc). Adds Herald rule stamps, like the existing X-Herald-Rules header.
Removes the "self" stamps, since you can just write a rule against `whatever(@epriestley)` equivalently. If there's routing logic around this, it can live in the routing layer. This avoids tons of self-actor, self-mention, self-reviewer, self-blocking-reviewer, self-resigned-reviewer, etc., stamps.
Use `natcasesort()` instead of `sort()` so that numeric values (like monograms) sort `9, 80, 700` instead of `700, 80, 9`.
Remove the commas from rendering since they don't really add anything.
Test Plan: Edited tasks and revisions, looked at mail stamps, saw stamps that looked pretty reasonable (with no more self stuff, no more commas, sorting numbers, and Herald stamps).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D18997
Summary:
Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision.
When you do, the corresponding object will get a more-visible cross-reference marker in its timeline:
{F5405517}
From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges.
Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13057
Differential Revision: https://secure.phabricator.com/D18978
Summary:
Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it.
The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront.
Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18946
Summary:
See PHI307. Currently, when reviews undraft, we retroactively add in older activity to the mail ("alice created this revision...").
However, we don't add that activity to the mail tags, so the relevant tags (like "revision created") are dropped forever.
Instead, use the same set of transactions for both mail body and mail tag construction.
This should be obsoleted in the relatively near future by T10448, but it's a better/more correct behavior in general and we probably can't get rid of tags completely for a while.
Test Plan:
Applied patch, created a revision with builds, saw it auto-undraft after builds finished. Used `bin/mail list-outbound` and `bin/mail show-outbound` to see the mail. Verified that it included retroactive text ("created this revision") AND retroactive tags.
Note that the tag for "A new revision is created" is `DifferentialTransaction::MAILTAG_REVIEW_REQUEST` with literal value `differential-review-request`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18941
Summary: See PHI280. We have a similar field for tasks already, this is generally a reasonable sort of thing to support, and the addition of "draft" states means there are some pretty reasonable use cases.
Test Plan:
- Wrote a status-based ("status is needs revision") Herald rule.
- Tested it against a "Needs Revision" revision (passed) and a "Changes Planned" revision (failed).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18938
Summary:
Ref T13047. In some reasonable cases, knowing the path and line number where a symbol appears is useful in ranking or filtering the set of matching symbols.
Giving symbol sources more information can't hurt, and it's generally free for us to include this context since we just need to grab it out of the document and pass it along.
We can't always get this data (for example, if a user types `s idx` into global search, we have no clue) but this is similar to other types of context which are only available sometimes (like which repository a symbol appears in).
Test Plan: Command-clicked some symbols in 1-up (unified) and 2-up (side-by-side) diff views with symbol indexes configured. Got accurate path and line information in the URI I was redirected to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18936
Summary:
Depends on D18924. Ref T13048. Each adapter defines which repetition options ("every time", "only the first time") users may select for rules.
Currently, this is all explicit and hard-coded. However, every adapter really just implements this rule (except for some bugs, see below):
> You can pick "only the first time" if this adapter fires more than once on the same object.
Since we already have a `isSingleEventAdapter()` method which lets us tell if an adapter fires more than once, just write this rule in the base class and delete all the copy/pasting.
This also fixes two bugs because of the copy/pasting: Pholio Mocks and Phriction Documents did not allow you to write "only the first time" rules. There's no reason for this, they just didn't copy/paste enough methods when they were implemented.
This will make a future diff (which introduces an "if the rule did not match last time" policy) cleaner.
Test Plan:
- Checked several different types of rules, saw appropriate options in the dropdown (pre-commit: no options; tasks: first or every).
- Checked mocks and wiki docs, saw that you can now write "only the first time" rules.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18925
Summary:
See PHI309. There is a window of time between when all builds pass and when Harbormaster actually publishes a revision out of draft.
If any other user tries to interact with the revision during that window, they'll pick up the undraft transaction as a side effect. However, they won't have permission to apply it and will be stopped by a validation error.
Instead, only automatically publish a revision if the actor is the revision author or some system/application user (essentially always Harbormaster).
Test Plan:
- Added a `echo ...; sleep(30);` to `HarbormasterBuildEngine->updateBuildable()` before the `applyTransactions()` at the bottom.
- Wrote an "Always, run an HTTP request" Herald rule and Harbormaster build plan.
- Ran daemons with `bin/phd debug task`.
- Created a new revision with `arc diff`, as user A.
- Waited for `phd` to enter the race window.
- In a separate browser, as user B, submitted a comment via `differential.revision.edit`.
- Before patch: edits during the race window were rejected with a validation error, "you don't have permission to request review".
- After patch: edits go through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18921
Summary:
Ref T13045. See that task for discussion.
This replaces `digestForIndex()` with a "clever" algorithm in `digestForAnchor()`. The new digest is the same as `digestForIndex()` except when the original output was "." or "_". In those cases, a replacement character is selected based on entropy accumulated by the digest function as it iterates through the string.
Test Plan: Added unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13045
Differential Revision: https://secure.phabricator.com/D18909
Summary: See PHI292. This is just a generalization of D18851: feed stories have the same issue as mail. Don't hide "requested a review" in either mail or feed.
Test Plan:
- Enable prototypes.
- No harbormaster builds.
- Create a revision.
- Pre-patch: no feed story.
- Post-patch: feed story.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18886
Summary:
Ref T13042. This is a very, very old policy-violating option from yesteryear which supported build systems publishing updates by adding comments to revisions, without sending email about it.
Harbormaster has served this role for a long time and this is policy-violating in the general case (it allows attackers to act in secret).
Test Plan: Grepped for affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13042
Differential Revision: https://secure.phabricator.com/D18881
Summary: Ref T13035. See that task for a description of the issue.
Test Plan:
- Enabled prototypes.
- Disabled all Herald rules that trigger Harbormaster builds.
- Created a new revision.
- Before patch: initial review request email was dropped.
- After patch: initial review request email is sent.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13035
Differential Revision: https://secure.phabricator.com/D18851
Summary:
See PHI230. Currently, we denormalize raw line counts onto diffs and revisions, but not added/removed line counts.
I'd like to try a `[---+ ]` sort of size hint element (see D16322 for more) as a general approach to conveying size information at a glance and see how it feels, since I think the raw size number isn't very scannable/useful and it may be a significant improvement to hint about how much of a change is throwing stuff out vs adding new stuff.
This just makes the data available without any subquerying and doesn't actually change the UI.
Test Plan:
Created a revision, saw detailed change information populate in the database.
```
mysql> select * from differential_revision where id = 292\G
*************************** 1. row ***************************
id: 292
title: WIP
originalTitle: WIP
phid: PHID-DREV-ux3cxptibn3l5pxsug3z
status: draft
summary: asdf
testPlan: asdf
authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
lastReviewerPHID: NULL
lineCount: 41
dateCreated: 1513179418
dateModified: 1513179418
attached: []
mailKey: h4mn6perdio47o4beomyvu75zezwvredx3mbrlgz
branchName: NULL
viewPolicy: users
editPolicy: users
repositoryPHID: PHID-REPO-wif5lutk5gn3y6ursk4p
properties: {"lines.added":40,"lines.removed":1}
activeDiffPHID: PHID-DIFF-ixjphpunpkenqgukpmce
1 row in set (0.00 sec)
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18832
Summary:
Fixes T13027. Ref T2543. When revisions promote from "Draft" because builds finish or no builds are configured, the status currently switches from "Draft" to "Needs Review" without re-running Herald.
This means that some rules -- notably, "Send me an email" rules -- don't fire as soon as they should.
Instead of applying this promotion in a hacky way inline, queue it and apply it normally in a second edit, after the current group finishes.
Test Plan:
- Created a revision, reviewed Herald transcripts.
- Saw three Herald passes:
- First pass (revision creation) triggered builds and no email.
- Second pass (builds finished) did not trigger builds (no update) and did not trigger email (revision still a draft).
- Third pass (after promotion out of 'draft') did not trigger builds (no update) but did trigger email (revision no longer a draft).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13027, T2543
Differential Revision: https://secure.phabricator.com/D18819
Summary:
Ref T10233. See PHI231. When users ignore the `arc land` prompt about bad revision states, make it explicitly clear in the transaction log that they broke the rules.
You can currently figure this out by noticing that there's no "This revision is accepted and ready to land." message, but it's unrealistic to expect non-expert users to look for the //absence// of a message to indicate something, and this state change is often relevant.
Test Plan: {F5302351}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10233
Differential Revision: https://secure.phabricator.com/D18808
Summary:
Depends on D18801. Ref T2543. See PHI229. I missed "Accept" before, but intended to disallow it (like "Reject") since I don't want drafts to be reviewable.
However, "Resign" seems fine to allow? So let's allow that for now.
Test Plan: Was no longer offered "Accept" on draft revisions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18802
Summary:
See PHI228. Ref T2543. The current logic gets this slightly wrong: prototypes are off, you create a draft with `--draft`, then promote it with "Request Review". This misses both branches.
Instead, test these conditions a little more broadly. We also need to store broadcast state since `getIsNewObject()` isn't good enough with this workflow.
Test Plan:
- With prototypes on and autopromotion, got a rich email after builds finished.
- With prototypes off, got a rich email immediately.
- With prototypes off and `--draft`, got a rich email after "Request Review".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18801
Summary: Whitespace has semantic meaning for yaml files, so we shouldn't suppress whitespace-only lines of diff by default.
Test Plan: Edited local config to include yaml files, saw expected whitespace changes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18775
Summary:
See PHI180. Currently, if you begin creating or editing an inline and then swap display modes (for example, with "View Unified"), your edit is lost.
Persisting the editor state is complicated and this is very rare, so just prevent the action and warn the user instead.
Also make the warning persist for a little longer since a few of the messages, including this one, take a couple seconds to read now.
Test Plan:
- Edited a comment, tried to swap display modes, got a warning.
- Swapped display modes normally with no comment being edited.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18774
Summary:
Depends on D18771. See PHI206. Currently, `arc diff --draft` only holds revisions in draft mode: it doesn't put them into draft mode if the install isn't configured to use draft mode.
Instead, make it a bit more forceful so that `arc diff --draft` can create into draft mode explicitly even if protoypes are off. This aligns with expection a little more clearly.
Test Plan: Ran `arc diff --draft` with prototypes off, got a revision held in draft mode.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18772
Summary: See PHI210. Ref T2543. Currently, we don't set this flag if you have prototypes off and don't get any of the new draft stuff, so the mail drops some of the details it is supposed to have.
Test Plan: Disabled prototypes, created a revision, saw summary / test plan in the initial mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18771
Summary:
See PHI199. Ref T2543. When you run a RevisionQuery with a legacy status constraint (via `differential.query`), we currently don't match "Draft" revisions.
Use the actual complete map from `DifferentialRevisionStatus` instead of hard coding the status list so "Draft" is included.
Test Plan:
- Ran `differential.query` with `ids` and `status` for a draft revision.
- Before patch: revision not returned in results.
- After patch: revision returned in results.
(Note that it returns as "Needs Review", for compatibility.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18765
Summary:
See PHI190. This clarifies the ruleset a bit:
- If you accepted, then the author used "Request Review" explicitly, we now show "Accepted Earlier" instead of "Accepted" in the "Reviewers" list on the main revision page. This makes it sligthly more clear why the revision is back in your review queue without picking through the transaction log.
- Instead of moving all non-current accepts into "Ready to Review", move only voided accepts into "Ready to Review". This stops us from pulling older accepts which haven't been voided (which could have been incorrectly pulled) and correctly pulls older, voided accepts from before an update (for example: accept, then request review, then update) and generally aligns better with intent/expectation.
Test Plan:
- Accepted, requested review.
- Saw reviewer as "Accepted Earlier".
- Saw review in "Ready to Review" bucket.
- Accepted, updated (with sticky accept).
- Saw reviewer as "Accepted Prior Diff".
- Saw review as "Waiting on Authors".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18764
Summary: See PHI195. This bulks out these API methods since all the requests are pretty straightforward.
Test Plan: Ran `edge.search` and `differential.revision.search`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18762
Summary:
See PHI197. Populates "status" transactions and a few other obvious types where there's no security/performance/payload/formatting issue I can come up with.
The names here are the same as the names for editing with `maniphest.edit`.
Test Plan: Used `transaction.search` to retrieve transactions of all new types.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18761
Summary: Ref T12689. See PHI178. This isn't a complete solution (you may still get mailed via packages/projects) but should fix the obvious issue, where "Resigned" reviewers are incorrectly always sent mail directly.
Test Plan: Had Alice resign, interacted as Bailey, no mail to Alice.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12689
Differential Revision: https://secure.phabricator.com/D18758
Summary:
Depends on D18756. Fixes T12539. See PHI190. Currently, when this occurs:
- Alice accepts.
- Bailey requests review.
- Alice views her dashboard.
...the revision appears in "Waiting on Other Reviewers" (regardless of whether other reviewers actually exist or not).
Instead, ignore these voided/non-current accepts and let the revisions appear in "Ready to Review", which is more natural.
Test Plan: Went through the steps above. On `master`, saw revision in "Waiting on Other Reviewers". After patch, saw it in "Ready to Review".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12539
Differential Revision: https://secure.phabricator.com/D18757
Summary:
Ref T12539. See PHI190. Currently, each Diff has a `revisionID`, but Revisions do not point at the current active diff. To find the active diff for a given revision, we need to issue a separate query.
Furthermore, this query is inefficient for bulk loads: if we have a lot of revisions, we end up querying for all diff IDs for all those revisions first, then selecting the largest ones and querying again to get the actual diff objects. This strategy could likely be optimized but the query is a mess in any case.
In several cases, it's useful to have the active diff PHID without needing to do a second query -- sometimes for convenience, and sometimes for performance.
T12539 is an example of such a case: it would be nice to refine the bucketing logic (which only depends on active diff PHIDs), but it feels bad to make the page heavier to do it.
For now, this is unused. I'll start using it to fix the bucketing issue, and then we can expand it gradually to address other performance/convenience issues.
Test Plan:
- Ran migrations, inspected database, saw sensible values.
- Created a new revision, saw a sensible database value.
- Updated an existing revision, saw database update properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12539
Differential Revision: https://secure.phabricator.com/D18756
Summary:
Ref T2543. Fixes two relatively minor things:
- When builds finish in Harbormaster, send mail "From" the author.
- Set the `firstBroadcast` flag so that initial mail picks up earlier history (notably, the "reviewers" line).
For now, I'm not setting `firstBroadcast` on explicit "Request Review" (but maybe we should), and not trying to deal with weird cases where you leave a bunch of comments on a draft. Those might be fine as-is or may get tweaked later.
Test Plan: Created a revision with Harbormaster builds, ran builds, saw initial email come "From" the right user with more metadata.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18748
Summary: Ref T2543. After D18731, Herald build rules run more often, but now incorrectly try to run builds when Diffusion closes a revision because a commit landed.
Test Plan: Made some mundane updates locally; this is tricky to test comprehensively locally so I'm mostly planning to just push it to `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18745
Summary:
Ref T2543. Instead of autosubmitting revisions to "Needs Review" when builds finish, allow them to be held in "Draft" indefinitely.
There's currently no UI for this. I plan to just expose it as `arc diff --draft` for now, in a followup change.
Test Plan:
- Created a revision (via Conduit) with "hold as draft", saw it hold as draft after builds finished.
- Created a revision (normally), saw it autosubmit after builds finished.
- Requested review of a "hold as draft" revision to kick it out of draft state.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18737
Summary:
Depends on D18740. Prepares `arc` to receive a `--draft` flag by letting us switch to "differential.revision.edit" instead of "differential.createrevision".
To "differential.revision.edit", we need a transaction list, but we can't automatically construct this list from a field map. Return the transaction list alongside the field map.
The next change uses this list (if available) to switch us to the modern API method.
Test Plan: Ran `arc diff` on the experiemntal branch with followup changes, got a new revision.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18741
Summary: See PHI90. For now, this only provides a limited amount of information, but should satisfy the use case in PHI90 and build toward a more complete version in the future.
Test Plan: Used new Conduit method to retrieve information about diffs.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18744
Summary:
Ref T2543. Fixes T10109.
Currently, Herald only runs in Differential when a change updates the diff. This is partly for historical reasons, and partly because we don't want to restart builds every time someone makes a comment. However, this behavior is inconsistent with other applications (which always trigger on any change), and occasionally confusing to users (in T10109, for example) or otherwise undesirable.
A similar issue is that T2543 has introduced a "Draft" state, where revisions don't send normal mail until builds finish. This interacts poorly with "Send me an email" rules (which shouldn't do anything here) and particularly with "Send me an email + only run these actions the first time the rule matches", since that might have an effect like "do nothing when the revision is created, then never anything again since you already did nothing once".
To navigate both of these issues, let objects tell Herald that certain actions (like mail or builds) are currently forbidden. If a rule uses a field or action which is currently forbidden, the whole rule automatically fails before it executes, but doesn't count toward "only the first time" as far as Herald's tracking of rule execution is concerned.
Then, forbid mail for draft revisions, and forbid builds for revisions which didn't just get updated. Forbidding mail fixes the issues with "Send me an email" that were created by the introduction of the draft state.
Finally, make Herald run on every revision update, not just substantive updates to the diff. This resolves T10109.
Test Plan:
Created revisions via the draft -> submit workflow, saw different transcripts. Here's a mail action being forbidden for a draft:
{F5237324}
Here's a build action being forbidden for a "mundane" update:
{F5237326}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T10109, T2543
Differential Revision: https://secure.phabricator.com/D18731
Summary:
Ref T2543. When Harbormaster finishes builds and promotes a draft revision to review, we currently publish "Harbormaster requested review of...".
Instead, attribute this action to the author, since that's more natural and more useful.
Test Plan: Promoted a diff locally, saw it attributed to me rather than Harbormaster.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18730
Summary: Ref T12680. Use the slightly sleeker construction from D18722 in Differential.
Test Plan: Viewed revision list, reordered by date modified.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12680
Differential Revision: https://secure.phabricator.com/D18727
Summary:
Ref T2543. When a revision is created, we check if any builds are waiting/failed, and submit it for review immediately if we aren't waiting for anything.
In doing this, we ignore builds with only autotargets, since these are client-side and failures from local `arc lint` / `arc unit` should not count (the user has already chosen to ignore/skip them).
The way we do this has some issues:
- Herald may have started builds, but they may still be PENDING and not have any targets yet. In this case, we'll see "no non-autotargets" and ignore the build, which is wrong.
- We have to load targets but don't really care about them, which is more work than we really need to do.
- And it's kind of complex, too.
Instead, just let `BuildQuery` filter out "autobuilds" (builds generated from autoplans) with a JOIN.
Test Plan: Ran `arc diff` with builds configured, got a clean "Draft" state instead of an incorrect promotion directly to "Needs Review".
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18721
Summary: Ref T2543. When revisions are in the draft state, tell the user what we're waiting for or why they aren't moving forward.
Test Plan: {F5228840}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18714
Summary: Ref T2543. This is a less ambitious version of the rule in D18628, which I backed off from, since I think this probably still has a fair number of loose ends to tie up.
Test Plan: Created a revision locally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18713
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
Ref T13000. This marks each table as either "data" (normal data), "cache" (automatically rebuilt, no need to ever dump) or "index" (can be manually rebuilt).
By default, `bin/storage dump` dumps data and index tables, but not cache tables.
With `--no-indexes`, it dumps only data tables. Indexes can be rebuilt after a restore with `bin/search index --all ...`.
Test Plan:
- Ran `--no-indexes` and normal dumps with `--trace`, verified that cache and index (former case) or cache only (latter case) tables were dumped with `--no-data`.
- Verified dump has the same number of `CREATE TABLE` statements as before the changes.
- Reviewed persistence tags in the web UI (note Ferret engine tables are "Index"):
{F5210886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18682
Summary:
Ref PHI91. When Owners (or Herald, or manual user action) adds package reviewers to a revision, later updates to the revision make some of them less relevant or irrelevant.
Provide a hint when a package reviewer doesn't own any of the paths that a diff changes. Humans can then decide if the reviewer is obsolete/irrelevant or not.
This is a rough cut to get the feature working, design could probably use some tweaking if it sticks.
Test Plan: {F5204309}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jboning
Differential Revision: https://secure.phabricator.com/D18663
Summary:
Ref T2543. This doesn't stand alone since mail still goes out normally, but gets this piece working: new revisions start as "Draft", then after updates if there are no builds they go into "Needs Review".
This should work in general because builds update revisions when they complete, to publish a "Harbormaster finished build yada yada" transaction. So either we'll un-draft immediately, or un-draft after the last build finishes.
I'll hold this until the mail and some other stuff (like UI hints) are in slightly better shape since I think it's probably too rough on its own.
Test Plan: Created revisions locally, saw them un-draft after builds.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18628
Summary:
Ref T2543. Currently, we always do some special things when a revision is created, mostly adding more stuff to the mail.
With drafts, we want to suppress initial mail and send this big, rich mail only when the revision actually moves out of "draft".
Prepare the code for this, with the actual methods hard-coded to the current behavior. This will probably take some tweaking but I think I got most of it.
Test Plan: Banged around in Differential so it sent some mail, saw normal mail without anything new.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18627
