1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

106 commits

Author SHA1 Message Date
epriestley
a641ec82a3 Add an "Authority" control to Packages to support "Watcher" packages
Summary: See T13657. An install has "watcher" packages which should not allow owners to "Force Accept" other packages.

Test Plan:
  - Created package A, which I own, on "/", with "Weak" authority.
  - Created package B, which I do not own, on "/src".
  - Created a revision which touches "/src" and added package B as a reviewer.
  - Attempted to accept the revision...
    - Before patch: permitted to "Force Accept" for package B.
    - After patch: not allowed to "Force Accept" for package B.
  - Verified that setting package "A" back to "Strong" authority allows a force-accept for package B.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21674
2021-06-25 13:48:46 -07:00
epriestley
2abf292821 Fix an issue where editing paths in Owners packages could raise an error: undefined index "display"
Summary:
Fixes T13464. Fully-realized paths have a "path" (normalized, effective path) and a "display" path (user-facing, un-normalized path).

During transaction validation we build ref keys for paths before we normalize the "display" values. A few different approaches could be taken to resolve this, but just default the "display" path to the raw "path" if it isn't present since that seems simplest.

Test Plan: Edited paths in an Owners package, no longer saw a warning in the logs.

Maniphest Tasks: T13464

Differential Revision: https://secure.phabricator.com/D20923
2019-11-21 11:29:04 -08:00
epriestley
53f8ad14fa Fix an issue in Owners where a transaction change could show too many effects
Summary:
Fixes T13324. Ref PHI1288. Currently, if you edit an Owners package that has some paths with no trailing slashes (like `README.md`) so their internal names and display names differ (`/README.md` display, vs `/README.md/` internal), the "Show Details" in the transaction log shows the path as re-normalized even if you didn't touch it.

Instead, be more careful about handling display paths vs internal paths.

(This code on the whole is significantly less clear than it probably could be, but this issue is so minor that I'm hesitant to start ripping things out.)

Test Plan:
  - In a package with some paths like `/src/` and some paths like `/src`:
  - Added new paths.
  - Removed paths.
  - Changed paths from `/src/` to `/src`.
  - Changed paths from `/src` to `/src/`.

In all cases, the "paths" list and the transaction record identically reflected the edit in the way I expected them to.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13324

Differential Revision: https://secure.phabricator.com/D20596
2019-06-20 16:08:31 -07:00
epriestley
8fab8d8a18 Prepare owners package audit rules to become more flexible
Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.

PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.

Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.

Test Plan:
  - Created several packages, some with and some without auditing.
  - Inspected database for: package state; and associated transactions.
  - Ran the migrations.
  - Inspected database to confirm that state and transactions migrated correctly.
  - Reviewed transaction logs.
  - Created and edited packages and audit state.
  - Viewed the "Package List" element in Diffusion.
  - Pulled package information with `owners.search`, got sensible results.
  - Edited package audit status with `owners.edit`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20124
2019-02-07 15:38:12 -08:00
epriestley
9f5e6bee90 Make the default behavior of getApplicationTransactionCommentObject() "return null" instead of "throw"
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.

Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.

This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.

Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.

Test Plan:
  - Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
  - Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jbrownEP

Differential Revision: https://secure.phabricator.com/D20121
2019-02-07 14:56:38 -08:00
epriestley
ee24eb60b7 In Owners Packages, make the API representation of the "Auditing" field more consistent
Summary:
Ref T13244. See PHI1047. A while ago, the "Review" field changed from "yes/no" to 20 flavors of "Non-Owner Blocking Under A Full Moon". The sky didn't fall, so we'll probably do this to "Audit" eventually too.

The "owners.search" API method anticipates this and returns "none" or "audit" to describe package audit statuses, so it can begin returning "audit-non-owner-reviewers" or whatever in the future.

However, the "owners.edit" API method doesn't work the same way, and takes strings, and the strings have to be numbers. This is goofy and confusing and generally bad.

Make "owners.edit" take the same strings that "owners.search" emits. For now, continue accepting the old values of "0" and "1".

Test Plan:
  - Edited audit status of packages via API using "none", "audit", "0", "1" (worked), and invalid values like "quack" (helpful error).
  - Edited audit status of packages via web UI.
  - Used `owners.search` to retrieve package information.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20091
2019-02-05 14:07:23 -08:00
epriestley
11cf8f05b1 Remove "getApplicationTransactionObject()" from ApplicationTransactionInterface
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).

The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.

The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.

