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: 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 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. 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. 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: 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: 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: Fixes T10058. We don't need to continue on this check if no path changes are being applied.
Test Plan: Archived an owners package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10058
Differential Revision: https://secure.phabricator.com/D14906
Summary:
Ref T9979. This uses ngrams (specifically, trigrams) to build a reasonably efficient index for substring matching. Specifically, for a package like "Example", with ID 123, we store rows like this:
```
< ex, 123>
<exa, 123>
<xam, 123>
<amp, 123>
<mpl, 123>
<ple, 123>
<le , 123>
```
When the user searches for `exam`, we join this table for packages with tokens `exa` and `xam`. MySQL can do this a lot more efficiently than it can process a `LIKE "%exam%"` query against a huge table.
When the user searches for a one-letter or two-letter string, we only search the beginnings of words. This is probably what they want, the only thing we can do quickly, and a reasonable/expected behavior for typeaheads.
Test Plan:
- Ran storage upgrades and search indexer.
- Searched for stuff with "name contains".
- Used typehaead and got sensible results.
- Searched for `aabbccddeeffgghhiijjkkllmmnnooppqqrrssttuuvvwwxxyyzz` and saw only 16 joins.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14846
Summary:
Ref T10004. Tweaks some of the UX a little to be more intuitive/inviting?
- Button says "Configure Form" instead of "Actions".
- Root list is less "developer-ey" and more "explain what this is for-ey".
Test Plan:
{F1028928}
{F1028929}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14808
Summary:
Ref T9964. Three goals here:
- Make it easier to supply Conduit documentation.
- Make automatic documentation for `*.edit` endpoints more complete, particularly for custom fields.
- Allow type resolution via Conduit types, so you can pass `["alincoln"]` to "subscribers" instead of needing to use PHIDs.
Test Plan:
- Viewed and used all search and edit endpoints, including custom fields.
- Used parameter type resolution to set subscribers to user "dog" instead of "PHID-USER-whatever".
- Viewed HTTP parameter documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14796
Summary:
Ref T9964.
- New mechanism for rich documentation on unusual/complicated edits.
- Add some docs to `paths.set` since it's not self-evident what you're supposed to pass in.
Test Plan: {F1027177}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14791
Summary: Ref T9964. Fixes T9752. Provides API access to enable/disable packages and change their paths.
Test Plan:
- Changed status via Conduit.
- Changed paths via Conduit.
- Tried to change a path use a nonsense/bogus repository PHID, got an error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9752, T9964
Differential Revision: https://secure.phabricator.com/D14790
Summary:
Ref T9964.
- Add a "paths" attachment for fetching paths.
- Always load owners. We will need this to do policy checks in the future, anyway, and this data is not large, is very useful, and is reasonable to load unconditionally.
Test Plan:
- Queried packages via API.
- Edited packages (paths, owners).
- Created a package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14775
Summary:
Ref T9132. Ref T9908. Fixes T5622. This allows you to copy some fields (projects, subscribers, custom fields, some per-application) from another object when creating a new object by passing the `?template=xyz` parameter.
Extend "copy" support to work with all custom fields.
Test Plan:
- Created new pastes, packages, tasks using `?template=...`
- Viewed new template docs page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5622, T9132, T9908
Differential Revision: https://secure.phabricator.com/D14699
Summary:
Ref T9132. Paste is in fairly good shape so Owners is up next. Reasoning:
- One install wants API access for it.
- It's a simple application for getting CustomFields working with EditEngine.
This only does the EditEngine part, so CustomFields are no longer editable until I make that work. That will be up next, and I'll hold this until that's ready.
Test Plan:
- Created and edited packages via web UI.
- Created and edited package editing forms via web UI.
- Created and edited packages via Conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14598
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: 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:
Ref T8320. This field doesn't have any special effects and has no reason to exist. Primary owners are always already owners.
This makes one minor implicit change to Owners: it is now possible to create a package with no owners. That seems fine; it is reasonable to create a package purely as an organizational tool and use it in, e.g., Herald.
Test Plan: Created and edited packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8320
Differential Revision: https://secure.phabricator.com/D13921
Summary:
Ref T8320. Ref T8004. This just tries to generally modernize
It also replaces the nonfunctional "Find Owners" link with a new property that just shows owning packages.
Test Plan:
- Created and edited packages.
{F720478}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8004, T8320
Differential Revision: https://secure.phabricator.com/D13911
Summary:
Ref T8320. Fixes T8317. Fixes T2831. Fixes T8073. Fixes T7127.
There was a bug with this line:
for ($ii = 0; $ii < count($paths); $ii++) {
...because the array may be sparse if there have been deletes, so `count($paths)` might be 3, but the real keys could be `1`, `5` and `6`. I think this was the primary issue behind T7127.
The old Editor did a lot of work to try to validate paths. When a path failed to validate, it silently discarded it. This was silly and pointless: it's incredibly bad UX; and it's totally fine if users saves "invalid" paths. This was likely the cause of T8317, and probably the cause of T8073.
T2831 I'm less sure about, but I can't reproduce it and I rewrote all the logic so I suspect it's gone.
This also records and shows edits, so if stuff does keep happening it should be more clear what's going on.
I removed some adjacent stuff:
- I removed the ability to delete packages. I'll add "disable" in a future diff, plus `bin/remove destroy`, like other objects. Getting rid of this now let me get rid of all the mail stuff.
- I removed "path validation" where packages would try to automatically update in response to commits. This doesn't necessarily make sense in Git/Mercurial, is sketchy, could easily have been the source of T2831, and seems generally complicated and not very valuable. We could maybe restore it some day, but I'd like to get Owners stable before trying to do crazy stuff like that.
Test Plan: {F437687}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8317, T8073, T7127, T2831, T8320
Differential Revision: https://secure.phabricator.com/D13032
Summary: Ref T7094. We basically need to make sure folks can see repositories before making owners packages about code within. This cleans up things a little bit by moving a bunch of logic out of the storage class and into an editor class.
Test Plan: made a package and it worked! deleted a package and it worked! discovered buggy behavior in more complicated edits and filed T7127; note this bug exists before and after this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11652
