Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary:
Fixes T11836. See some prior discussion in T8376#120613.
The policy hint in headers in the UI is not exhaustive, and can not reasonably be exhaustive. For example, on a revision, it may say "All Users", but really mean "All users who can see the space this object is in and the repository it belongs to, plus the revision author and reviewers".
These rules are explained if you click (and, often, in the documentation), but "All Users" is still at least somewhat misleading.
I don't think there's any perfect solution here that balances the needs of both new and experienced users perfectly, but this change tries to do a bit better about avoiding cases where we say something very open (like "All Users") when the real policy is not very open.
Specifically, I've made these changes to the header:
- Spaces are now listed in the tag, so it will say `(S3 > All Users)` instead of `(All Users)`. They're already listed in the header, this just makes it more explicit that Spaces are a policy container and part of the view policy.
- Extended policy objects are now listed in the tag, so it will say `(S3 > rARC > All Users)` for a revision in the Arcanist repository which is also in Space 3.
- Objects can now provide a "Policy Codex", which is an object that represents a rulebook of more sophisticated policy descriptions. This codex can replace the tag with something else.
- Imported calendar events now say "Uses Import Policy" instead of, e.g., "All Users".
I've made these changes to the policy dialog:
- Split it into more visually separate sections.
- Added an explicit section for extended policies ("You must also have access to these other objects: ...").
- Broken the object policy rules into a "Special Rules" section (for rules like "you can only see a revision if you can see the repository it is part of") and an "Object Policy" section (for the actual object policy).
- Tried to make it a little more readable?
- The new policy dialogs are great to curl up with in front of a fire with a nice cup of cocoa.
I've made these changes to infrastructure:
- Implementing `PhabricatorPolicyInterface` no longer requires you to implement `describeAutomaticCapability()`.
- Instead, implement `PhabricatorPolicyCodexInterface` and return a `PhabricatorPolicyCodex` object.
- This "codex" is a policy rulebook which can set all the policy icons, labels, colors, rules, etc., to properly explain complex policies.
- Broadly, the old method was usually either not useful (most objects have no special rules) or not powerful enough (objects with special rules often need to do more in order to explain them).
Test Plan:
{F1912860}
{F1912861}
{F1912862}
{F1912863}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T11836
Differential Revision: https://secure.phabricator.com/D16830
Summary:
Fixes T11748. This option currently implies a line limit (e.g., inline patches that are less than 100 lines long). This breaks down if a diff has a 10MB line, like a huge blob of JSON all on one line.
For now, imply a reasonable byte limit (256 bytes per line).
See T11767 for future work to make this and related options more cohesive.
Test Plan:
- With option at `1000`: sent Differential email, saw patches inlined.
- With option at `10`: sent Differential email, saw patches dropped because of the byte limit.
- `var_dump()`'d the actual limits and used `bin/worker execute --id ...` to sanity check that things were working properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11748
Differential Revision: https://secure.phabricator.com/D16714
Summary:
Ref T11650. Currently, we load packages and then discard the archived ones.
However, this gets "dominion" rules (where a more-general package gives up ownership if a more-specific package exists) wrong if the more-specific package is archived: we incorrectly give up ownership.
Instead, just ignore these packages completely when loading affected packages. This is slightly simpler.
(There are technically two pieces of code we have to do this for, which should be a single piece of code but which haven't yet been unified.)
Test Plan:
- Created packages:
- Package A, on "/" (strong dominion, autoreview).
- Package B, on "/x/" (weak dominion, autoreview).
- Package C, on "/x/y" (archived, autoreview).
- Create a revision affecting "/x/y".
- Saw correct path ownership in table of contents ("B", strongest package only).
- Saw correct autoreview behavior (A + B).
- (Prior to patch, in `master`, reproduced the problem behaviors described in T11650, with bad dominion rules and failure to autoreview B.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11650
Differential Revision: https://secure.phabricator.com/D16564
Summary:
Fixes T11610. Clean up some sketchy old code from long ago.
If you had rules that use conditions like "Accepted revision exists" and ran them in the test console, we'd never load the "CommitData" and fatal.
Instead, load CommitData in `newTestAdapter()` and generally make these pathways a little more modern.
Test Plan:
- Wrote an "Accepted Revision Exists" rule.
- Ran a commit in the test console.
- Before patch, got fatal from T11610.
- After patch, got clean test result.
- Also pushed a commit and reviewed the transcript to make sure the rule ran properly.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T11610
Differential Revision: https://secure.phabricator.com/D16522
Summary:
This is hacky, and I'm not sure I'm happy with it; Until T9365 is done, this will show up
broken tests with an appropriate star in the Revision History.
Test Plan: Created 1M messages in a couple of old diffs in a revision. The query took ~80us (On SSD drive).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16483
Summary:
Fixes T10423. Ref T11524. This changes `diffusion.rawdiffquery` to return a file PHID instead of a blob of data.
This is better in general, but particularly better for huge diffs (as in T10423) and diffs with non-utf8 data (as in T10423).
Test Plan:
- Used `bin/differential extract` to extract a latin1 diff, got a clean diff.
- Used `bin/repository reparse --herald` to rerun herald on a latin1 diff, got a clean result.
- Pushed latin1 diffs to test commit hooks.
- Triggered the the too large / too slow logic.
- Viewed latin1 diffs in Diffusion.
- Used "blame past this change" in Diffusion to hit the `before` logic.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T10423, T11524
Differential Revision: https://secure.phabricator.com/D16460
Summary: Ref T11132. This is a new default default (no dashboard) homepage. It offers (Diffs) (Tasks) (Repositories) in the main column and (Feed) in the side column. No NUX stuff, No logged out public view (upcoming diff). This should be complete, but unclear how to bucketize Differential.
Test Plan: Test new account's default homepage.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16449
Summary: This needs an `isset()` for cases when authority and packages don't completely overlap.
Test Plan:
- With a package set to trigger autoreview, created a revision.
- Observed error log, saw no more error.
- Saw package trigger autoreview properly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16398
Summary:
Fixes T9719. Currently, the Herald "Test Console" has a big `instanceof` thing, so new adapters (like a Calendar adapter, or third-party adapters) aren't available automatically. Instead, do a standard modular thing: load the available adapters, ask which ones can test the object the user selected, then let the user pick which one they want to move forward with.
Additionally, it isn't very clear that you can't test "commit hook" rules because they rely on push state which we don't really have a good way to simulate. When the user picks a commit, we now show them the "Hook" events, but the options are disabled and explain why they can not be selected.
Test Plan:
- Ran test rules for revisions, commits, mocks, tasks, wiki documents, questions, and outbound mail.
- Plugged in a commit, got a more-helpful choice screen explaining why you do a test run of hook rules.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9719
Differential Revision: https://secure.phabricator.com/D16360
Summary:
Ref T11404. Depends on D16351. Currently, both `differential.query` and `differential.revision.search` issue `2N` queries to fetch:
- dependencies for each revision; and
- projects for each revision.
Fix this:
- Take these custom fields out of Conduit so they don't load this data by default.
- For `differential.query`, put this data back in by hard coding it.
- For `differential.revision.search`, just leave it out. You can already optionally get projects efficiently, and this endpoint is a work in progress. I would tentatively be inclined to expose graph data as a "graph" extension once we need it.
This makes both methods execute in `O(1)` time (which is still 20-30 queries, but at least it's not 320 queries anymore).
Test Plan:
- Ran `differential.query`, observed no change in results but 199 fewer internal queries.
- Ran `differential.revision.search`, observed data gone from results and 200 fewer internal queries.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16352
Summary:
Ref T11404. Depends on D16350.
Currently, custom fields can issue "N+1" queries in some cases, so querying 100 revisions issues 100 extra queries.
This affects all `*.search` endpoints for objects with custom fields, and some older endpoints (notably `differential.query`).
This change bulk loads "normal" custom fields, which gets rid of some of these queries. Instead of loading fields for each object, we build a big list of all fields and load them all at once.
The next change will tackle the remaining inefficient edge queries.
Test Plan:
- Configured a custom field with normal database storage in Differential.
- Ran `differential.query`, looking at custom fields in results for correctness.
- Ran `differential.revision.search`, looking at custom fields in results for correctness.
- In both cases, observed queries drop from `3N` to `2N` (all the "normal" custom field stuff got bulk loaded).
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16351
Summary: Ref T10909. Ref T9224. We label this field "Host" in the UI; make the storage format consistent.
Test Plan:
- Viewed month view, day view, detail view of an event.
- Created a new event, saw myself as the host.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9224, T10909
Differential Revision: https://secure.phabricator.com/D16291
Summary: Fixes T11305, Ref T7754. Makes this menu dropdown act like actions and collapse to a fa-bars menu.
Test Plan:
View on mobile, desktop, browser. Click an action, spawn new page.
{F1717953}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7754, T11305
Differential Revision: https://secure.phabricator.com/D16265
Summary:
Ref T4788. As it turns out, our tasks are very tightly connected.
Instead of loading every parent/child task, then every parent/child of those tasks, etc., etc., only load tasks in the "same direction" that we're already heading.
For example, we load children of children, but not parents of children. And we load parents of parents, but not children of parents.
Basically we only go "up" and "down" now, but not "out" as much. This should reduce the gigantic multiple-thousand-node graphs currently shown in the UI.
I still discover the whole graph for revisiosn, because I think it's probably more useful and always much smaller. That might need adjustment too, though.
Test Plan: Seems fine locally??
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16218
Summary:
Ref T4788. This separates the revision graph view into a base class with core logic and a revision class with Differential-specific logic, so I can subclass it in Maniphest, etc., and try using it in other applications to show similar graphs.
Not sure if we'll stick with it, but even if we don't this makes the code a bit cleaner and gets custom rendering logic out of the RevisionViewController, which is nice.
Test Plan: Viewed revisions, saw the stack UI completely unchanged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16213
Summary: Ref T10628. Turn these into tabs in a single box, since "local commits" and "similar revisions" are of particularly rare use.
Test Plan: {F1707196}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10628
Differential Revision: https://secure.phabricator.com/D16209
Summary:
Ref T10628. Switch this to be nicer and more modern.
- When there's only one tab, add an option to hide it.
Test Plan:
- Viewed normal revisions (no tabs).
- Viewed X vs Y revisions (two tabs, rightmost tab selected by default).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10628
Differential Revision: https://secure.phabricator.com/D16206
Summary: Fixes T11240. Also simplify things a little and share a bit more code.
Test Plan:
- Viewed revisions and tasks, opened submenu.
- Viewed as a user without edit permission, saw the menus greyed out.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11240
Differential Revision: https://secure.phabricator.com/D16201
Summary: Ref T4788. This moves everything except "merge" to the new code.
Test Plan:
- Edited relationships in Differential, Diffusion, and Pholio.
- Uninstalled Pholio, made sure "Edit Mocks..." actions vanished.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16193
Summary:
Ref T9838.
Add a Properties field to Revision, and update a `wasAcceptedBeforeClose` when closing a revision.
Test Plan:
A quick run through the obvious steps (Close with commit/manually, with or w/o accept) and calling `differential.query` shows the `wasAcceptedBeforeClose` property was setup correctly.
Pushing closed + accepted passes the relevant herald, which was my immediate issue; Pushing un-accepted is blocked.
Test the "commit" rule (Different from "pre-commit") by hacking the DB and running the "has accepted revision" rule in a test-console.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T9838
Differential Revision: https://secure.phabricator.com/D15085
Summary:
Ref T11179. This generates the Maniphest menu items in a modular way. It doesn't change any of the underlying code yet.
Searching for commits doesn't work particularly well so I've just hidden that for now, but the item itself works fine.
Test Plan: {F1696849}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16162
Summary:
Ref T11035. This only fixes half of the issue: comment editing has been fixed, but normal transactions which edit things like descriptions haven't yet.
The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order.
Test Plan:
- Mentioned `@dog` in a comment.
- Removed `@dog` as a subscriber.
- Edited the comment, adding some unrelated text at the end (e.g., fixing a typo).
- Before change: `@dog` re-added as subscriber.
- After change: no re-add.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11035
Differential Revision: https://secure.phabricator.com/D16108
Summary:
When having lots of repos, seeing "all revisions in this project" is hard, and we ended up adding herald rules to basically copy project tags to the revisions on a per-project basis. Adding a "tagged: project" function to the Repositories search field allows users to find differentials within a project.
Fix T10850.
Test Plan: search differentials by tagging project and repository in the Repository field
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: avivey, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10850
Differential Revision: https://secure.phabricator.com/D16096
Summary: Ref T11123. This implements a very basic skeleton for modern revision search.
Test Plan: Viewed and executed Conduit API method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11123
Differential Revision: https://secure.phabricator.com/D16089
Summary:
Ref T10856. The rendering logic was already there, but it was expecting the information under `properties`
field, whereas arc puts it under `metadata`. Not sure if that something that changed a long time ago or if
it was always like this.
Test Plan: {F1252657 size=full}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T10856
Differential Revision: https://secure.phabricator.com/D15828
Summary: Ref T4103. Fully modernize the filetree show/hide, durable column show/hide, and profile menu collapse/wide settings.
Test Plan:
- Toggled filetree on/off, reloaded page, setting stuck.
- Same with conpherence column and profile menus.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16034
Summary: Change metamta.differential.patch-format over to an enum option now that they're implemented.
Test Plan: Looked at settings page.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16032
Summary:
Ref T11092. With Quicksand (or, possibly, some as-yet-unknown non-Quicksand workflow) the client can get stuck with an out-of-date revision PHID.
We then save comments with a `revisionPHID` from one revision and a `changesetID` from a different one.
Detect and prevent this. This stops the workflow immediately when the use first clicks, so it should allow us to detect this issue if it has some other non-Quicksand cause.
Test Plan:
- Opened revision `D123`.
- Pressed `\` to enable the sidebar and Quicksand.
- Clicked a link to revision `D124`.
- Added inlines.
Previously, these could ghost. The exact UI behavior is difficult to describe, but in the database they end up with a `changesetID` for `D124` but the original `revisionPHID` for `D123`, presumably because state is sticking around from the first page.
After this patch, an exception is thrown immediately. Additionally:
- Reloaded to clear quicksand state, added comments fine.
- Disabled sidebar/quicksand, added comments fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11092
Differential Revision: https://secure.phabricator.com/D16031
Summary:
Ref T4103. This doesn't get everything, but takes care of most of the easy stuff.
The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next.
Test Plan:
- Grepped for `loadPreferences` to identify callsites.
- Changed start-of-week setting, loaded Calendar, saw correct start.
- Visited welcome page, read "Adjust Settings" point.
- Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence.
- Enabled Filetree, toggled in Differential.
- Disabled Filetree, no longer visible in Differential.
- Changed "Unified Diffs" preference to "Small Screens" vs "Always".
- Toggled filetree in Diffusion.
- Edited a task, saw sensible projects in policy dropdown.
- Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd.
- Toggled "monospaced textareas", used a comment box, got appropriate fonts.
- Toggled durable column.
- Disabled title glyphs.
- Changed monospaced font to 18px/36px impact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16004
Summary:
Ref T10769. The user availability cache write shouldn't happen in read-only mode, nor should the Differential parse cache write.
(We might want to turn off the availbility feature completely since it's potentially expensive if we can't cache it, but I think we're OK for now.)
Test Plan:
In read-only mode:
- Browsed as a user with an out-of-date availability cache.
- Loaded an older revision without cached parse data.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10769
Differential Revision: https://secure.phabricator.com/D15988
Summary:
Ref T11051. This is still not as clear as it should be, but is at least working as intended now.
I believe this part of the code just never worked. The test plan on D10489 didn't specifically cover it.
Test Plan:
Did this sort of thing in a repository:
```
$ git checkout -b featurex
$ echo x >> y
$ git commit -am wip
$ arc diff
```
Then I simulated just pushing it (this flow is a little more involved than necessary):
```
$ arc land --hold
$ git commit --amend
$ # remove all metadata -- particularly, "Differential Revision"!
$ git push HEAD:master
```
I got a not-great but more-useful dialog:
{F1667318}
Prior to this change, the hash match was incorrectly not reported at all.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11051
Differential Revision: https://secure.phabricator.com/D15989
Summary: Fixes T11010. This also needs to be inflated until we fix the whole client/server responsibility issue here.
Test Plan:
- Created a revision while observing error log, no error.
- Disabled "allow self accept", tried to make myself a reviewer, got rejected with an error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11010
Differential Revision: https://secure.phabricator.com/D15966
Summary:
Ref T10939. This makes the `viewer()` function work again. It retains its own meaning (viewer, plus all their projects and packages).
There's no `exact-viewer()` function; we could conceivably add one eventually if we need it.
Test Plan:
- Queried for `viewer()`, got the same results as querying by my own username.
- Browsed function in token browser.
- Reviewed autogenerated documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15951
Summary:
Ref T10939. If you haven't installed a dashboard, we show an "Active Revisions" panel on the homepage by default. I waited a bit to update this, but the new buckets don't seem to have caused any major problems so far.
Update this to use the new logic. I'm just showing "must review" + "should review", which is similar to the old beahvior.
Also replace the notification count with this same number. This is a little different from the old behavior, but simpler, and I think we should probably move toward getting rid of these counts completely.
Test Plan:
- Viewed homepage as logged-in user, saw my revisions (including revisions I have authority over only because of project membership).
- Saw consistent notification count.
- Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15950
Summary:
Ref T10694. If this feels good, I'd plan to eventually add something similar to other applications ("View Task", etc).
Not sure if we should keep the object link later in the mail body or not. I left it for now.
Test Plan: {F1307256, size=full}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15884
Fixes T10981. Ref T10939. `arc` currently has some odd, hard-coded checks
(missing reviewers, all reviewers away) that depend on the field value being
in a certain format.
The recent changes swapped the field value from scalars (PHIDs) to
dictionaries and broke this workflow. It worked fine in testing because we
apply these checks very inconsistently (not on update or `--edit`).
To get around this for now, serialize into "PHID!" and then unserialize on
the other side. This is icky but keeps us from needing to require an `arc`
upgrade.
These checks are generally bad news and should move to the server side in the
long run (T4631).
(This probably prevents clean `arc diff`, so I'm just cowboy committing it.)
Auditors: chad
Summary: Ref T10939. This adds UI, transactions, etc, to adjust dominion rules.
Test Plan:
- Read documentation.
- Changed dominion rules.
- Created packages on `/` ("A") and `/x` ("B") with "Auto Review: Review".
- Touched `/x`.
- Verified that A and B were added with strong dominion.
- Verified that only B was added when A was set to weak dominion.
- Viewed file in Diffusion, saw correct ownership with strong/weak dominion rules.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15936
Summary: Ref T10939. Fixes T4887. Supports "username!" to add a reviewer as blocking.
Test Plan: Added and removed blocking and non-blocking reviewers via CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4887, T10939
Differential Revision: https://secure.phabricator.com/D15934
Summary:
Ref T10939. Adds a `blocking(...)` token.
This code is pretty iffy and going to get worse before it gets better, but the fix (T10967 + EditEngine) is going to be a fair chunk of work down the road.
Test Plan: {F1426966}
Reviewers: chad
Reviewed By: chad
Subscribers: scode
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15933
Summary:
Ref T10939. I don't think this is hugely important, but it doesn't clutter things up much and it's nice as a hint.
T4055 was the original request specifically asking for this. It wanted a separate bucket, but I think this use case isn't common/strong enough to justify that.
I would like to improve Differential's "X depends on Y" feature in the long term. We don't tend to use/need it much, but it could easily do a better and more automatic job of supporting review of a group of revisions.
Test Plan: {F1426636}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15930
Summary: Ref T10939. These poor stragglers got left out in the rain. Didn't catch any issues otherwise.
Test Plan: {F1426604}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15929
Summary: Ref T10939. For various historical reasons, revision status is a numeric string. This comparison fails because it's `(string) !== (int)`. Just use `!=` so this will still work if we turn it into a real string in the future.
Test Plan: Tried a more specific test case locally, got better looking results in "Must Review" and "Should Review".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15928
Summary:
Ref T10939. I'm not //totally// opposed to the existence of this element, but I think it's the kind of thing that would never make it upstream today. I think this should just be a T418 custom sort of thing in the long run, not a mainline upstream feature.
Overall, I think this thing is nearly useless and just adds visual clutter. My dashboard is about 100% red. This also sort of teaches users that it's fine to let revisions sit for a couple of days, which isn't what I'd like the UI to teach. Finally, removing it helps the UI feel a little less cluttered after the visually busy changes in D15926.
Test Plan: Grepped for removed config. Viewed revision list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15927
Summary:
Ref T10939. Fixes T9263. Ref T4144.
First, this resolves users (converting users into all packages and projects they are responsible for) earlier, so bucketing can act on that data correctly. Previously, your own blocking reviews would appear in "Must Review" but your packages/projects' would not. Now, all of them will.
Second, this adds `exact(username)` to mean "just me, not my packages/projects". You can use this along with "Bucket: By Required Action" to create a personal view of "Active Revisions" if you'd like, and ignore all your project/package reviews.
Test Plan: Queried by "me" and "exact(me)", got reasonable looking results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T9263, T10939
Differential Revision: https://secure.phabricator.com/D15925
Summary:
Ref T10939. Ref T4144. This splits the existing buckets ("Blocking Others", "Action Required", "Waiting on Others") into 6-7 buckets with a stronger focus on what the next action you need to take is.
See T10939#175423 for some discussion.
Overall, I think some of the root problems here are caused by reviewer laziness and shotgun review workflows (where a ton of people get automatically added to everything, probably unnecessarily), but these buckets haven't been updated since the introduction of blocking reviewers or project/package reviewers and I think splitting the 3 buckets into 6 buckets isn't unreasonable, even though it's kind of a lot of buckets and the root problem here is approximately "I want to ignore a bunch of stuff on my dashboard".
I didn't remove the old bucketing code yet since it's still in use on the default homepage.
This also isn't quite right until I fix the tokenizer to work properly, since it won't bucket project/package reviewers accurately.
Test Plan: {F1395972}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T10939
Differential Revision: https://secure.phabricator.com/D15924
Summary:
Ref T10939. Currently, Differential hard-codes some behaviors for the "active" filter. This introduces "buckets" to make this grouping behavior more general/flexible.
The buckets don't actually do any grouping yet, this just gets rid of the `$query === 'active'` stuff so far.
These buckets change the page size to a large value, becuase pagination won't currently work with bucketing.
The problem is that we normally paginate by selecting one more result than we need: so if we're building a page of size 10, we'll select 11 results. This is fast, and if we get 11 back, we know there's a next page with at least one result on it.
With buckets, we can't do this, since our 11 results might come back in these buckets:
- A, B, C, A, C, C, A, A, B, B, (B)
So we know there are more results, and we know that bucket B has more results, but we have no clue if bucket A and bucket C have more results or not (or if there's anything in bucket D, etc).
We might need to select a thousand more results to get the first (D) or the next (A).
So we could render something like "Some buckets have more results, click here to go to the next page", but users would normally expect to be able to see "This specific bucket, A, has more results.", and we can't do that without a lot more work.
It doesn't really matter for revisions, because almost no one has 1K of them, but this may need to be resolved eventually.
(I have some OK-ish ideas for resolving it but nothing I'm particularly happy with.)
Test Plan: {F1376542}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15923
Summary:
Ref T10939. Ref T4144. This moves the revision SearchEngine to modern code so I can add some kind of bucketing layer on top of it.
This seems to have worked pretty cleanly. One thing is that I removed the ability to search for "pending drafts":
- This was added in D1927 from a bootcamp task, was an indirect solution to a questionable problem, and almost certainly would not meet the bar today.
- Later, in D3324, we added the icons to the list. I think this is a better solution in general. In particular, it specifically addressed the query being kind of junky.
- At the time, Differential had a prebuilt "Drafts" filter. This was removed in D6347 with the move to ApplicationSearch, which simplified the large number of prebuilt filters. Although we got a lot of feedback about that, none requested that the drafts filter be restored.
Test Plan: Searched for responsible users, subscribers, orders, projects, repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4144, T10939
Differential Revision: https://secure.phabricator.com/D15921
Summary:
Ref T10939. If you already own a package, don't trigger the subscribe/review rules.
Document how these rules work.
Test Plan:
- Read documentation.
- Removed reviewers, updated a revision, got autoreviewed.
- Joined package.
- Removed reveiwers, updated a revision, no more autoreview.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15918
Summary: Ref T10939. Packages are valid reviewers, so let Herald "Add Reviewers" and "Add Blocking Reviewers" actions add them.
Test Plan:
- Wrote a rule to add package reviewers.
- Hit the rule, saw a package reviewer added, viewed transcript.
{F1311731}
{F1311732}
{F1311733}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15917
Summary:
Ref T10939. Fixes T8887. This enables and implements the "review" and "blocking review" options for packages.
This is a bit copy-pastey from `DifferentialReviewersHeraldAction`, which doesn't feel awesome. I think the right fix is Glorious Infrasturcture, though -- I filed T10967 to track that.
Test Plan:
- Set package autoreveiw to "Review".
- Updated, got a reveiwer.
- Set autoreview to "blocking".
- Updated, got a blocking reviewer.
{F1311720}
{F1311721}
{F1311722}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15916
Summary:
Ref T10939. Ref T8887. This moves toward letting packages automatically become reviewers or blocking reviewers of owned code.
This change adds an "Auto Review" option to packages. Because adding reviewers/blocking reviewers is a little tricky, it doesn't actually have these options yet -- just a "subscribe" option. I'll do the reviewer work in the next update.
Test Plan:
Created a revision in a package with "Auto Review: Subscribe to Changes". The package got subscribed.
{F1311677}
{F1311678}
{F1311679}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15915
Summary:
Ref T10939. This lets you add packages as reviewers manually.
"Project Reviewers" now lists both projects and packages. I have renamed this to "Coalition Reviewers" but that's probably horrible and confusing. I'm not sure "Group Reviewers" is much better.
Test Plan:
- Added a package as a reviewer manually.
- Joined it, got authority over it.
- Saw the review on my dashboard.
- Accepted the revision, got authority extended to the package review.
{F1311652}
{F1311653}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15914
Summary:
Ref T10939. This allows the CLI to parse reviewers and subscribers like this:
```Reviewers: epriestley, O123 Some Package Name```
The rule goes:
- If a reviewer or subscriber starts with a monogram (like `X111`), just look that up and ignore everything until the next comma.
- Otherwise, split it on spaces and look up each part.
This means that these are valid:
```
alincoln htaft
alincoln, htaft
#a #b epriestley
O123 Some Package, epriestley, #b
```
I think the only real downside is that this:
```
O123 Some Package epriestley
```
...ignores the "epriestley" part. However, I don't expect users to be typing package monograms manually -- they just need to be representable by `arc land` and `arc diff --edit` and such. Those flows will always add commas and make the parse unambiguous.
Test Plan:
- Added test coverage.
- `amend --show`'d a revision with a package subscriber (this isn't currently possible to produce using the web UI, it came from a future change) and saw `Subscribers: O123 package name, usera, userb`.
- Updated a revision with a package subscriber.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15911
Summary:
Ref T10939. This has no effect yet since packages can not actually become reviewers, I'm just inching toward support.
- When searching for "responsible users", include revisions that need review by packages you have authority over.
- When calculating review authority, include authority over packages you are a member of (these currently never exist).
Test Plan:
This isn't reachable so I just `var_dump()`'d stuff and looked at the generated queries, which appeared correct/reasonable.
I'll vet this more thoroughly once packages can actually become reviewers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15909
Summary: Fixes T9790. This uses a simple renderer, like the inline context renderer, that emphasizes getting a quick glance at small changes and working reasonably on mobile devices.
Test Plan:
- Set `inline` setting to `9999`.
- Created a diff.
- Saw it render reasonably in HTML mail.
- Also tested text mail to make sure I didn't break that.
{F1310137, size=full}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15901
Summary: Ref T10694. This setting no longer has any effect: we always show a limited amount of context now.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15886
Summary:
Ref T10694. General improvements:
- Remove leading empty lines from context snippets.
- Remove trailing empty lines from context snippets.
- If we removed everything, render a note.
- Try using `style` instead of `<pre>`? My thinking is that maybe Airmail has weird default rules for `<pre>`, since that's the biggest / most obvious thing that's different about this element to me.
Test Plan: Viewed normal comments locally, faked a comment on an empty line in the middle of a lot of other empty lines.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15864
Summary:
Ref T10694.
- Shift margins/padding around so inlines with multiple paragraphs get reasonable spacing.
- Add `text-decoration: none` to the "View Inline" link to kill the underline.
Test Plan: {F1265342}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15863
Summary: Ref T10694. Move the inline style more toward a mix of standard`<pre>` style and the web UI style for inlines.
Test Plan: See screenshots in comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15857
Summary:
Ref T10694. This mostly prevents us from having a degenerate case if someone leaves a 200-line inline.
- For one-line inlines, show 1 line of context above and below (3 lines total).
- For 3+ line inlines, show just the inline.
- For 7+ line inlines, show only the first part.
Test Plan: Made a bunch of weird long/short/different-sized comments, saw reasonble-appearing context in text and HTML mail output.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15853
Summary:
Ref T10694. Ref T9790. When generating inline diff context, highlight it and then mangle the highlighted output into `style="..."` so it works in HTML.
Also try to tighten up some spacing/formatting stuff.
Test Plan:
Got some output in this vein:
{F1259937}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790, T10694
Differential Revision: https://secure.phabricator.com/D15852
Summary:
Ref T10694. This is still missing some pieces, but seems to get most of the data into the mail in a plausible format:
- When an inline remarks on code, show the patch inline in the mail body.
- When an inline replies to another inline, show that other inline in the mail body.
- Apply remarkup rendering to inline content.
- Apply basic styling to mail body blocks.
Not covered yet:
- Syntax highlighting.
- Diff highlighting.
- Maybe clearer style/layout hints to connect comments to what they reply to? Current approach might get messy with inlines that have blockquotes and code blocks inside them, for example.
- I probably want to cap the amount of diff context we ever show to ~7 lines, even if you drag over 200 lines of code.
- CSS is a generally a bit rough still.
- The `unified-comment-context` option is effectively always on now, and should be removed.
- Text section is getting indented right now but probably shouldn't be.
- Spacing, etc., might be a bit off.
Test Plan:
Rigged Home to render these things, got a plausible-looking render (top is text, bottom is HTML):
{F1259052}
Sent myself some inline comment mail, got a plausible result.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15850
Summary: Fixes T10906, Fixes T10820. Adds new icons, grey-er colors for previous states. Also, I think fixed a few bugs?
Test Plan: Fake each state, verify icon is as intended.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10820, T10906
Differential Revision: https://secure.phabricator.com/D15830
Summary: This will stop breaking if you have subscribers and tags when updating a revision (`Error parsing field "Subscribers": The objects you have listed include objects which do not exist (Tags:)`), which I broke in D15749.
Test Plan: run through arc-diff --update that failed earlier.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15762
Summary:
In calendar, dashboard, diffusion, diviner, feed, fund,
maniphest, pholio, ponder, and slowvote use the term 'tags' if possible.
This intenctionally skips diffusion, differential, and the projects application itself.
Ref T10326 Ref T10349
Test Plan: inspection on a running, locally modified, system
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10835, T10326, T10349
Differential Revision: https://secure.phabricator.com/D15753
Summary: Users can't find the "Tags" field in the Edit Menu; Added keyword "Tag".
Test Plan: Looked in Edit page; I think this shouldn't change anything else?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15749
Summary: Found another bouncing around.
Test Plan: Review in diff
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15651
Summary: Bumps to 14px, fixes some on Differential
Test Plan: view various headers in Differential
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15647
Summary: Going to render these all normal case instead of all caps, and bump up the font size. Should be more consistent. Yellow if you green anything orange.
Test Plan: grep, lint
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15645
Summary: Fixes T10704. This is just bad copy-paste -- "O" for "old" should be "N" for "new".
Test Plan:
- Followed steps on T10704.
- Applied patch.
- Marked inline done, replied, etc. No more JS errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10704
Differential Revision: https://secure.phabricator.com/D15566
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary: Updates the Harbormaster UI to match the new two column everywhere else.
Test Plan: Did best I could, tested builds, plans, steps, buildables. Unable to test lint/unit locally, I need to set that up. Kick the tires for me pls. :3
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15523
Summary: Ref T9456. Some rough edges and we can't complete the build yet since I haven't written a webhook, but this mostly seems to be working.
Test Plan:
- Ran this build on some stuff.
- Ran a normal HTTP step build to make sure I didn't break that.
{F880301}
{F880302}
{F880303}
Reviewers: chad
Reviewed By: chad
Subscribers: JustinTulloss, joshma
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14286
Summary: This updates (all?) of Diffusion/Audit to new UI, included edit and other extra form pages. It's fairly complete but I don't know all the nooks and crannies so to speak to fully verify I didn't mess anything up.
Test Plan: Tested creating new repositories, browsing, searching, auditing. Need more eyes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15487
Summary: Fixes T10591. This was accidentally reverted in 148a50e48b, probably when resolvign a merge/rebase.
Test Plan: Will push to production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10591
Differential Revision: https://secure.phabricator.com/D15474
Summary:
First pass at converting Differential, I likely have some buggy-poos but thought I'd toss this up now in case very bad bugs present.
To do:
- Need to put status back on Hovercards
- "Diff Detail" probably needs a better design
Test Plan: Looking at lots of diffs, admittedly I dont have harbormaster, etc, running locally. Checked Diffusion for Table of Content changes on small and large commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15463
Summary:
Ref T10563. This isn't a complete fix, but should make viewing complex inline threads a little more manageable.
This just tries to put stuff in thread order instead of in pure chronological order. We can likely improve the display treatment -- this is a pretty minimal approach, but should improve clarity.
Test Plan:
T10563 has a "before" shot. Here's the "after":
{F1169018}
This makes it a bit easier to follow the conversations.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10563
Differential Revision: https://secure.phabricator.com/D15459
Summary: I think this works?
Test Plan:
i am wizard
{F1168808}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15457
Summary: Ref T10093. Changes must be pushed to staging before they can be landed from the web.
Test Plan:
Changes must be pushed to staging before they can be landed from the web.
{F1161909}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10093
Differential Revision: https://secure.phabricator.com/D15427
Summary:
Two minor changes here:
- Replace `get/setUser()` with `get/setViewer()` for consistency with everything else.
- `getViewer()` now throws if no viewer is set. We had a lot of code that either "should" check this but didn't, or did check it in an identical way, duplicating work. In contrast, very little code checks for a viewer but works if one is not present.
Test Plan:
- Grepped for `->user`.
- Attempted to fix all callsites inside `*View` classes.
- Browsed around a bunch of applications, particularly Calendar, Differential and Diffusion, which seemed most heavily affected.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15412
Summary:
Every caller returns `true`. This was added a long time ago for Projects, but projects are no longer subscribable.
I don't anticipate needing this in the future.
Test Plan: Grepped for this method.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15409
Summary: Reworks Maniphest into a two column view. Moves priority and color to header, assignee to sidebar. quest points to header, and author to gutter. may be some confusion since priority only displays on open tickets.
Test Plan: with and without description, custom fields, points, tablet, mobile and desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15396
Summary: Ref T10457. This gives unit test results a more first-class treatment in the Differential UI, and consolidates some rendering code.
Test Plan:
Before:
{F1135536}
After:
{F1135537}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15365
Summary: Ref T10457. These lack color and iconography and are difficult to parse. Make them easier to read.
Test Plan:
Before:
{F1135396}
After:
{F1135399}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15362
Summary:
Ref T10457. This makes diffs/revisions show the revision as the buildable title, and commits show the commit as the title.
Previously, the title was "Buildable X".
Also makes icons/colors/labels more consitent.
Test Plan: {F1131885}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15355
Summary: Moves all the one off object calls to PHUIRemarkupView, adds a "Document" call as well (future plans).
Test Plan: Visited most pages I could get access to, but may want extra careful eyes on this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15281
Summary: No UI changes, just some search and replace for UI consistency.
Test Plan: Test person and object hovercards still work. UIExamples too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15172
Summary: Fixes T10176. The prefix is not useful in the JIRA context, and doubtfully useful in Asana.
Test Plan: Load, make comment on revision, see link in JIRA is pretty.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T10176
Differential Revision: https://secure.phabricator.com/D15119
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary: See T10214 for context. These transaction types are obviously wrong as far as I can tell.
Test Plan:
Created a revision and didn't see an error in the daemon log.
```lang=php
<?php
require_once dirname(__FILE__).'/phabricator/scripts/__init_script__.php';
$yelirekim = (new PhabricatorPeopleQuery)
->setViewer(PhabricatorUser::getOmnipotentUser())
->withUsernames(['yelirekim'])
->executeOne();
$raw_diff = (new PhabricatorDifferenceEngine)
->generateRawDiffFromFileContent('oldfile', 'newfile');
$diff = (new ConduitCall('differential.createrawdiff', [
'diff' => $raw_diff,
]))
->setUser($yelirekim)
->execute();
$xactions = (new DifferentialDiffTransactionQuery)
->setViewer($yelirekim)
->withObjectPHIDs([$diff['phid']])
->execute();
foreach ($xactions as $xaction) {
echo $xaction->getPHID().':'.$xaction->getTitle().PHP_EOL;
}
```
for sanity
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: michaeljs1990, epriestley
Differential Revision: https://secure.phabricator.com/D15112
Summary:
Ref T6183. Ref T10054. Historically, only members could watch projects because there were some weird special cases with policies. These policy issues have been resolved and Herald is generally powerful enough to do equivalent watches on most objects anyway.
Also puts a "Watch Project" button on the feed panel to make the behavior and meaning more obvious.
Test Plan:
- Watched a project I was not a member of.
- Clicked the feed watch/unwatch button.
{F1064909}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6183, T10054
Differential Revision: https://secure.phabricator.com/D15063
Summary: Fixes T10169. Diffs with no build targets were incorrectly showing as though they had no test coverage, when we actually want to show them having no coverage information available.
Test Plan: Viewed an older revision, saw a column of "Not Executable" before change, now see no column.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10169
Differential Revision: https://secure.phabricator.com/D15044
Summary: Fixes T10139. This clarification seems reasonable to me.
Test Plan:
- Viewed a revision with context.
- Clicked "Show All Context".
- Saw "All Context Shown".
{F1060624}
{F1060625}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10139
Differential Revision: https://secure.phabricator.com/D15009
Summary:
Ref T8612. If a change affects more than 10K paths + hunks, tell the user it's too big and don't bother trying to write it. We're mostly bounded by INSERTs here.
Also, fix an issue with file upload errors. The keys are real PHP constants, but were accidentally converted to strings in D12797, causing every error to show as "unknown error".
Test Plan: {F1057509}
Reviewers: joshuaspence
Reviewed By: joshuaspence
Maniphest Tasks: T8612
Differential Revision: https://secure.phabricator.com/D14977
Summary:
Fixes T9319. Proxied requests (e.g., in the cluster) for binary files (like images) currently fail because we can not return binary data over Conduit in JSON.
Although Conduit will eventually support binary-safe encodings, a cleaner approach to this is just to return a `filePHID` instead of the raw content. This is generally faster and more flexible, and gives us more opportunities to add caching later.
After making the call, the client pulls the file data separately.
We also no longer need to return a complex data structure because we don't do blame over this call any longer.
Test Plan:
- Viewed images in Diffusion.
- Viewed READMEs in Diffusion.
- Used `bin/differential attach-commit rX Dy` to hit attach pathway.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14970
Summary: Ref T9319. Ref T2783. This won't currently work in a future environment where daemons and repositories are not on the same host. Send it over Conduit instead.
Test Plan: Used `bin/differential attach-commit rX Dy` to force attachment, saw valid content pull over Conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2783, T9319
Differential Revision: https://secure.phabricator.com/D14969
Summary:
Ref T9319. See D14967. As before, this is making a deeply-buried, complex operation easier to test by providing a CLI command.
This adds `bin/differential attach-commit rXnnnn Dnnnn` to pretend that `rXnnnn` was just committed and matched `Dnnnn`.
Test Plan:
- Ran `bin/differential attach-commit X Y` for several different values, saw updates in the UI.
- Faked the message parser to make sure stuff still worked there.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14968
Summary:
Ref T9319. When we discover a commit, we sometimes update the corresponding revision with a "this is the actual committed change" diff and send out a link to the changes between review and commit.
This is currently very difficult to test, because it only happens the first time and you have to either go set up a bunch of objects or add a bunch of special casing to the parser to hit the workflow.
I'm making some changes to how it pulls file content. To make those changes easier to test, first start extracting this stuff so the code can be run with `bin/differential extract ...` instead of needing to do a bunch of more complicated setup steps.
Test Plan:
- Ran `bin/differential extract ...` to extract diffs from commits.
- Forced my way through the daemon workflow by faking out a bunch of flags, got a clean extract + attach + update. After this patch, this should rarely be necessary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14967
Summary:
Ref T4245. Like everything else, accept more identifiers.
This needs a change in `arc`, which I've made a note about elsewhere.
Test Plan: Used "Update Now" from web UI, saw update get scheduled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14932
Summary: Ref T4245. Pass the whole repository in so it can do something else in a future change.
Test Plan: Loaded changesets in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14931
Summary:
Ref T9994.
- Allow errors to be dismissed.
- Tailor messaging for closed/abandoned revisions.
- Reduce scare messaging on land dialog, since it's not really that scary anymore.
Test Plan:
- Dismissed errors.
- Hit new warnings.
- Wasn't as scared when landing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9994
Differential Revision: https://secure.phabricator.com/D14886
Summary:
Ref T8980. Move away from events to EngineExtensions.
This also simplifies hovercards a bit:
- Removes tasks from revision cards.
- Removes blockers/blocked from task cards.
- Removes "Send Message" from user cards.
These mostly felt cluttery to me. Open to arguments to retain them. I think we can make better use of the space, though (e.g., flags, projects + board columns).
Test Plan:
- Viewed people, task, revision, commit and project hovercards.
{F1043256}
{F1043257}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14878
Summary:
Ref T8980. This isn't 100% coverage but should be pretty much all of the common ones.
These feel a touch iffy to me at first glance so I didn't go crazy trying to hunt all of them down. I have some other plans for them so maybe they'll feel better by the end of it.
Test Plan: Hovered over author, reviewers, blocked tasks, projects, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14877
Summary: Ref T10032. This is sufficent to hit NUX without doing anything bad.
Test Plan:
- Visited NUX.
- Browsed normally.
{F1043191}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14876
Summary:
Fixes T9156.
- Fix hashtag generation.
- Fix various badnesses.
- Improve project name generator.
Test Plan:
```
$ ./bin/lipsum generate projects
GENERATORS Selected generators: Projects.
WARNING This command generates synthetic test data, including user accounts. It is intended for use in development environments so you can test features more easily. There is no easy way to delete this data or undo the effects of this command. If you run it in a production environment, it will pollute your data with large amounts of meaningless garbage that you can not get rid of.
Are you sure you want to generate piles of garbage? [y/N] y
LIPSUM Generating synthetic test objects forever. Use ^C to stop when satisfied.
Generated "Project": Self-Flying Data Center Swag Performance
Generated "Project": Optimize Cars
Generated "Project": Triaging Culture Optimization
Generated "Project": Automating Experience
Generated "Project": Accelerating NUX Performance
Generated "Project": Optimizing Culture Optimization
Generated "Project": Optimize Hardware
```
{F1042949}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9156
Differential Revision: https://secure.phabricator.com/D14874
Summary: Ref T9156. This makes the UX a little more modern/standard/safe.
Test Plan:
```
epriestley@orbital ~/dev/phabricator $ ./bin/lipsum generate
Choose which type or types of test data you want to generate, or select "all".
- Differential Revisions
- Files
- Maniphest Tasks
- Pastes
- Pholio Mocks
- Projects
- User Accounts
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9156
Differential Revision: https://secure.phabricator.com/D14873
Summary: Ref T9979. This simplifies/standardizes the code a bit, but mostly gives us more consistent class names and structure.
Test Plan:
- Used `bin/search index --type ...` to index documents of every indexable type.
- Searched for documents by unique text, found them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14842
Summary:
Ref T9979. This is currently hard-coded but can be done in a generic way.
This has one minor behavioral changes: answer text is no longer included in the question text index in Ponder. I'm not planning to accommodate that for now since I don't want to dig this hole any deeper than I already have. This behavior should be different anyway (e.g., index the answer, then show the question in the results or something).
Test Plan:
- Put a unique word in a Maniphest comment.
- Searched for the word.
- Found the task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14837
Summary: Ref T10032, adds "Basic" NUX to more applications.
Test Plan: Visit each with ?nux=true and click on the create link. T10032 is tracking which apps need general modernization to pick up these changes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10032
Differential Revision: https://secure.phabricator.com/D14847
Summary:
Ref T3462. If someone works directly on `master`, we currently show "Branch: master (branched from master)" in the UI.
Although this is sort of technically accurate, it is confusing.
Instead, just show "Branch: master" in this situation.
Test Plan: Saw "master" instead of "master (branched from master)".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3462
Differential Revision: https://secure.phabricator.com/D14829
Summary:
Ref T10004. After D14804, we get this behavior by default and no longer need to set it explicitly.
(If some endpoint did eventually need to set it explicitly, it could just change what it passes to `setHref()`, but I believe we currently have no such endpoints and do not foresee ever having any.)
Test Plan:
- As a logged out user, clicked various links in Differential, Maniphest, Files, etc., always got redirected to a sensible place after login.
- Grepped for `setObjectURI()`, `getObjectURI()` (there are a few remaining callsites, but to a different method with the same name in Doorkeeper).
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14805
Summary:
Ref T9952. Ref T3462. After D14736, if we have information about the target/"onto" branch, use it in the UI:
- Show "feature (branched from master)" instead of "feature".
- Default "Land Revision" to hit the correct branch.
Test Plan:
- Branched from `test` with branch tracking.
- Diffed.
- Saw "feature (branched from test)" in UI.
- Saw "test" fill as default in "Land Revision", despite the repository having a different default branch.
{F1020587}
{F1020588}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3462, T9952
Differential Revision: https://secure.phabricator.com/D14737
Summary:
Ref T9952. Default the branch target in the dialog to be whatever branch is the default branch for the repository.
This will be correct for repositories like ours (which land everything into `master`) and correct most of the time for repositories which have some other "primary" branch (maybe `development`).
It won't be great if there are multiple open lines of development in a repository (for example, some changes go to `newdesignpro` and some changes go to `legacy-1.2`). I'll do work in T3462 next to improve those cases so we can pick a better default.
Test Plan:
- Saw dialog default to "master".
- Changed repo default branch, saw it default to "notmaster" instead.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14734
Summary:
Ref T9952. This adds a typeahead so you can pick a branch to target.
It does not choose a default branch, the user must pick a branch explicitly.
Test Plan:
- Landed rGITTESTd587fada48fc to `master` (by typing "master").
- Landed rGITTEST86c339b2ef01 to `notmaster` (by typing "notmaster").
{F1020531}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14733
Summary:
Ref T9132. When I've touched `PhabricatorApplication` I keep hitting this bad `pht()` junk.
The warning is correct, these strings are not extactable and can not be translated.
Fix it so they can be extracted and translated.
Broadly, in all cases we want to render one of these:
> 95 Things (for fewer than some limit)
> 99+ Things (when we hit the limit)
Test Plan: Looked at homepage status counts, moused over them, saw reasonable strings. Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14638
Summary:
Ref T9845. In Differential, this is not a remarkup block -- it's a mail section. `addTextSection()` has special magic behavior when handed a prebuilt section since D9375.
Swapping to `addRemarkupSection()` causes the error in T9845 and renders nothing in the comment section.
Even if it were a block of text, it would not be appropriate to add it as remarkup. This would incorrectly render comments in files like `__init__.py`, which are common on Python (the filename would render as "__init__.py"). Okay that's a bad example since it works fine but, uh, a file named `T123` would be no good or whatever.
I'll realign T9845 to clean this up and fix it more durably.
Test Plan: Sent myself some mail with inline comments, saw them in the mail.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9845
Differential Revision: https://secure.phabricator.com/D14589
Summary: Ref T992. I noticed that `ManiphestTask` mail doesn't render Remarkup properly (instead, it renders Remarkup literally). I //think// this is because the code calls `addTextSection()` rather than `addRemarkupSection()`.
Test Plan: Created a new Maniphest Task and saw Remarkup in the generated self-email (inspect the email contents with `./bin/mail show-outbound`). I didn't test the other affected applications.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D14511
Summary:
Ref T9787. To fix this, I want to change how file PHIDs are extracted slightly: specifically, I'm going to extract them later in the editing process.
Before doing this, clean up a couple of bad implementations:
- Owners extracts its description as a file PHID. This is an error.
- Extract the description as a remarkup block instead.
- Add an edge table so stuff like file attachment works properly.
- Differential has a no-op extract method. This is presumably just a copy/paste issue from long ago.
Test Plan:
- Edited a revision in Differential.
- Dropped a file into the description of an Owners package.
- Before change: this did not attach the file.
- After change: the file now attaches properly and shows up as "Attached" in the file details.
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Subscribers: joshuaspence
Maniphest Tasks: T9787
Differential Revision: https://secure.phabricator.com/D14493
Summary: Some linter messages, such as those produced by `ArcanistPHPCompatibilityXHPASTLinterRule`, contain backticks but are currently rendered as Remarkup literals. I think that it is generally desirable to allow lint messages to be rendered as Remarkup, although we should ideally have a way to render Remarkup for use on the command line (I actually think that this already exists, but I don't think that `arc lint` does this when rendering linter messages).
Test Plan: Resubmitted D14481 to my dev install and saw Remarkuped lint messages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14485
Summary:
Fixes T9672. This was never turned into a custom field, for no particular reason. Convert it into one.
This is substantially similar to the existing "Apply Patch" field, which does the same thing (only shows a command).
We might rethink or remove this eventually (e.g., in a post-"Land Revision" world) but this makes it easier, at the very least.
Test Plan:
- Viewed a non-accepted revision (no hint).
- Viewed an accepted revision from a raw diff source (no hint).
- Viewed an accepted revision from Git (`arc land` hint).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9672
Differential Revision: https://secure.phabricator.com/D14367
Summary:
Ref T182. Make the disabled state of the button more accurately reflect whether clicking it will work.
Don't allow "land" to proceed unless the revision is accepted.
Test Plan: Saw button in disabled state, clicked it, got "only accepted revisions" message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14350
Summary:
Ref T182. When viewing a revision, if there are several error operations and then a success operation, we currently show the last error. This is misleading.
Instead, don't show anything if there's a success (this may require tuning eventually if you can land multiple times onto different branches or whatever, but should be reasonable for now).
Also make the table a little nicer, particularly for merge failure output.
Test Plan: {F910385}
Reviewers: chad, Mnkras
Reviewed By: Mnkras
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14348
Summary:
Ref T182.
- We just show the oldest operation right now, but we usually care about the oldest non-failure.
- Only query for actual land operations when rendering the revision operations dialog (maybe eventually we'll show more stuff?).
- For now, prevent multiple lands / repeated lands or queueing up lands while other lands are happening.
Test Plan: Landed a revision. Tried to land it more / again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14338
Summary:
Ref T182. Replace the total mess we had before with a sort-of-reasonable element.
This automatically updates using "javascript".
Test Plan:
{F901983}
{F901984}
Used "Land Revision", saw the land status go from "Waiting" -> "Working" -> "Landed" without having to mash reload over and over again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14314
Summary: Ref T182. Make a reasonable attempt to get the commit message, author, and committer data correct.
Test Plan: BEHOLD: rGITTEST810b7f17cd0c909256a45d29a5062fcf417d0489
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14280
Summary:
Ref T182. If 35 other things are configured completely correctly, make it remotely possible that this button may do something approximating the thing that the user wanted.
This primarily fleshes out the idea that "operations" (like landing, merging or cherry-picking) can have some beahavior, and when we run an operation we do whatever that behavior is instead of just running `git show`.
Broadly, this isn't too terrible because Drydock seems like it actually works properly for the most part (???!?!).
Test Plan: {F876431}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14270
Summary:
Ref T182. This doesn't do anything interesting yet and is mostly scaffolding, but here's roughly the workflow. From previous revision, you can configure "Repository Automation" for a repository:
{F875741}
If it's configured, a new "Land Revision" button shows up:
{F875743}
Once you click it you get a big warning dialog that it won't work, and then this shows up at the top of the revision (completely temporary/placeholder UI, some day a nice progress bar or whatever):
{F875747}
If you're lucky, the operation eventually sort of works:
{F875750}
It only runs `git show` right now, doesn't actually do any writes or anything.
Test Plan:
- Clicked "Land Revision".
- Watched `phd debug task`.
- Saw it log `git show` to output.
- Verified operation success in UI (by fiddling URL, no way to get there normally yet).
Reviewers: chad
Reviewed By: chad
Subscribers: revi
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14266
Summary:
Fixes T9494. This:
- Removes all the random GC.x.y.z config.
- Puts it all in one place that's locked and which you use `bin/garbage set-policy ...` to adjust.
- Makes every TTL-based GC configurable.
- Simplifies the code in the actual GCs.
Test Plan:
- Ran `bin/garbage collect` to collect some garbage, until it stopped collecting.
- Ran `bin/garbage set-policy ...` to shorten policy. Saw change in web UI. Ran `bin/garbage collect` again and saw it collect more garbage.
- Set policy to indefinite and saw it not collect garabge.
- Set policy to default and saw it reflected in web UI / `collect`.
- Ran `bin/phd debug trigger` and saw all GCs fire with reasonable looking queries.
- Read new docs.
{F857928}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9494
Differential Revision: https://secure.phabricator.com/D14219
Summary: Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers).
Test Plan: With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9352
Differential Revision: https://secure.phabricator.com/D14222
Summary:
Ref T9252. This primarily allows Harbormaster to request (and Drydock to fulfill) working copies with a patch from a staging area. Doing this means we can do builds on in-review changes from `arc diff`.
This is a little cobbled-together but should basically work.
Also fix some other issues:
- Yielded, awakend workers are fine to update but could complain.
- We can't log slot lock failures to resources if we don't end up saving them.
- Killing the transaction would wipe out the log.
- Fix some TODOs, etc.
Test Plan: Ran Harbormaster builds on a local revision.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14214
Summary: This is to make it more obvious how to ignore a folder that may be at the root, and it resolves https://secure.phabricator.com/T8894
Test Plan: Just a documentation change
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, nornagon
Differential Revision: https://secure.phabricator.com/D14193
Summary: Ref T9252. This is still crude in a few ways but basically works, at least for commits.
Test Plan:
- Made a build plan with just this build step.
- Ran `bin/harbormaster build --plan 10 ...` on a commit.
- It actually built a working copy, leased it, took no action, and released the lease. MAGIC~~~
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14160
Summary:
This reverts commit 1583738842.
See T8646 for discussion. This version of the feature feels terrible on real data.
Test Plan: Strict revert.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14097
Summary:
Fixes T9126. In particular:
- Add "Browse" links to all history views.
- Use icons to show "Browse" and "History" links, instead of text.
- Use FontAwesome.
- Generally standardize handling of these elements.
This might need a little design attention, but I think it's an improvement overall.
Test Plan:
- Viewed repository history.
- Viewed branch history.
- Viewed file history.
- Viewed table of contents on a commit.
- Viewed merged changes on a merge commit.
- Viewed a directory containing an external.
- Viewed a deleted file.
{F788419}
{F788420}
{F788421}
{F788422}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9126
Differential Revision: https://secure.phabricator.com/D14096
Summary:
Ref T8646. This is fairly rough:
This interface is very niche, and not really flexible enough to accommodate other result customization (but I don't think we have any plans here)?
I'm just //summarizing// the content of documents, basically showing the first paragraph of their content, summary, etc. This isn't what Google does: it shows snippets surrounding the actual search terms. However, this is more involved and might be less useful in structured data: for example, I'd imagine that the first line of most phriciton documents, maniphest tasks and Differential revisions really might be the best machine-generatable summary of them. The actual contextual snippeting in Google doesn't often seem hugely useful to me. But this might also not be very useful.
There's not much design, not sure if you had any ideas.
I only implemented this for tasks, revisions and the wiki since those seem most useful.
I'm generally on the fence about this, but it's not a ton of work to swap out for something else later. Maybe we can see how it feels? But happy to toss it or rethink the approach.
Test Plan: {F788026}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8646
Differential Revision: https://secure.phabricator.com/D14095
Summary: Ref T9134. It looks like this functionality was removed in D13848.
Test Plan: Submitted a diff successfully.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9134
Differential Revision: https://secure.phabricator.com/D13869
Summary:
Fixes T9268. Currently, we try to match any string like "a2f313f1" as a commit/revision, so short hashes will get picked up.
However, we don't require a word boundary or terminal after the match, so for input like "aaa...aaaaz" the engine can get stuck trying to split the string into sub-matches.
That is, in the original case, the input "aaaz" had valid matches against `[rA-Z0-9a-f]+` up to "z" of:
aaa
aa a
a aa
a a a
All of these will fail once it hits "z", but it has to try them all. This complexity is explosive with longer strings.
Instead, require a word boundary or EOL after the match, so this is the only valid match:
aaa
Then the engine sees the "z", says "nope, no match" and doesn't have to backtrack across all possible combinations.
Test Plan: Added a failing unit test, applied patch, clean test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9268
Differential Revision: https://secure.phabricator.com/D13997
Summary:
Ref T9218. See discussion there for rationale; I think this is the right behavior to pursue.
The screenshot below is pretty ugly. I think it's a lot worse than most real-world cases will be, since you have to sort of opt-in to having crazy levels of overlapping packages, and it's perfectly normal/reasonable for files owned by one package. Owners is powerful enough to let you specify sub-packages with exclusive ownership.
That said, this may be more typical than I hope. I don't think we can reduce the complexity here much for free, but it would might be reasonable to add some view options (e.g.: group by package?, show only packages I own?, show packages as icons with a tooltip?) if it's an issue.
Test Plan: {F734956}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9218
Differential Revision: https://secure.phabricator.com/D13940
Summary: Fixes T8428. Adds status to packages, allows setting and application search. I presume though these need checked elsewhere?
Test Plan: New package, edit package, archive package, run search queries.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8428
Differential Revision: https://secure.phabricator.com/D13925
Summary:
Fixes T8004.
- For paths which are part of a package, show the package.
- Highlight paths which are part of a package you (the viewer) have authority over.
Test Plan:
{F725418}
- Viewed owned and unowned chagnes in Diffusion and Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8004
Differential Revision: https://secure.phabricator.com/D13923
Summary:
Fixes T2183. We now use the same rendering element in both places.
Intentional changes:
- Package highlighting is out, coming back to both apps in next diff.
- removed redundant-feeling "Change" link. The information is now shown with a character ("M", "V", etc.) and the page is a click away under "History". Clicking the path also jumps you to substantially similar content. (We could restore it fairly easily, I just think it's probably the least useful thing in the table right now.)
Test Plan: Viewed a bunch of commits in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13910
Summary:
Ref T2183. Introduces a new View which can (in theory) unify the Revision, Diff and Commit table of contents views.
This has the same behavior as before, but accepts slightly more general primitives and parameters and has somewhat cleaner code.
I've made one intentinoal behavior change: removing the "Open All in Editor" button. I suspect this is essentially unused, and is a pain to keep around. We can look at restoring it if anyone notices.
Test Plan: Looked at a bunch of revisions, no changes from before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13908
Summary: Ref T2183. These properties are not used and not useful.
Test Plan: Grepped for callsites and uses. Loaded revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13906
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.
Test Plan: Poked around a bunch of pages.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13589
Summary: Fixes T9128. I missed this when converting other controllers to look in Harbormaster for lint/unit data.
Test Plan: Copy/pasted a diff successfully.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9128
Differential Revision: https://secure.phabricator.com/D13865
Summary:
Ref T8096. This modernizes the last thing which was reading the old datasource.
Also fix a bug where it didn't work.
Test Plan: {F698405}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13852
Summary: Ref T8096. This information has moved into Harbormaster.
Test Plan:
Pushed some test results in; see right margin:
{F698394}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13850
Summary:
Ref T8096.
Long ago, support for "postponed" lint and unit tests got hacked in. `arc` would publish a bunch of ghost results, and then something else would fill the results in later.
This was always a hack. It is not nearly as powerful or flexible as having a real build system, and is obsolete with Harbormaster, which supports these operations in a more reasonable and straightforward way.
This was used (only? almost only?) at Facebook.
- Remove `differential.finishpostponedlinters`. This only served to update postponed linters.
- Remove lint magic in `differential.setdiffproperty`. This magic only made sense in the context of postponed linters.
- Remove `differential.updateunitresults`. The only made sense for postponed unit tests.
And one minor change: when a diff contains >100 affected files, we hide the content by default, but show content for files with inline comments. Previously, we'd do this for lint inlines, too. I don't tink this is too useful, and it's much simpler to just remove it. We could add it back at some point, but I think large changes often trigger a lot of lint and no one actually cares. The behavior for actual human inlines is retained.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13848
Summary: Ref T8096. These are still reading out of the old diff property, but should read from Harbormaster instead.
Test Plan: {F698346}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13847
Summary: Ref T8096. Currently, we hide passing tests by default (they aren't interesting) but should also hide skipped tests by default (they aren't interesting either).
Test Plan: {F694485}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13821
Summary: Fixes T9060. These actions still work fine, but the transcripts got messed up a bit.
Test Plan: Viewed transcripts with blocking actions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9060
Differential Revision: https://secure.phabricator.com/D13782
Summary: Ref T8726. No surprises.
Test Plan:
Created rules using both action variants, applied upgrade, saw rules still work correctly.
{F658842}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13701
Summary: Ref T8726. Converts these actions to be modular. No real surprises in this change.
Test Plan:
{F658709}
- Wrote some rules.
- Migrated them forward.
- Used a bunch of these rules.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13699
Summary:
Ref T8726. This modularizes "Mark with flag", plus rebuilds transcripts in a more modern/flexible way. The big transcript stuff is:
- Transcripts are now translatable.
- Transcripts can now show multiple outputs from a single action. For example, an action like "add A, B, C to subscribers" can now say "added A; B is invalid; C was already subscribed".
Test Plan: {F637784}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, eadler, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13649
Summary:
Ref T8726. Herald actions are technically sort-of modular already, but make them more aggressively modular similar to `HeraldField`.
I plan to obsolete and replace `HeraldCustomAction`.
Test Plan: Saw actions in nice groups; created and ran a "Do Nothing" action. Transcripts are a bit rough for now.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13646
Summary:
Fixes T8917. Prior to T2618, deleting inlines prompted users, then really deleted the rows.
After T2618, we delete immediately and offer "Undo". However, some interactions with drafts were missed, and we were only clearing the "this revision has a draft" flag on one of the delete pathways (when you delete all the comment text, then save the comment).
Make both the "Delete" action and the "Delete All Comment Text + Save" workflows do the same thing: mark the row as deleted, and clear any relevant drafts.
Test Plan:
- Made an inline comment on a clean revision with no "draft comments" marker in the list view.
- Used "delete" to delete it.
- After applying the patch, verified that no "draft commetns" marker appears in the list view.
- Used Delete and Edit + Remove Text + Save to delete comments in Differential and Diffusion.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8917
Differential Revision: https://secure.phabricator.com/D13665
Summary:
Ref T8726. Some adapters now have a large number of fields, and we lost the sort-of-human-readable implicit ordering when fields were modularized.
Instead, group and sort fields.
Test Plan: {F603066}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13619
Summary: Ref T8726. This gets rid of all the `VALUE_*` constants and lets Fields provide arbitrary typeaheads without upstream/JS changes.
Test Plan: Used all tokenizers. Used "Another Herald Rule". Grepped for all removed constants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13616
Summary:
Ref T8726. I want to modularize values and reduce how hard-coded / copypasta'd they are.
- Rename `get...StandardCondition()` to `get...StandardType()`, since we can drive both conditions and values from it.
- Rename `STANDARD_LIST` to `STANDARD_PHID_LIST` for consistency: all "lists" are lists of PHIDs.
- For all standard types which don't require typehaeads, lift their logic into the base class.
- I'll lift typeaheads soon, but need to generalize them first.
Test Plan: Edited various Herald rules, saw value UI generate correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13612
Summary: Ref T8726. Make all the DifferentialRevision stuff modular.
Test Plan:
- Created a rule with all fields.
- Ran upgrade.
- Saw all fields preserved with new modular versions.
- Used test console to run rule with all fields, verified field values as broadly sensible.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13598
Summary: Ref T8726. This deals with all the Differential diff fields, same deal as previous changes.
Test Plan:
- Wrote a rule with every field.
- Migrated it.
- Saw the same rule working.
- Rigged the hell out of transcripts (diffs normally do not generate transcripts, because the only action is "block" and they don't exist yet when Herald runs).
- Verified that all fields looked sensible in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13590
Summary: Fixes T7604. This is the big scary change which drops the "arcanist project" fields from the database permanently.
Test Plan:
`grep`ped for the following to ensure that I had found all remaining references:
- `/arcanistProject/i`
- `/arcanist_project/i`
- `/projectName/i`
- `/project_name/i`
- `/project_id/i`
WARNING: Wait at least one month before landing this.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12899
Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13422
Summary: Ref T8099. I think these colors make sense, but if any seem wrong, lmk. Colors the type of file change.
Test Plan: Test a few diffs locally, read carefully, ask epriestley.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13502
Summary: Ref T8650. This should stop the problem, but isn't a root cause fix. See discussion on the task.
Test Plan: Made some local diffs, but this is a bit hard to reproduce reliably.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8650
Differential Revision: https://secure.phabricator.com/D13441
Summary:
Ref T8096. Various tweaks here:
- Sort result lists by importance (even lint -- "errors first" seems better than "alphabetical by file", I think?).
- Do sane stuff with display limits.
- Add a "view all" view.
- Don't show a huge table of passing tests in Differential.
- Link to full results.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13407
Summary: Ref T8096. No functional changes, just a bit less code.
Test Plan: Viewed some revisions, saw the same stuff as before.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13404
Summary: Fixes T8095. Still needs UI/UX work (see T8096) but this has all the core features now.
Test Plan: Saw Harbormaster lint/unit data as though it was Differential lint-unit data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13401
Summary:
Ref T8096. Fixes a few bugs and glitches.
- Set build completion time when handling a message.
- Format duration information in a more human-readable way.
- Use a table for build variables.
- Fix up container PHIDs on diffs (a touch hacky, should be OK for now though).
Test Plan: Browsed around the UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13382
Summary:
Ref T8095. When build results are reported for a target, allow them to include unit and lint results.
There is no real way to see this stuff in the UI yet, either in Harbormaster or Differential.
Test Plan: Manually called this method with some results, saw Harbormaster update appropriately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13380
Summary: Ref T8095. This weird grey table has no remaining callsites and can be removed.
Test Plan: Grepped for symbols.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13379
Summary:
Ref T8095. Same as D13377, but for unit results.
This is a bit rough and there's some duplication between this and unit results. I'll likely merge them later, but I think some of it is superficial since these iterations are still a little crude.
Test Plan: {F523499}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13378
Summary:
Ref T8095. Render lint results in a future-ready way.
This makes the renderer accept `HarbormasterBuildLintMessage` objects. If we have legacy data instead, it converts it into `HarbormasterBuildLintMessage` objects.
Design is a bit rough but will be cleaned up later after T7739.
This moves away from "postponed linters", which are obsolete after Harbormaster (and were only ever used by Facebook).
Test Plan: {F523429}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13377
Summary: Ref T8099, This adds a consistent background color to object and policy tags, and highlights them when they deviate from the normal. Still likely worth revamping 'closed' and 'review' state colors.
Test Plan: Review lots of diffs and tasks.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13399
Summary:
Fixes T6787. I'm kind of cheating a little bit here by not unifying default selection with `initializeNew(...)` methods, but I figure we can let this settle for a bit and then go do that later. It's pretty minor.
Since we're not doing templates I kind of want to swap the `'template'` key to `'type'` so maybe I'll do that too at some point.
@chad, freel free to change these, I was just trying to make them pretty obvious. I //do// think it's good for them to stand out, but my approach is probably a bit inconsistent/heavy-handed in the new design.
Test Plan:
{F525024}
{F525025}
{F525026}
{F525027}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: johnny-bit, joshuaspence, chad, epriestley
Maniphest Tasks: T6787
Differential Revision: https://secure.phabricator.com/D13387
Summary:
Ref T8095. Two general problems:
- I want Harbormaster to own all lint and unit test results.
- I don't want users to have to configure anything for `arc` to keep working automatically.
These are in conflict because generic lint/unit test ownership in Harbormaster requires that build targets exist which we can attach build results to. However, we can't currently create build targets on demand: Harbormaster assumes it is responsible for creating targets, then running code or making third-party service calls to actually run the builds.
I considered two broad approaches to let `arc` push results into Harbormaster without requiring administrators to configure some kind of "arc results" build plan:
# Add magic target PHIDs like `PHID-MAGIC-this-is-really-arc-unit`.
# Add new code to build real targets with real PHIDs.
(1) is probably a bit less work to get off the ground, but I think it's worse overall and very likely to create more problems in the long run. I particularly worry that it will lead to a small amount of special casing in a very large number of places, which seems more fragile.
(2) is more work upfront but I think does a better job of putting all the special casing in one place that we can, e.g., more reasonably unit test, and letting the rest of the code rarely/never care about this case since it's just dealing with normal plans/steps/targets as far as it can tell.
This diff introduces "autoplans", which are source templates for plans/steps. This let us "push" these targets into Harbormaster. Hypthetically, any process "like" arc can use autoplans to upload test/lint/etc results. In practice, probably only `arc` will ever use this, but I think it's still quite a bit cleaner than the alternative despite all the generality.
Workflow is basically:
- `arc` creates a diff.
- `arc` calls `harbormaster.queryautotargets`, passing the diff PHID and saying "I have some lint and unit results I want to stick on this thing".
- Harbormaster builds the plan, steps, and targets (if any of them don't already exist), and hands back the target PHIDs so `arc` has a completely standard-looking place to put results.
- `arc` uploads the test results to the right targets, as though Harbormaster had asked it to run unit/lint in the first place.
(This doesn't actually do any of that yet, just sets things up.)
I'll maybe doc turn that ^^^^^^ into a doc for posterity since I think it's hard to guess what an "autotarget" is, but I'm going to grab some lunch first.
Test Plan:
- Added unit tests to make sure we can build these things properly.
- Used `harbormaster.queryautotargets` to build autotargets for a bunch of diffs.
- Verified targets come up in "waiting for message" state.
- Verified plans and steps are not editable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13345
Summary: Ref T8099, Now that we have cleaner headers, we can add more pop to important items, like the state of an Object. This makes it easier to just note the color and generally understand the state of the object (closed, returned, accepted, open). @epriestley, I think you previously thought this was a bug, but if it still feels bad, let me know.
Test Plan:
Review Differential, Maniphest, Polls for various object states.
{F520973}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13369
Summary: Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?
Test Plan: Review and do a search in each application changed. Grep for all call sites.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13332
Summary:
Ref T8575. We run a big "(A) UNION (B)" query on the home page and on the main Differential page.
"A" can always be improved by using `%Ls`, so it can use the second half of the `(authorPHID, status)` key.
"B" can sometimes be improved if the fraction of open revisions is smaller than the fraction of revisions you are reviewing. This is true for me on secure.phabricator.com (I'm a reviewer, either directly or via 'Blessed Reviewers', on about 80% of revisions, but <5% are open). In these cases, a `(status, phid)` key is more efficient.
Test Plan: Tweaked queries and added keys on this server, saw less derpy query plans and performance.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8575
Differential Revision: https://secure.phabricator.com/D13325
Summary:
Fixes T5138. Some of the "revision" properties are really "diff" properties, but we only show the properties for the most recent / current diff.
- Immediately, this makes it hard or impossible to review, e.g., lint/unit results for older diffs.
- Longer-term, these limits will become more problematic with more data on diffs after Harbormaster.
Instead, separate "revision" from "diff" properties.
(In the long term, it might make sense to show more diffs in this panel -- e.g., tabs for the 8 most recent updates or something -- but I went with the simplest approach for now since I don't have a clean way to deal with 100-update revisions offhand.)
Test Plan: {F500480}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T5138
Differential Revision: https://secure.phabricator.com/D13282
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: This `break` statement causes `./bin/hunks migrate` to only migrate one-hunk-at-a-time, which is unnecessary and slightly misleading. Instead, allow the script to migrate //all// legacy hunks to modern storage. In particular, this means that we can recommend that installs run this command sometime before D13222 is landed.
Test Plan: It's a pain to setup the data necessary to test this, but this is identical to the change that I made on our production install when I migrated our hunk storage.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13288
Summary:
Ref T5681. Ref T6860. This doesn't do anything interesting on its own, just makes the next diff smaller.
In the next diff, policies become aware of the types of objects they're acting on. We need to specify which object type all the "Default View/Edit" settings are for so they get the right rules.
For example, a rule like "Allow task author" is OK for "View Policy" on a task, and also OK for "Default View Policy" on ManiphestApplication. But it's not OK for "Can Create Tasks" on ManiphestApplication.
So annotate all the "template"/"default" policies with their types. The next diff will use these to let you select appropriate rules for the given object type.
Test Plan:
- Used `grep` to find these.
- This change has no effect.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T6860
Differential Revision: https://secure.phabricator.com/D13251
Summary:
Ref T8455. Use standard effects for revisions, instead of a custom effect.
This fixes the major issue (conduit error) in T8455 because the standard effect now performs PHID type filtering.
This retains other behaviors (in particular: not re-CC'ing explicitly removed CCs).
Test Plan:
- With a Herald rule that adds a mailing list as a CC, created a revision before the change and hit the error in T8455. After the change, saw correct behavior.
- Wrote a normal Herald rule to add CCs and created a revision, saw it fire properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13183
Summary:
Ref T8455. The Herald code in general isn't nearly as modular as it should be, and the subscriber code particularly has some legacy cruft. This is making it fragile and causing the issue described in T8455.
Currently, each Herald adapter has essentially identical code which it uses to determine which users are subscribed to an object. Instead, share code between object types.
I removed "explicitCCs":
- The value was always identical to doing the query in the common/standard way.
- They were only used to print a diagnostic message on transcripts, which I think is no longer relevant.
- I believe it predates transactions, so when it was added you couldn't figure out the old object state by looking at the transaction history. Now, CC changes are recorded there, so there's no need to restate the CC state on the transcript.
- Even if we do want to restore this (or something similar), we can do it directly from Herald now.
Test Plan:
- Created rules that use the "CCs" field in Herald, Pholio, Maniphest and Differential.
- Updated objects in each application.
- Observed valid field reads in the tranascript.
- Grepped for `FIELD_CC`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13177
Summary: Ref T8463.
Test Plan:
- Created a new revision via web UI with a username `@mention` in the summary and no repository.
- Prior to patch, hit a "not attached" error.
- After patch, no error.
- Created a new web UI revision, as above, but with a repository; saw repository work fine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8463
Differential Revision: https://secure.phabricator.com/D13205
Summary: Remove the `*TransactionType` classes and define the constants in the corresponding `*Transaction` class instead.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13188
Fixes this, which only triggers on some kinds of mail:
```
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] [2015-06-04 02:56:38] EXCEPTION: (PhutilProxyException) Error while executing Task ID 902251. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on DifferentialRevision (via getActiveDiff()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=6dede2e2c513), phutil(head=master, ref.master=afc05a9a7f00)
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/differential/storage/DifferentialRevision.php:158]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #1 <#2> DifferentialRevision::getActiveDiff() called at [<phabricator>/src/applications/differential/customfield/DifferentialBranchField.php:72]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #2 <#2> DifferentialBranchField::updateTransactionMailBody(PhabricatorMetaMTAMailBody, DifferentialTransactionEditor, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2481]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #3 <#2> PhabricatorApplicationTransactionEditor::addCustomFieldsToMailBody(PhabricatorMetaMTAMailBody, DifferentialRevision, array) called at [<phabricator>/src/applications/differential/editor/DifferentialTransactionEditor.php:1208]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #4 <#2> DifferentialTransactionEditor::buildMailBody(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2178]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #5 <#2> PhabricatorApplicationTransactionEditor::sendMailToTarget(DifferentialRevision, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2152]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #6 <#2> PhabricatorApplicationTransactionEditor::sendMail(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:998]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #7 <#2> PhabricatorApplicationTransactionEditor::publishTransactions(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #8 <#2> PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #9 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #10 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:20]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #11 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #12 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Auditors: btrahan
Summary:
Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.
There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.
Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:
- This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them.
- This is more correct, since we want to freeze the lists at this moment in time.
Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook.
---
This is a bit fragile and I'm not //thrilled// about it.
It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.
Test Plan:
Directly updated editors:
- Created a new Phriction page, saw "Document Content".
- Edited a Phriction page, saw "Document Diff".
- Edited a revision, got normal looking mail.
- Faked in `changedPriorToCommitURI` and verified it survived the state boundary.
- Sent Audit mail.
- Sent invoice mail.
Indirect editors - for these, I just made a change and made sure the mail generated:
- Updated a paste.
- Updated an event.
- Updated a thread.
- Updated a task.
- Updated a mock.
- Updated a question.
- Updated a project.
- Updated a file.
- Updated an initiative.
- Updated a Legalpad document.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, fabe
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13115
Summary:
Ref T7703. See that task and inline for a bunch of discussion.
Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later.
We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times.
Test Plan:
- Added and executed unit tests.
- Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions.
- Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7703
Differential Revision: https://secure.phabricator.com/D13104
Summary: Ref T8387. This is now completely obsoleted by mailing list users.
Test Plan: Grepped for `mailinglist` and related symbols.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13129
Summary: Ref T7604. Remove these "arcanist project" fields from the `LiskDAO` classes, but leave the data intact (there are no more read/writes to this data).
Test Plan:
- Grepped for calls to these methods.
- Ran `./bin/storage adjust` to check for schema issues.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D13012
Summary: Ref T8099, No specific reason for these to be small buttons.
Test Plan: Test desktop and mobile layouts.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13081
Summary: These could cause F442758.
Test Plan: {F442779}
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13042
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.
This is sticky per-user.
My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).
Specifically, this adds a new action here:
{F435621}
Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:
{F435626}
You can click the icon to reveal all hidden comments below the line.
Test Plan:
- Hid comments.
- Showed comments.
- Created, edited, deleted and submitted comments.
- Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: jparise, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D13009
Summary:
We don't want to support this right now, so disable it, similar to "Land To Hosted" feature.
Keep to the code as an example for advanced installs.
Ref T182, T8313
Test Plan: load a diff.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T182, T8313
Differential Revision: https://secure.phabricator.com/D13022
Summary: These format strings use `%d` instead of `%s`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12996
Summary:
fixes T8260. Only turn on symbol links if:
- The repository has any configuration about symbols, or
- There actually are symbols in the repository.
Test Plan: Look at revisions and files in various states of configurations and having symbols.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T8260
Differential Revision: https://secure.phabricator.com/D12946
Summary: Converts most all tables to be directly set via `setTable` to an ObjectBox. I think this path is more flexible design wise, as we can change the box based on children, and not just CSS. We also already do this with PropertyList, Forms, ObjectList, and Header. `setCollapsed` is added to ObjectBox to all children objects to bleed to the edges (like diffs).
Test Plan: I did a grep of `appendChild($table)` as well as searches for `PHUIObjectBoxView`, also with manual opening of hundreds of files. I'm sure I missed 5-8 places. If you just appendChild($table) nothing breaks, it just looks a little funny.
Reviewers: epriestley, btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12955
Summary: Fixes T8264. Broken in D12929. Sweep all the applyBuiltin implementations and always break; rather than return
Test Plan: added myself to a project successfully (showed up as a member)
Reviewers: epriestley, chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403, T8264
Differential Revision: https://secure.phabricator.com/D12940
Summary: Ref T6403. This was actually simple stuff.
Test Plan: changed the edit policy of a paste. changed the edit and join policy of a phame blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12933
Summary: Ref T6403. Conpherence keeps track of comments for message counts so we needed some special attention there. Otherwise, straight-forward.
Test Plan: left a comment on a diff with inline comments. sent messages in conpherence successfully. verified unread count incremented correctly for sent messages for users.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12932
Summary: Ref T6403. This one was pretty easy since no one does anything custom with subscribers.
Test Plan: subscribed / unscribed to a random commit ("audit"). joined / left, watched / unwatched a project
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12930
Summary: Ref T6403. This does TYPE_EDGE since I just had to deal with T8252. Look like this fixes a few editors (maybe) that would have had fatals with mentions like slowvote and ponder.
Test Plan: made a phame post mentioning a task and it worked! joined / left a project, watched / unwatched a project and that worked! blind faith for other sites.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12929
Summary:
Fixes T7977.
- Move Indexed Languages and See Symbols From config to Repository
- Make symbol search skip projects
This also makes the default languages to Everything instead of Nothing.
Test Plan:
- Browse files, click symbols.
- Use quick search to find symbols
- Browse revision, click symbols
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12687
Summary:
Fixes T6956. Before this change, we called PhabricatorUser::getOmnipotentUser in the various delete methods to query the data. Now, we use $engine->getViewer(), since its always a good thing to have less calls to PhabricatorUser::getOmnipotentUser thrown around the codebase.
I used the "codemod" tool to audit the existing calls to PhabricatorDestructorEngine (all of them) so ostensibly this gets all the spots. If I missed something though, its still going to work, so this change is very low risk.
Test Plan: ./bin/remove destroy P1; visit P1 and get a 404
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6956
Differential Revision: https://secure.phabricator.com/D12866
Summary:
New, cleaner, ObjectItemLists. Lots of minor style tweaks, basic overview:
- Remove FootIcons
- Remove Stackable
- Remove Plain List
- Add StatusIcon
- Add setting ObjectList to an ObjectBox
- Minor retouches to Headers
Mostly, this should give us an idea of life with the new Object Lists. I'll take another application by application pass down the road. This mostly looks at implementation in Maniphest, Differential, Audit, Workboards. Checked a few other areas and dialogs while testing, and everything looks square.
Test Plan: Maniphest, Differential, Homepage, Audit, People, and other applications. Drag reorder, etc.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12865
Summary:
Ref T7707. Handles currently have a "status" field and a "disabled" field.
The "status" field has these possible values: "open", "closed", "1", "2". durp durp durp
Instead, do:
- status = <open, closed>
- availability = <full, partial, none, disabled>
I think these make more sense? And are a bit more general? And use the same kind of constants for all values!
Test Plan: Looked at all affected handles in all states (probably).
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12832
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Ref T7447. Fixes T7600. This likely needs significant adjustment, but implements content-aware comment porting for line changes.
Specifically, this moves lines around to adjust their position considering added and removed lines between the diffs and across rebases.
It does not try to do any actual content (line against line) matching.
Test Plan:
- Unit tests.
- Poking around in the web UI seems to generate mostly reasonable-ish results?
- This may be a huge step backward in some cases that I just haven't hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7600, T7447
Differential Revision: https://secure.phabricator.com/D12741
Summary: Ref T7776. This could get better, but I think I got most of the big stuff. It's ~4x faster now.
Test Plan:
Before:
{F393338}
After:
{F393339}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7776
Differential Revision: https://secure.phabricator.com/D12730
Summary: Ref T7447. At least some users dislike this feature so strongly that they'd prefer not to have it at all.
Test Plan: Viewed ghosts; toggled preference, no more ghosts.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12704
Summary: See D12702.
Test Plan: Made something a link and clicked it, seemed to work OK.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim, epriestley
Differential Revision: https://secure.phabricator.com/D12703
Summary: Closes T7928, E{id} is available via global search.
Test Plan: Create calendar event, search for its monogram in global search, event should be accessible.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7928
Differential Revision: https://secure.phabricator.com/D12581
Summary: Seems reasonable? At least, it always matches however a user might think about documents (app or document). Unclear if "Diffusion" for example, are actually needed.
Test Plan: tested searching for "phriction", "wiki", "document", etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12577
Summary: "Authors" and "Subscribers" convert easily without any extra work.
Test Plan: Used both fields; used functions and normal tokens.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12556
Summary:
See M1433. Fixes T7266. Fixes T4475. Ref T7314.
Future work/notes/etc:
- Write the User Guide (see TODO).
- This might needs some design tweaks -- I think it's functionally almost-equivalent to the mock, but the UI isn't quite the same.
- (Mobile design is a touch off-looking I think?)
- When you use a custom query, the duplicate "magnifying glass" icons are a little weird. Maybe change one or the other.
- Maybe worth adding an "Open Documents in Current Application" option? Planning to wait for feedback on that.
- Need a Quicksand integration to change the current application at some point.
- Searching in "Current Application" from, e.g., the 404 page just searches all documents. Current plan is to just document this behavior, since the icon is a pretty good callout and it seems plausible that this is intuitive enough that users won't have a hard time with it.
Test Plan:
New dropdown:
{F379150}
Device-ish:
{F379151}
Normal search (current application, from maniphest, selects tasks):
{F379153}
Application search from non-application:
{F379154}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: johnny-bit, epriestley
Maniphest Tasks: T7266, T7314, T4475
Differential Revision: https://secure.phabricator.com/D12509
Summary:
Ref T7447. Ref T7870.
When a ghostly inline appears on the page, the timeline isn't currently aware that it's present, so it links elsewhere.
Instead, apply adjustments before rendering the timeline.
Ref T5030. This makes the behaviors in T5030 irrelevant most of the time.
Test Plan:
Before:
{F378106}
After:
- Inlines visible on page are linked directly.
- Inlines which we can't port (e.g., on files not present on current page) are still linked off-page.
- Used "Show Older" to page through and verify consistent rendering.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T5030, T7870, T7447
Differential Revision: https://secure.phabricator.com/D12497
Summary:
Ref T7447. Ref T7870. When a diff affects more than 100 files, we collapse files by default, then expand files with inlines.
Ghostly inlines currently don't cause files to expand, but reasonably should.
Test Plan:
Before:
{F378065}
After:
{F378066}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12495
Summary:
Ref T7447. Ref T7870.
- Forward: When rendering some file "B" which was moved from "A", port ghosts for both "A" and "B" to it.
- Backward: When porting a comment on "X" which was moved from some "Y", allow it to port to a file named "Y" if it can't find a file named "X".
Test Plan:
Before:
{F377809}
After:
{F377806}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12493
Summary:
Ref T7447. Ref T7870. See T7870 for a detailed description of this issue.
NOTE: Replying to these inlines from the UI still does the wrong thing, because we use the database line number, not the UI line number.
Test Plan:
Before:
{F377732}
After:
{F377733}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12491
Summary:
Ref T7447. This might be overkill, but I want to over-explain things until we have more confidence that this is rarely confusing.
NOTE: I'm playing it a bit fast and loose with `setIsGhost()` (passing a dictionary) because making API changes requires changing the interface and Diffusion, which is a pain. I'll clean this up at the end once the interface is more final. This is well-contained for now.
Test Plan:
- Viewed "base vs 2" in a diff with 3 diffs, saw some "older comments" and some "newer comments".
- Hovered the tags for an explanation of comment spookiness.
{F377703}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12490
Summary:
Ref T7447. After compiling inlines which will appear on the changeset, remove inlines which
Later stages remove these anyway, so it doesn't change anything to keep them around, but we can filter them out here cheaply.
This will also let us drive the Differential timeline view with the same logic a few diffs from now, to improve how it renders inlines. Generalize things a little bit.
Test Plan:
- Made a comment on the left of diff 1.
- Made diff 2.
- Viewed diff 2 vs diff 1.
- Verified old-left comment was filtered out by the new loop.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12488
Summary:
Ref T7447. This ports comments forward and backward in the best case:
- The old comment is on a changeset with the same filename.
- The old and new files are pretty much the same, line-for-line.
This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice.
Errata:
- Design is me cobbling something together, probably worth tweaking.
- "Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff.
Test Plan: {F377214}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: johnny-bit, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12484
Summary:
Ref T7447. This class is currently a big mess with a lot of `withWeirdSpecialThingUsedInOnePlace()` type qualifiers.
Try to generalize/normalize it a bit.
Test Plan:
- Viewed inline comments.
- Created a new inline comment.
- Edited an inline comment.
- Marked an inline comment complete.
- Deleted, then undeleted an inline comment.
- Previewed inline comments.
- Viewed drafts as another user, verified they don't show up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12483
Summary:
Ref T4100. This just makes the "specify stuff in query parameters" workflow a little better:
- You can now do `?projects=differential,diffusion`.
- You can now do `?projects=projects(alincoln)`.
Test Plan: Did that stuff ^^^^
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12468
Summary:
Ref T4100. Ref T5595. This implements these fields in one mega-field:
- Projects
- Not in projects
- In any project
- Include results in no projects
- In users' projects
Hopefully, this is a step in the right direction.
Test Plan: {F375555}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, chad, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12463
Summary:
Ref T4100. I can simplify the logic a bit here by moving some rendering into the datasources, but a few TokenizerControls currently don't have datasources.
Require datasources and always provide datasources.
Test Plan:
- Used previously-datasourceless controls (e.g., "Add Reviewers").
- Used normal controls.
- Manually verified that no other controls are missing datasources.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12456
Summary:
Ref T4100. Ref T5595.
To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.
With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.
For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.
For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.
This causes no behavioral changes.
Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, hach-que, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12453
Summary: Ref T4100. This restores the simpler behavior. See discussion in T4100#107445
Test Plan: Used Differential search, saw my token.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12447
Summary:
Ref T4100. This is still a bit rough around the edges, but mostly does what we're after.
- Implements viewer() and members(...) functions.
- The new browse workflow makes these discoverable.
Test Plan: {F374201}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12444
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.
Test Plan: Sorted and paged results in various applications.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12378
Summary:
Ref T7803. Removes some getReversePaging().
This also fixes `null` column handling, by adding an explicit `'null'` key with possible values "head" (put NULL before other values) or "tail" (put NULL after other values).
Maniphest has some glitchiness in paging through NULLs right now, but I believe it's all pre-existing and will be resolved when it fully converts. Diffusion is fully converted and pages through NULL correctly.
Test Plan:
- Failed to identify any reason for ChangesetQuery to reverse paging.
- Paged thorugh Diffusion.
- Paged through Maniphest.
- Maniphest has some issues when paging inside a NULL section, but these issues are preexisting and will be resolved later in this change sequence.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12371
Summary:
Ref T7803. This is a performance hack, not a real order, and isn't really meaningful or pageable.
After D12158, we constraint his query on `dateModified` anyway, which should generally give the database a relatively small result set to examine.
Test Plan: Browsed Differential and Diffusion. Checked query plan, it didn't look too crazy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12361
Summary: Ref T7803. Reduce the amount of code we're trusting to build SQL queries.
Test Plan:
- Paged through results in Maniphest, Differential and Diffusion.
- Some of the NULLable groups in Maniphest are a bit funky but this was preexisting.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12353
Summary:
Fixes T7731. When a user writes a "Send me an email" rule, always try send them an email, even if their notification settings would normally downgrade it to a notification.
In particular, this is stronger than these downgrades:
- Downgrades due to "self actions";
- downgrades due to "mail tags".
Test Plan:
- Wrote various Herald rules with "Send me an email" rules.
- Used `bin/mail list-outbound` / `show-outbound` to vet generated mail.
- Mail reacted properly to a variety of conditions (disabled accounts, settings, "send me an email" rule, forced delivery).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12300
Summary:
Ref T7731. For no particular reason, we currently put `ruleID` and `rulePHID` on `HeraldEffect` objects.
Pretty much all callers need the `HeraldRule` objects instead, and some go to great lengths to get them.
Just attach the `Rule` objects.
Test Plan: Will test thoroughly after next-ish changeset.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12269
Summary:
Ref T7731. Every adapter subclass currently implements this effect in an essentially identical way.
Some day far from now the effects will be modular and this mess will vanish completely, but reduce its sprawl for now.
Test Plan: I'll test this thoroughly at the end of the change sequence since writing rules is a pain.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12268
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.
- Reduce code duplication between existing receivers.
- Expose receiving objects in help menus where appropriate.
- Connect some "TODO" receivers.
Test Plan:
- Sent mail to every supported object type.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12249
Summary: Ref T7199. This makes the page look less janky and provides more context about how mail commands work and how to use them.
Test Plan: {F355959}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12245
Summary:
Ref T7199. Convert the single help menu item into a dropdown and allow applications to list multiple items there.
When an application has mail command objects, link them in the menu.
Test Plan:
{F355925}
{F355926}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12244
Summary: Ref T7199. This needs some polish and isn't reachable from the UI, but technically has all of the information.
Test Plan:
{F355899}
{F355900}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12241
Summary: Ref T7199. Convert Differential to modern modular commands.
Test Plan: Used `bin/mail receive-test` to send command and comment mail to Differential.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12239
Summary:
Ref T7199. In the vein of D12231, these options were a bad idea.
- They once served a very narrow, Facebook-specific need (see T1992), except even Facebook only used the Differential setting AFAIK.
- Outside of that special case, they are unused and essentially unusable (generally speaking, they do not meaningfully implement anything modular or replaceable).
- I have no knowledge of any install ever changing these settings, and can imagine no reason why they would.
Moving forward:
- If they really need to, they can fork locally and chagne one line.
- I expect "!actions" to make mail at least somewhat more modular soon, anyway.
- Any derived handlers would break after T7199 and need to be rewritten anyway, so this is just taking advantage of a BC break to do cleanup.
Test Plan:
- Grepped for removed configuration.
- Sent some mail from applications, verified the reply handlers set proper reply addresses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12233
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.
- There is zero reason to ever set these to different things.
- Unsurprisingly, I don't know of any install which has them set to different things.
Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.
We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.
Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12231