So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.

Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.

Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19920
2018-12-20 15:16:19 -08:00
epriestley
937e88c399 Remove obsolete, no-op implementations of "willRenderTimeline()"
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.

All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.

Test Plan: Used `grep willRenderTimeline` to find callsites, found none.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19919
2018-12-20 15:04:49 -08:00
epriestley
4a98e0ff65 Allow Owners packages to be configured to ignore generated paths in Differential
Summary:
Depends on D19427. Ref T13130. See PHI251. Support configuring owners packages so they ignore generated paths.

This is still a little rough. A couple limitations:

  - It's hard to figure out how to use this control if you don't know what it's for, but we don't currently have a "CheckboxesEditField". I may add that soon.
  - The attribute ignore list doesn't apply to Diffusion, only Differential, which isn't obvious. I'll either try to make it work in Diffusion or note this somewhere.
  - No documentation yet (which could mitigate the other two issues a bit).

But the actual behavior seems to work fine.

Test Plan:
  - Set a package to ignore paths with the "generated" attribute. Saw the package stop matching generated paths in Differential.
  - Removed the attribute from the ignore list.
  - Tried to set invalid attributes, got sensible errors.
  - Queried a package with Conduit, got the ignored attribute list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19428
2018-05-05 08:47:29 -07:00
epriestley
dc510354c3 Remove explicit "mailKey" from Owners packages
Summary:
Depends on D19426. Ref T13130. Ref T13065. While I'm making changes to Owners for "Ignore generated paths", clean up the "mailKey" column.

We recently (D19399) added code to automatically generate and manage mail keys so we don't need a ton of `mailKey` properties in the future. Migrate existing mail keys and blow away the explicit column on packages.

Test Plan: Ran migration, manually looked at the database and saw sensible data. Edited a package to send some mail, which looked good.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130, T13065

Differential Revision: https://secure.phabricator.com/D19427
2018-05-05 08:47:08 -07:00
epriestley
5e2af4b9b5 Prepare to support an "Ignore generated files" flag in Owners
Summary:
Depends on D19425. Ref T13130. See PHI251. Now that changesets have a durable "generated" attribute, we can let owners packages check it when we're computing which packages are affected by a revision.

There's no way to actualy configure a package to have this behavior yet.

Test Plan:
  - Created a revision affecting a generated file and a non-generated file.
    - When I faked `mustMatchUngeneratedPaths()` to `return true;`, saw the non-generated file get no packages owning it.
    - Normally: lots of packages owning it).
  - Created a revision affecting only generated files.
    - When I faked things, saw no Owners actions trigger.
    - Normally: some packages added reviewers or subscribers.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19426
2018-05-05 08:46:47 -07:00
epriestley
7915445543 Fix two issues with Differential updates and Owners
Summary:
Ref T13114.

  - Followup fix for D19267, which didn't work correctly with //new// revision creation.
  - Followup fix for changes in T11015. Some of the querying logic was still handling "/x.y" and "/x.y/" differently. Instead, normalize consistently to "/x.y/"

Test Plan:
  - Created a new revision cleanly.
  - Created a package owning only a `example.txt` file and saw Differential find it as an owning package in the table of contents.

Maniphest Tasks: T13114

Differential Revision: https://secure.phabricator.com/D19268
2018-03-29 11:32:23 -07:00
epriestley
9d0cf3c8b8 Before anyone notices, break the API
Summary: See PHI439. Use slightly richer "dominion" return values for consistency.

Test Plan: Fetched results with `owners.search` API method.

Differential Revision: https://secure.phabricator.com/D19208
2018-03-09 12:21:18 -08:00
epriestley
3e992c6713 Add audit, review, and dominion information to "owners.search" API method
Summary:
See PHI439. This fills in additional information about Owners packages.

Also removes dead `primaryOwnerPHID`.

Test Plan: Called `owners.search` and reviewed the results. Grepped for `primaryOwnerPHID`.

Differential Revision: https://secure.phabricator.com/D19207
2018-03-09 12:11:13 -08:00
epriestley
516aaad341 Use "pathIndex" in some owners package queries to improve query plans
Summary: Depends on D19184. Ref T11015. Now that we have a digest index column, we can improve some of the queries a bit.

Test Plan:
  - Ran queries from revision pages before and after with and without EXPLAIN.
  - Saw the same results with much better EXPLAIN plans.
  - Fragment size is now fixed at 12 bytes per fragment, so we can shove more of them in a single query.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19185
