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: 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 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: 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 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: 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: 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
Summary: Ref T4245. These mostly relate to building URIs.
Test Plan: Tried to hunt down as many of these in the UI as I could. Some are a bit tricky but they should be low-risk.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14933
Summary: Ref T4245. These are all straightforward to remove.
Test Plan:
- Edited paths in a package.
- Ran `bin/audit delete --repositories ...` with various identifiers.
- Searched by repository for `R3`, `rAAAA` in Harbormaster.
- Did a Herald dry run on a commit.
- Browsed commits, made comments.
- Viewed a Releeph product list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14927
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. 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 T9482. This button goes to the wrong place and this table conditionally hides itself so I missed it.
Instead:
- Always show the table, with an empty string if there are no relevant commits.
- Link to a working UI.
Test Plan: Saw table. Clicked button.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9482
Differential Revision: https://secure.phabricator.com/D14189
Summary: You can already pass other icons, but this makes it a bit simpler.
Test Plan: Test Maniphest, Badges
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14131
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
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 T8099, functionally I prefer to be able to set anything 'table-like' with `setTable` for design consistency. This looses the restriction and did some light grepping for other missed cases.
Test Plan: Test new UI, grep for other missing cases.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13471
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 T8320. There's currently one enormous form; split it into a general information form (name, description, owners) and a paths form.
I think this is a little more manageable from both a UX point of view and from an "I have to convert this to use ApplicationTransactions" point of view.
Test Plan:
- Edited paths.
- Edited non-path information.
- Created new packages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8320
Differential Revision: https://secure.phabricator.com/D13026
Summary:
Ref T8320. Modernize search and major interfaces.
This slightly regresses some list view and search features; I'll probably restore some later (once the Query has proper `needX(...)` methods) and drop the rest.
Test Plan: Browsed, edited, deleted, and created packages.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8320
Differential Revision: https://secure.phabricator.com/D13024
Summary:
Ref T7803. Currently, available high-level orders are spread across Query and SearchEngine classes and implemented separately for each application.
Lift the concept of "builtin" (high-level, user-facing, named) orders (similar to "builtin" queries in ApplicationSearch) into the root Query class, and let it drive the SearchEngine implementation. This allows you to define a new order in one place and have it automatically work across the entire stack.
This will also let Conduit expose this information in a straightforward way.
Test Plan:
- Used ApplicationSearch in Diffusion.
- Used all result orderings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12379
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.
Test Plan:
- Sent a new message; used "To".
- I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
- OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^
Did not test:
- Lint is nontrivial to test locally, I'll test it in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T7689
Differential Revision: https://secure.phabricator.com/D12224
Summary: Fixes T7618. The "button" needs to be a PHUIButtonView later on.
Test Plan: Forced condition, loaded page, saw button instead of fatal.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7618
Differential Revision: https://secure.phabricator.com/D12108
Summary: Update Owners per current UI standards, add crumbs at each level, removed AphrontPanels, check spacing.
Test Plan: Tested a list of owner packages, editing a package, creating a package, and various filters.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11961
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
Summary: Ref T6822. This method needs to be `public` because it is called from `PhabricatorApplicationSearchController::buildApplicationMenu()`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11416
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within `PhabricatorController` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11241
Summary: Fixes T6834. Without this asort() call, repositories are ordered by ID, which is arbitrary to the user. This sorts them alphabetically by callsign.
Test Plan: edited a package and saw repos sorted by call sign
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6834
Differential Revision: https://secure.phabricator.com/D11095
Summary: Fixes T5387. I broke this in D8805. Fix it by using the application search parameters that D8805 introduced.
Test Plan: verified that the two links mentioned in T5387 worked for me. Also tried manual links on secure.phabricator.com and those showed the right data even...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5387
Differential Revision: https://secure.phabricator.com/D10817
Summary: Missed this in previous pass. Send these as links in HTML emails.
Test Plan: Register a new user that nees approval.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10815
