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
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
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
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
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
Summary:
Ref T13130. See <https://discourse.phabricator-community.org/t/unable-to-create-owners-package-with-same-path-in-multiple-repositories/1400/1>.
When you edit paths in Owners, we deduplicate similar paths, like `/x/y` and `/x/y/`. However, this logic currently only examines the paths, and incorrectly deduplicates the same path in different repositories.
Instead, consider the repository before deduplicating.
Test Plan:
- Edited an Owners package and added the path "/" in two different repositories.
- Before: only one surived the edit.
- After: both survived.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19420
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
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