2018-03-06 20:33:18 -08:00
epriestley
df1e9ce646 Treat Owners paths like "/src/backend" and "/src/backend/" identically
Summary:
Depends on D19183. Ref T11015. Currently, adding a trailing slash works great and omitting it mysteriously doesn't work.

Store a normalized version with an unconditional trailing slash for the lookup logic to operate on, and a separate display version which tracks what the user actually typed.

Test Plan:
  - Entered "/src/main.c", "/src/main.c/", saw them de-duplicate.
  - Entered "/src/main.c", saw it stay that way in the UI but appear as "/src/main.c/" internally.
  - Added a rule for "/src/applications/owners" (no slash), created a revision touching paths in that directory, saw Owners fire for it.
  - Changed the display value of a path only ("/src/main.c" to "/src/main.c/"), saw the update reflected in the UI without any beahvioral change.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19184
2018-03-06 20:31:46 -08:00
epriestley
adde4089b4 Allow owners paths to be arbitrarily long and add storage for display paths
Summary:
Depends on D19182. Ref T11015. This changes `path` from `text255` to `longtext` because paths may be arbitrarily long.

It adds `pathDisplay` to prepare for display paths and storage paths having different values. For now, `pathDisplay` is copied from `path` and always has the same value.

Test Plan:
  - Ran migration, checked database for sanity (all `pathDisplay` and `path` values identical).
  - Added new paths, saw `pathDisplay` and `path` get the same values.
  - Added an unreasonably enormous path with far more than 255 characters.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19183
2018-03-06 20:31:22 -08:00
epriestley
8cb273a053 Add a unique key to OwnersPath on "<packageID, repositoryPHID, pathIndex>"
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
2018-03-06 20:30:59 -08:00
epriestley
1bf4422c74 Add and populate a pathIndex column for OwnersPath
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
2018-03-06 20:30:33 -08:00
epriestley
d14a0f4787 Add "All" and "With Non-Owner Author" options for all Owners Package autoreview rules
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
2018-03-06 19:01:58 -08:00
epriestley
01b1854fb4 Fix an issue with attempting to index comments on packages
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
2017-10-20 09:38:45 -07:00
Dmitri Iouchtchenko
9bd6a37055 Fix spelling
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
2017-10-09 10:48:04 -07:00
epriestley
c9152b586b Support Ferret engine in Owners
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
2017-09-07 13:23:46 -07:00
epriestley
9f8907ccf7 Make Owners behavior when multiple packages own the same path with different dominion rules more consistent
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
2017-06-01 16:42:09 -07:00
epriestley
d2baa88171 Allow Owners packages to have arbitrarily long names
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
2017-04-27 18:04:03 -07:00
epriestley
3bea3fbb12 When computing revision ownership, cache some intermediate results for performance
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
2017-02-27 09:11:57 -08:00
Mike Riley
8247edff98 Modularize Owners package transactions
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
2016-10-13 21:07:02 +00:00
epriestley
7f6fa28363 When loading packages affected by a change to a particular path, ignore archived packages
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
2016-09-16 14:02:53 -07:00
epriestley
991e49b711 Correct spelling of "therefore"
Summary: Fixes T11565.

Test Plan: `git grep -i therefor | grep -vi therefore`

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11565

Differential Revision: https://secure.phabricator.com/D16476
2016-08-30 14:09:05 -07:00
epriestley
d1eed54d85 Fix expansion of projects into lists of user PHIDs
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
2016-05-24 06:38:37 -07:00
epriestley
809c7bf996 Allow users to manage package dominion rules
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
2016-05-17 10:57:43 -07:00
epriestley
6cb2bde48d Add "Dominion" rules for Owners Packages
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
2016-05-17 10:57:06 -07:00
epriestley
332d787dc8 Support "Review Changes" and "Block Changes" settings for Owners package "Auto Review"
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
2016-05-13 17:22:36 -07:00
epriestley
52ac242eb3 Implement "Auto Review" in packages with a "Subscribe" option
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
2016-05-13 17:21:58 -07:00
epriestley
547abfe873 Make packages mailable and subscribable
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
2016-05-13 17:18:57 -07:00
epriestley
9abc16df4d Give Owners packages the "O" monogram
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
2016-05-13 17:18:15 -07:00
epriestley
2a3c3b2b98 Provide bin/nuance import and ngram indexes for sources
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
2016-03-08 10:30:24 -08:00
epriestley
71ee97d74f Give Owners real view and edit policies
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
2016-02-15 11:56:35 -08:00
epriestley
373ff7f9d4 Read materialized project members instead of real members
Summary:
Ref T10010. This will allow us to find superprojects with `withMemberPHIDs(...)` queries.

  - Copy all the current real member edges to materialized member edges.
  - Redirect all reads to look at materialized members.
  - This table is already kept in sync by earlier work with indexing.

