Summary:
Depends on D19181. Ref T11015. This nukes duplicates from the table if they exist, then adds a unique key.
(Duplicates should not exist and can not be added with any recent version of the web UI.)
Test Plan:
- Tried to add duplicates with web UI, didn't have any luck.
- Explicitly added duplicates with manual `INSERT`s.
- Viewed packages in web UI and saw duplicates.
- Ran migrations, got a clean purge and a nice unique key.
- There's still no way to actually hit a duplicate key error in the UI (unless you can collide hashes, I suppose), this is purely a correctness/robustness change.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19182
Summary: Ref T11015. This supports making path names arbitrarily long and putting a proper unique key on the table.
Test Plan:
- Migrated, checked database, saw nice digested indexes.
- Edited a package, saw new rows update with digested indexes.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19181
Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.
Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.
Maniphest Tasks: T13099, T11664
Differential Revision: https://secure.phabricator.com/D19180
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.
I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.
In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.
Then allow this header through for "Must Encrypt" mail.
Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19012
Summary: See rPcd14194a329788d5fff6365bcade278fd18f3612 for a similar change. Implement `getApplicationTransactionCommentObject()` to return `null` explicitly.
Test Plan: Ran `bin/search index --type ownerspackage`, got indexing after change.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18715
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary: Ref T12819. Same deal as before, but smaller diffs after D18559.
Test Plan: Indexed and searched for packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18564
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.
Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18077
Summary:
Fixes T12789. See that task for discussion. Currently, when multiple packages own the same path but have different dominion rules we get some weird/aribtrary/inconsistent results.
Instead, implement these rules:
- If zero or more weak and one or more strong packages claim a path, the strong packages (exactly) all own it.
- If one or more weak packages and zero strong packages claim a path, the weak packages all own it.
The major change here is that instead of keeping the //first// weak package we run into, we keep all the weak packages with the longest claim that we run into.
This needs to be implemented twice because Owners has two different near-copies of this logic, only one of which has test coverage. Some day maybe this will get fixed.
Test Plan:
- Added failing unit tests, made them pass.
- Viewed all A/B strong/weak combinations in Diffusion, saw sensible ownership results.
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Maniphest Tasks: T12789
Differential Revision: https://secure.phabricator.com/D18064
Summary:
- Change column type from `sort128` to `sort`.
- Remove `originalName`. This column is unused. Long ago, we used it to generate a `Thread-Topic` header for mail, but just use PHIDs now (the value just needs to be stable for a given object, users normally don't see it).
Test Plan:
- Created a package with a beautifully long name. Magnificent!
- Grepped for `originalName` / `getOriginalName()`, found no Owners hits.
- Verified that there isn't any name-length validation code to remove.
{F4925637}
Reviewers: chad, amckinley
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17798
Summary: Ref T12600. Basically all the property (not path) information on a hovercard for owner packages.
Test Plan:
Create a package with LOTS OF RULES. Test it as open and archived states.
{F4923441}
{F4923444}
Reviewers: epriestley, jmeador
Reviewed By: jmeador
Subscribers: jmeador, Korvin
Maniphest Tasks: T12600
Differential Revision: https://secure.phabricator.com/D17793
Summary: When a notice is in a table view in a two column layout, reset the margins.
Test Plan: Visit OwnerDetails with no paths set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17792
Summary:
Ref T12272. If you own a package which owns "/", this allows you to force-accept package reviews for packages which own sub-paths, like "/src/adventure/".
The default UI looks something like this:
```
[X] Accept as epriestley
[X] Accept as Root Package
[ ] Force accept as Adventure Package
```
By default, force-accepts are not selected.
(I may do some UI cleanup and/or annotate "because you own X" in the future and/or mark these accepts specially in some way, particularly if this proves confusing along whatever dimension.)
Test Plan: {F4314747}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12272
Differential Revision: https://secure.phabricator.com/D17569
Summary:
Ref T12319. With large datasets, the computation of which packages own paths in a revision is needlessly slow.
Improve performance through caching:
- Cache which paths belong to each repository.
- Cache the split fragments of each path.
- Cache the path fragment counts.
- Micro-optimize accessing `$this->path`.
Test Plan:
- Used `bin/lipsum` to generate 4,000 packages with 150,000 paths.
- Created a revision affecting 100 paths in `phabricator/` (these paths mostly overlap with `bin/lipsum` path rules, since Lipsum uses Phabricator-like rules to generate paths).
- Before optimizations, this revision spent about 5.5 seconds computing paths.
- After optimizations, it spends about 275ms.
{F3423414}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17424
Summary: Ref T10390. Simplifies dropdown by rolling out canUseInPanel in useless panels
Test Plan: Add a query panel, see less options.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17341
Summary:
Ref T10978. Currently, during commit import, we write an "Audit Not Required" auditor for commits which don't require an audit.
This auditor is used to power the "Commits in this package" query in Owners.
This conflates audits and commit/package membership. I think it might even predate edges. Code needs to dance around this mess and we get the wrong result in some cases, since auditors are now editable.
Instead, write an explicit edge which just says "this commit is part of such-and-such packages". Then use that to run the query. Logical!
I'll issue guidance on this but I'm not migrating it, since it fixes itself going forward and only really affects the UI in Owners.
Test Plan:
- Ran `bin/audit update-owners` with various arguments.
- Viewed packages in web UI, saw them load the proper commits.
- Queried by packages in Diffusion explicitly.
- Clicked the "View All" link in Owners and got to the right search UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17264
Summary:
Fixes T9430. Fixes T9362. Fixes T9544. This changes the default view of Audit to work like Differential, where commits you need to audit or respond to are shown in buckets.
This is a bit messy and probably needs some followups. This stuff has changed from a compatibility viewpoint:
- The query works differently now (but in a better, modern way), so existing saved queries will need to be updated.
- I've removed the counters from the home page instead of updating them, since they're going to get wiped out by ProfileMenu soon anyway.
- When bucketed queries return too many results (more than 1,000) we now show a warning about it. This isn't greaaaat but it seems good enough for now.
Test Plan: {F2351123}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9430, T9362, T9544
Differential Revision: https://secure.phabricator.com/D17192
Summary:
Fixes T6630. Long ago, "Audit", "Diffusion" and "Repositories" were three totally separate applications.
This separation isn't useful and the three rapidly became intertwined. Ideally, they would all be one application.
This doesn't take us quite that far, but Audit no longer has any controllers and has little actual behavior.
The "Audit" screen has always just been a SearchEngine view of commits with some filters on it, and this formalizes that and puts a link to it in Diffusion. (This view has other uses, too.)
Test Plan:
- Accessed audit from home page.
- Accessed audit/commits from Diffusion.
- Could no longer uninstall Audit on its own.
- Grepped for `/audit/` and `AuditApplication`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6630
Differential Revision: https://secure.phabricator.com/D17186
Summary: When a user queries by package monogram explicitly, search by package ID.
Test Plan: {F2305075}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17142
Summary: Ref T7643. When we send mail about a change to a package description, allow it to say "CHANGES TO PACKAGE DESCRIPTION" instead of "EDIT DETAILS". Smooth!
Test Plan: {F1909417}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16818
Summary: Converts Owners package transactions to modular transactions.
Test Plan:
- created a new package
- edited all simple properties from the web ui
- checked that project and user owners were added as reviewers appropriately to new diffs
- inspected the change details for various types of path add / remove / update / reorder changes
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16651
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:
Ref T11016. I think I inverted the meaning of this function by accident in D14893.
The intent is to return a list of users: direct users, and all members of all projects.
Prior to this patch actually returns direct users, and all projects they are members of.
Test Plan:
- Created "Project with Dog".
- Added user "dog" to project.
- Created package "X", owning file "/x", with audit enabled.
- Made "X" owned by "Project with Dog".
- Modified "/x" and had user "dog" accept it.
- Landed change.
- Prior to change: package "X" incorrectly added as auditor.
- After change: package "X" correctly omitted as auditor, because a member reviewed the change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11016
Differential Revision: https://secure.phabricator.com/D15971
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. This supports two settings for packages (although they can't be configured yet):
- **Strong Dominion**: If the package owns `a/`, it always owns every subpath, even if another package also owns the subpath. For example, if I own `src/differential/`, I always own it even if someone else claims `src/differential/js/` as part of the "Javascript" package. This is the current behavior, and the default.
- **Weak Dominion**: If the package owns `a/`, but another package owns `a/b/`, the package gives up control of those paths and no longer owns paths in `a/b/`. This is a new behavior which can make defining some types of packages easier.
In the next change, I'll allow users to switch these modes and document what they mean.
Test Plan:
- Ran existing unit tests.
- Added new unit tests.
Reviewers: chad
Reviewed By: chad
Subscribers: joel
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15935
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. These appear in "Subscribers" tokenizers now and we got a maybe slightly better icon in the last FA update: {icon shopping-bag} instead of {icon list-alt}.
(I don't feel strongly about this, the old icon just doesn't seem very evocative.)
Test Plan:
o.( O___O ).o
{F1311641}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15913
Summary:
Ref T10939. Fixes T7834.
- Make packages into mailable objects, like projects and users.
- Packages resolve recipients by resolving project and user owners into recipients.
Test Plan:
- Added a comment to a revision with a package subscriber.
- Used `bin/mail show-outbound` to see that owners got mail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7834, T10939
Differential Revision: https://secure.phabricator.com/D15912
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 isn't ideal because it's easy to confuse with zero ("O" vs "0") but I think this will mostly be read-only so it's probably one of the least-bad uses we could make of "O". We haven't really gotten into trouble with "I" (vs "1") for initiatives. Still, open to better ideas.
The goal here is to allow commit messages to include packages in some reasonable way, like `Reviewers: O123 Package Name, epriestley, alincoln`. The parser will ignore the "Package Name" part, that's just for humans. And I don't expect humans to type this, but when the use `arc diff --edit` or similar to update an //existing// revision, the reviewer needs to be represented somehow. It also needs to appear in the commit messages that `arc land` finalizes somehow.
I didn't hook up `/O123` as a URI, but this should do everything else I think.
Test Plan:
- Viewed package list.
- Viewed package detail.
- Did global search for `O12`.
- Used `O12` and `{O12}` remarkup rules.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15910
Summary:
Ref T10939. This is just a bug. I thought this was what was described in T10174 but that's actually talking about something completely different.
Also make a `<select />` slightly easier to use.
Test Plan:
- Created a package with auditing enabled.
- Pushed a change.
- Saw audit trigger.
- Disabled the package, pushed a change.
- Before patch: saw audit trigger improperly.
- After patch: restarted daemons, then saw audit correctly not trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15907
Summary: This is completely obsoleted by `owners.search`. See D15472.
Test Plan: Viewed API method in UI console.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15769
Summary: Brings the edit paths page in owners up to new UI
Test Plan: Edit some paths, yo.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15596
Summary: Cleans up EditEngine, adds new layout to EditEngine and descendents
Test Plan: Test creating a new form, reordering, marking and unmarking defaults. View new forms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15531
Summary:
Ref T10537. More infrastructure:
- Put a `bin/nuance` in place with `bin/nuance import`. This has no useful behavior yet.
- Allow sources to be searched by substring. This supports `bin/nuance import --source whatever` so you don't have to dig up PHIDs.
Test Plan:
- Applied migrations.
- Ran `bin/nuance import --source ...` (no meaningful effect, but works fine).
- Searched for sources by substring in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15436
Summary: Moves over everything except Maniphest, which has some special behavior.
Test Plan:
- Viewed a badge.
- Viewed a calendar event.
- Viewed a countdown.
- Viewed a Fund initiative.
- Viewed a Herald rule.
- Viewed a macro.
- Viewed an application.
- Viewed an owners package.
- Viewed a credential.
- Viewed a Ponder question.
- Viewed a poll.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15416
Summary: Clean up owners a bit, move to two columns.
Test Plan:
Review a package, edit paths, remove all paths. Archive.
{F1139351}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15388
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: Fixes T10360. In modern code, most of the meat is automatic.
Test Plan:
- Edited view policy and edit policy from web UI.
- Viewed package, saw policy badge in header.
- Tried to edit a package as a user without permission, got appropriate disabled states and errors.
- Changed policies via Conduit.
- Tried to view a package as a user without permission.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10360
Differential Revision: https://secure.phabricator.com/D15275
Summary: Ref T4245. This further reduces the reliance on callsigns in Diffusion.
Test Plan:
- Pretty reasonable test coverage already exists.
- Browsed repository list, browse view, history view, content view, change view, commit view, tag view, branch view of repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14937