Basically, flow is:

  - Writes (joining, leaving, adding/removing members) write to the real member edge type.
  - After a project's members change, they're copied to the materialized member edge type for that project and all of its superprojects.
  - Reads look at materialized members, so "Parent" sees the members of "Child" and "Grandchild" as its own members, but we still have the "real members" edge type to keep track of "natural" or "direct" members.

Test Plan:
  - Ran migration.
  - Ran unit tests.
  - Saw the same projects as projects I was a member of.
  - Added some `var_dump()` stuff to verify the Owners changed.
  - Used `grep` to look for other readers of this edge type.
  - Made some project updates.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10010

Differential Revision: https://secure.phabricator.com/D14893
2015-12-27 09:26:27 -08:00
epriestley
96fe8c0b83 Implement basic ngram search for Owners Package names
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
2015-12-22 08:00:33 -08:00
epriestley
5cb0de1efc Restore "Create" transactions
Summary:
Ref T10004. This restores "alice created this task." transactions, but in a generic way so we don't have to special case one of the other edits with an old `null` value.

In most cases, creating an object now shows only an "alice created this thing." transaction, unless nonempty defaults (usually, policy or spaces) were adjusted.

Test Plan: Created pastes, tasks, blogs, packages, and forms. Saw a single "alice created this thing." transaction.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10004

Differential Revision: https://secure.phabricator.com/D14820
2015-12-18 11:56:03 -08:00
epriestley
0a50219f1b Formalize custom Conduit fields on objects
Summary: Ref T9964. This just adds more structure to application fields, to make it harder to make typos and easier to validate them later.

Test Plan: Viewed APIs, called some APIs, saw good documentation and correct results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9964

Differential Revision: https://secure.phabricator.com/D14776
2015-12-14 11:54:13 -08:00
epriestley
10cdc55cf7 Give "owners.search" a "paths" attachment and a default "owners" value
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
2015-12-14 11:53:50 -08:00
epriestley
3db175f79d Add a "content" attachment for Pastes for Conduit API
Summary: Ref T9964. Builds on D14772. Allows callers to get the raw content of pastes as an attachment.

Test Plan:
  - Read docs.
  - Executed attachment query.
  - Saw raw paste content.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9964

Differential Revision: https://secure.phabricator.com/D14774
2015-12-14 11:53:32 -08:00
epriestley
9499987cfe Add "owners.search" Conduit API endpoint, with CustomField support
Summary:
Ref T9964. Adds a new-style "owners.search" endpoint, and an extension for customfields.

Puts enough indirection in place to give us nice, consistent "custom.key" user-facing keys instead of "std:custom:owners:na0shf9a8dfdsafl" junk.

Test Plan:
  - Searched Owners via API.
  - Searched by ID.
  - Ordered by custom fields.
  - Reviewed API docs.
  - Used normal search with ordering.
  - Viewed custom field values in search results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9964

Differential Revision: https://secure.phabricator.com/D14758
2015-12-13 02:11:59 -08:00
epriestley
7c98cd85fe Implement DestructibleInterface for Owners Packages
Summary:
Fixes T9945. This is straightforward.

The two sub-object types are very lightweight so I just deleted them directly instead of loading + delete()'ing (or implementing DestructibleInterface on them, which would require they have PHIDs).

Also improve a US English localization.

Test Plan:
  - Used `bin/remove destroy PHID-... --trace` to destroy a package.
  - Verified it was gone.
  - Inspected the SQL in the log for general reasonableness.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9945

Differential Revision: https://secure.phabricator.com/D14729
2015-12-10 07:04:06 -08:00
epriestley
a407b83dc2 Move Owners to EditEngine
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
2015-12-02 05:21:15 -08:00
epriestley
5aae89babb Fix file PHID extraction in Owners and Differential
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
2015-11-17 08:36:50 -08:00
epriestley
f8080ce931 Add CustomField support to Owners
Summary: Fixes T9351. This is straightforward since this application is now relatively modern and doesn't have any bizarre craziness.

Test Plan:
{F787981}

{F787982}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9351

Differential Revision: https://secure.phabricator.com/D14093
2015-09-10 13:32:31 -07:00
Chad Little
e0faa66772 Allow Owners Packages to be archived
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
2015-08-18 13:36:05 -07:00