Summary:
Ref T10967. There have been two different ways to load reviewers for a while: `needReviewerStatus()` and `needRelationships()`.
The `needRelationships()` stuff was a false start along time ago that didn't really go anywhere. I believe the idea was that we might want to load several different types of edges (subscribers, reviewers, etc) on lots of different types of objects. However, all that stuff pretty much ended up modularizing so that main `Query` classes did not need to know about it, so `needRelationships()` never got generalized or went anywhere.
A handful of things still use it, but get rid of them: they should either `needReviewerStatus()` to get reviewer info, or the ~3 callsites that care about subscribers can just load them directly.
Test Plan:
- Grepped for removed methods (`needRelationships()`, `getReviewers()`, `getCCPHIDs()`, etc).
- Browsed Diffusion, Differential.
- Called `differential.query`.
It's possible I missed some stuff, but it should mostly show up as super obvious fatals ("call needReviewerStatus() before getReviewerStatus()!").
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17518
Summary:
Ref T10967.
When we query for revisions with particular reviewers, use the new table to drive the query.
When we load revisions for use in the application, also use the new table to drive the query.
This doesn't convert everything: there's some old `loadRelationships()` stuff still using the old table. But this moves the major stuff over.
(This also changes the icon for "commented" from a question mark to a speech bubble.)
Test Plan:
- Viewed revision lists and detail views on old and new code, saw identical outcomes.
- Updated revisions, accepted/rejected/commented on revisions.
- Hit the "Accepted Older" and "Commented Older" states by taking an action and then updating.
- Grepped for removed methods (like `getEdgeData()` and `getDiffID()`).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17517
Summary:
Ref T10967. We have a "commented" state to help reviewers get a better sense of who is part of a discussion, and a "last action" state to help distinguish between "accept" and "accepted an older version", for the purposes of sticky accepts and as a UI hint.
Currently, these are first-class states, partly beacuse we were somewhat limited in what we could do with edges. However, a more flexible way to represent them is as flags separate from the primary state flag.
In the new storage, write them as separate state information: `lastActionDiffPHID` stores the Diff PHID of the last review action (accept, reject, etc). `lastCommentDiffPHID` stores the Diff PHID of the last comment (top-level or inline).
Test Plan: Applied storage changes, commented and acted on a revision. Saw appropriate state reflected in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17514
Summary:
Ref T10967. `differential.createcomment` is a frozen API method which has been obsoleted by `differential.revision.edit`.
It is the only remaining way to apply an "accept", "reject", or "resign" action using the old "ACTION" code.
Instead of using the old code, sneakly apply a new type of transaction in these cases instead.
Then, remove all the remaining old code for this stuff on the write pathways.
Test Plan:
- Used "differential.createcomment" to accept, reject, and resign from a revision.
- Grepped for all removed ACTION_X constants, found them only in rendering code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17513
Summary: Ref T10967. See that task for some discussion. This lets us do double writes on this pathway.
Test Plan: Set an Owners package to auto-review. Created revisions which triggered it: one with no reviewers (autoreview added); one with the package as a blocking reviewer explicitly (no automatic stuff happened, as expected).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17512
Summary:
Ref T10967. This converts the reviewer update action in Herald from an older edge write to a newer ModularTransactions write.
The major value from this is that we get a double-write to the new reviewers table.
Test Plan:
- Wrote a Herald rule to add a reviewer and a blocking reviewer.
- Saw them added properly to a revision with: no reviewers; both as blocking; A as blocking, B as nonblocking; A as nonblocking, B as blocking.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17511
Summary: Ref T5307. Actually check the built in query with query, not engine.
Test Plan: Try a builtin query, and a custom query when making a dashboard panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17521
Summary: Ref T5307. Adds a better query check query, sets required for the name, adds the correct URI for cancelling.
Test Plan: Test a form without a name, fake a query string, test cancel button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17520
Summary: Ref T5307. This adds an additional action to Use Results for creating a panel from the query.
Test Plan:
Navigate to Maniphest, select dropdown for Use Results. Try any of the following:
- Try to set a panel without a name (fail)
- Muck up query or engine (fail)
- Set a fake Dashboard ID (fail)
Give panel a name and select a dashboard I have edit permissions to, get taken to dashboard.
Reviewers: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17516
Summary: Fixes T12416. See that task for discussion. Slightly older versions of `git` do not appear to support use of `--` to separate flags and arguments.
Test Plan:
- Ran `bin/repository update PHABX`.
- In T12416, had a user with Git 2.1.4 confirm that `git ls-remote X` worked while `git ls-remote -- X` failed.
- Read `git help ls-remote` to look for any kind of suspicious `--destroy-the-world` flags, didn't see any that made me uneasy.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12416
Differential Revision: https://secure.phabricator.com/D17508
Summary:
Ref T5378. This repackages an existing check to see if a URI is a URI for the current install into a more reasonable form.
In an upcoming change, I'll use this new check to test whether `http://example.whatever.com/T123` is a link to a task on the current install or not.
Test Plan: This stuff has good test coverage already; added some more.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5378
Differential Revision: https://secure.phabricator.com/D17502
Summary:
Ref T12392. The logic currently goes like this:
- Try a fetch.
- If that fails, try repairing the origin URI.
- Then try again.
This is pretty complicated, and we can use this simpler logic instead:
- Set the origin URI to the right value.
- Try a fetch.
Setting the origin URI is very fast. This can normally only get us in any trouble in very obscure situations which haven't occurred for many years:
- Pretty much all of this is already covered by `verifyGitOrigin()`, which we run earlier.
- Origins could be configured to have multiple URIs for some reason, but shouldn't be.
- Years ago, you could configure Phabricator to point at a local repository it didn't own and that could conceivably have a different "origin" that you might not want us to delete. If you did this, the daemons have been spewing errors for 3-4 years without you fixing it. The cost of fixing the remote URI is very small even if anyone is affected by this (just set it back to the old value) and there's zero reason to do this and the scenario is ridiculous.
Test Plan: Ran `bin/repository update PHABX --trace --verbose`, saw fetches go through cleanly after URI adjustment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12392
Differential Revision: https://secure.phabricator.com/D17498
Summary:
Ref T12296. Ref T12392. Currently, when we're observing a remote repository, we periodically run `git fetch ...`.
Instead, periodically run `git ls-remote` (to list refs in the remote) and `git for-each-ref` (to list local refs) and only continue if the two lists are different.
The motivations for this are:
- In T12296, it appears that doing this is //faster// than doing a no-op `git fetch`. This effect seems to reproduce locally in a clean environment (900ms for `ls-remote` + 100ms for `for-each-ref` vs about 1.4s for `fetch`). I don't have any explanation for why this is, but there it is. This isn't a huge change, although the time we're saving does appear to mostly be local CPU time, which is good for us.
- Because we control all writes, we could cache `git for-each-ref` in the future and do fewer disk operations. This doesn't necessarily seem too valuable, though.
- This allows us to tell if a fetch will do anything or not, and make better decisions around clustering (in particular, simplify how observed repository versioning works). With `git fetch`, we can't easily distinguish between "fetch, but nothing changed" and "legitimate fetch".
If a repository updates very regularly we end up doing slightly more work this way (that is, if `ls-remote` always comes back with changes, we do a little extra work), but this is normally very rare.
This might not get non-bare repositories quite right in some cases (i.e., incorrectly detect them as changed when they are unchanged) but we haven't created non-bare repositories for many years.
Test Plan: Ran `bin/repository update --trace --verbose PHABX`, saw sensible construction of local and remote maps and accurate detection of whether a fetch would do anything or not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12392, T12296
Differential Revision: https://secure.phabricator.com/D17497
Summary: Ref T12270. Builds out a BadgeCache for PhabricatorUser, primarily for Timeline, potentially feed? This should still work if we later let people pick which two, just switch query in BadgeCache.
Test Plan: Give out badges, test timeline for displaying badges from handles and without queries. Revoke a badge, see cache change.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17503
Summary:
Fixes T12406. When importing commits, we automatically add auditors if the message lists "Auditors: username".
If the list of auditors includes the commit author, this edit fails because you can't audit your own commits (previously, you sometimes could and/or we didn't validate).
Instead, just ignore "Auditors: author".
Test Plan:
- Made a commit with "Auditors: epriestley".
- Pushed it.
- Saw the HeraldWorker get stuck with the error in T12406.
- Applied the change; worker now succeeded.
Reviewers: chad
Reviewed By: chad
Subscribers: alexmv
Maniphest Tasks: T12406
Differential Revision: https://secure.phabricator.com/D17507
Summary:
Fixes T12409. Config entries may be marked as "deleted", and `bin/config set --database` doesn't un-delete them, so the edit doesn't do anything.
The "most correct" fix here is to swap to transactions so we run the same code, but just fix this narrowly for now since it's one line of code.
Test Plan:
- Set `maniphest.default-priority` to `123`.
- Deleted `maniphest.default-priority` from the web UI by deleting all the text in the box.
- Before patch: `bin/config set --database maniphest.default-priority 789` had no effect.
- After patch: `bin/config set --database maniphest.default-priority 789` worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12409
Differential Revision: https://secure.phabricator.com/D17506
Summary: Extends PHUIListItemView to take an icon, link as an "Action Item" that displays on the right side of the menu link. Does not display on Favorites. This allows for adding edit, external, or other links (documentation?) to any menu item. Right now the secondary link is only visible when the item is selected. This feels right, but if we offer it in other ways, users may always want it visible. We could look at making it onhover.
Test Plan:
Add a bunch of random global and personal dashboards to my menu. Add a menu to Favorites, see no link. Test mobile, link works.
{F4136699}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17505
Summary:
Via HackerOne. When you view a raw file in Differential, we currently generate a permanent file with default permissions. This may be incorrect: default permissions may be broader than the diff's permissions.
The other three methods of downloading/viewing raw files ("Download" in Diffusion and Differential, "View Raw" in Diffusion and Differential) already apply policies correctly and generate temporary files. However, this workflow was missed when other workflows were updated.
Beyond updating the workflow, delete any files we've generated in the past. This wipes the slate clean on any security issues and frees up a little disk space.
Test Plan:
- Ran migration script, saw existing files get purged.
- Did "View Raw File", got a new file.
- Verified that the file was temporary and properly attached to the diff, with "NO ONE" permissions.
- Double-checked that Diffusion already runs policy logic correctly and applies appropriate policies.
- Double-checked that "Download Raw Diff" in Differential already runs policy logic correctly.
- Double-chekced that "Download Raw Diff" in Diffusion already runs policy logic correctly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17504
Summary:
Fixes T12400. Adds a "Has MFA" filter to People so you can figure out who you need to harass before turning on "require MFA".
When you run this as a non-admin, you don't currently actually hit the exception: the query just doesn't work. I think this is probably okay, but if we add more of these it might be better to make the "this didn't work" more explicit since it could be confusing in some weird edge cases (like, an administrator sending a non-administrator a link which they expect will show the non-administrator some interesting query results, but they actually just get no constraint). The exception is more of a fail-safe in case we make application changes in the future and don't remember this weird special case.
Test Plan:
- As an administrator and non-administrator, used People and Conduit to query MFA, no-MFA, and don't-care-about-MFA. These queries worked for an admin and didn't work for a non-admin.
- Viewed the list as an administrator, saw MFA users annotated.
- Viewed config help, clicked link as an admin, ended up in the right place.
{F4093033}
{F4093034}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12400
Differential Revision: https://secure.phabricator.com/D17500
Summary: Fixes T12398. This adds `withBadgeStatuses` as a query parameter when searching for Awards to show. In most (all?) cases we currently only show active badges.
Test Plan: Assign myself a badge, archive it and verify it does not appear on profile, comment form, or timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12398
Differential Revision: https://secure.phabricator.com/D17499
Summary: Ref T12264. This allows users to install a dashboard they are viewing to their personal home menu or as a global home menu item. Has some basic ability to be extended later for maybe projects.
Test Plan:
Build a dashboard, click "Install Dashboard".
- As user only get personal option
- As HomeApp edit person, see both options
- Try installation as either, with and without label set
- Fake "global" form as user, get error
- Don't set anything, get error
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12264
Differential Revision: https://secure.phabricator.com/D17492
Summary:
Ref T10967. This is an incremental step toward removing "reviewers" back to a dedicated storage table so we can handle changes like T11050.
This adds the storage table, and starts doing double writes to it (so new or updated reviewers write to both the old edge table and the new "reviewers" table).
Then we can do a migration, swap readers over one at a time, and eventually remove the old write and old storage and then implement new features.
This change has no user-facing impact, it just causes us to write new data to two places instead of one.
This is not completely exhaustive: the Herald "Add Reviewers" action is still doing a manual EDGE transaction. I'll clean that up next and do another pass to look for anything else I missed.
This is also a bit copy/pastey for now but the logic around "RESIGN" is a little different in the two cases until T11050. I'll unify it in future changes.
Test Plan:
- Did a no-op edit.
- Did a no-op comment.
- Added reviewers.
- Removed reviewers.
- Accepted and rejected revisions.
After all of these edits, did a `SELECT * FROM differential_reviewer` manually and saw consistent-looking rows in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17495
Summary: Fix tag alignment on project cards when there are multiple tags. Also fixes T12381.
Test Plan: Review a project and people hovercard in sandbox, ensure multiple tags look as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12381
Differential Revision: https://secure.phabricator.com/D17488
Summary: These were once ordered, but I think we switched to being defined in the Engine and never implemented the sorts there. This adds sort ordering to Tasks, Projects, and Repositories.
Test Plan: Review Favorites Menu in local install, see order is now set per the engine. Click Edit Favorites, and re-order. See order sticks.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17490
Summary:
Ref T12270. This just drops the constraint for now, rather than dealing with all the typecasting stuff and putting us in a position which will almost certainly require backward compatibility breaks in the future.
Also renames "badges.*" to "badge.*" for consistency (all other methods are singular: token.*, project.*, differential.revision.*, etc).
Test Plan:
Saw "qualities" now "Not Supported", while other constraints continue to work:
{F3887194}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17487
Summary: Allow people to award and remove badges via conduit, but not from the standard badges form.
Test Plan:
Build a generator and generate awards. Didn't test the revoke yet.
{F3857766}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17482
Summary:
Fixes T12378. Two minor issues here:
- CAN_INTERACT on tasks uses "USER", but should just use the view policy, which may be more permissive ("PUBLIC").
- CAN_INTERACT is currently prevented from being "PUBLIC" by additional safeguards. Define an explicit capability object for the permission which returns `true` from `shouldAllowPublicPolicySetting()`.
Test Plan:
- Viewed an unlocked task as a logged-out user, saw "login to comment" instead of "locked".
- Viewed a locked task as a logged-out user, saw "locked".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12378
Differential Revision: https://secure.phabricator.com/D17485
Summary: This is overly broad and I missed it in local testing with just a single account. Let's pull just the author in.
Test Plan: Review a commit page that wasn't my own, see other authors commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17481
Summary: This should be blue, not grey.
Test Plan: Add a milestone and subproject to a project
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17477
Summary: Fixes T10698. This shows badges under the comment preview if the application uses TransactionCommentView. I suspect not everything does, but will pick the fix up for free when modernized.
Test Plan: Test commenting on a task with and without a user that has a badge. See badge preview.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10698
Differential Revision: https://secure.phabricator.com/D17480
Summary: This currently queries all tasks, make it limit to only open tasks.
Test Plan: Assign myself an open and a resolved task. See only open on profile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17476
Summary: Fixes T12360. I'll probably make a non-audit commit list for this, maybe, eventually, until then add all the needed audit information.
Test Plan: Review commits in my profile, see data and not a fatal.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12360
Differential Revision: https://secure.phabricator.com/D17475
Summary: This just adds a few more dimensions to the generator.
Test Plan: run `bin/lipsum generate badges`, verify new icons and quality work.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17474
Summary: Probably useful if you use Phrequent.
Test Plan: I did not test this beyond lint/unit.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17473
Summary: Mostly a minor nit-pick, but I hate sending users off the profile and disorient them onto application search. These pages are pretty easy to maintain, I don't expect to need to do more here. I dropped Differential outright. Kept Tasks and Commits. Now you can browse everything about a user on their profile without leaving. Maybe add a link to ApplicationSearch? Not sure it's important.
Test Plan: Review tasks and commits on mine and other user profiles.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17470
Summary: Ref T12270. Adds the date the badge was awarded.
Test Plan: Award a badge, see date on profile badge when card is flipped.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17471
Summary: Fixes T10319. This looks for custom profile image, then falls back to a generated profile image.
Test Plan: Create a new user, log in, and see new profile image. Note this seems to break `bin/lipsum generate user`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17467
Summary: Ref T12337. This just fills out a couple more task relationships.
Test Plan: Viewed the edges in the Conduit console, queried for them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12337
Differential Revision: https://secure.phabricator.com/D17465
Summary: Ref T10319. This adds a basic means of generating default profile images for users. You can generate them for everyone, a group of users, or force updates. This only generated images and stores them in files. It does not assign them to users.
Test Plan:
`bin/people profileimage --all` to generate all images.
`bin/people profileimage --users chad` to generate a user.
`bin/people profileimage --all --force` to force rebuilding all images.
{F3662810}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17464
Summary:
Ref T12337. Ref T5873. This provides a generic "edge.search" method which feels like other "verison 3" `*.search` methods.
The major issues here are:
1. Edges use constants internally, which aren't great for an API.
2. A lot of edges are internal and probably not useful to query.
3. Edges don't have a real "id", so paginating them properly is challenging.
I've solved these things like this:
- Edges must opt-in to being available via Conduit by providing a human-readable key (like "mention" instead of "52"). This solvs (1) and (2).
- I faked a mostly-reasonable behavior for paginating.
Test Plan:
Ran various valid and invalid searches. Paginated a large search. Reviewed UI.
{F3651818}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12337, T5873
Differential Revision: https://secure.phabricator.com/D17462
Summary: Ref T12335. Allows you to lock tasks to keep your precious tokens.
Test Plan:
- Awarded tokens to an unlocked task.
- Locked the task.
- Could no longer award/rescind tokens.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12335
Differential Revision: https://secure.phabricator.com/D17461
Summary: Ref T10319. Adds in database columns for upcoming default generated avatar support.
Test Plan: Ran storage upgrade, log into local site to verify it didn't blow up.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17459
Summary: Ref T10319. This swaps the default in the Picture Chooser to allow picking of the custom unique avatar. We're currently going with 100k unique possibilities. The logic roughly hashes a user name and picks an image pack, color, and border. Based on that, we select the first character of their username, or fall back to Psyduck if not [a-z][0-9].
Test Plan:
Set the following usernames from ProfilePicture as a test: chad, epriestley, sally, 007, _cat_, -doggie-.
{F3453979}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17430
Summary:
Ref T12313. This puts a UI on revoking credentials after a widespread compromise like Cloudbleed or a local one like copy/pasting a token into public chat.
For now, I'm only providing a revoker for conduit tokens since that's the immediate use case.
Test Plan:
- Revoked in user + type, everything + user, everywhere + type, and everything + everywhere modes.
- Verified that conduit tokens were destroyed in all cases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12313
Differential Revision: https://secure.phabricator.com/D17458
Summary: Ref T9010. This adds more icons and lets the IconChooser handle more icons more easier.
Test Plan: Test Project Icons, Badges Icons
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9006, T9010
Differential Revision: https://secure.phabricator.com/D17456
Summary: Fixes T12347. Ref T12314. Validation gets called no matter what, but is only relevant if the form supports subtypes.
Test Plan: Marked/unmarked a Paste form as editable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12347, T12314
Differential Revision: https://secure.phabricator.com/D17457
Summary: Fixes T10798. Separates these two since they don't need to be combined and it allows for more flexibility / scalability.
Test Plan:
- Add Badge
- Edit Badge
- Add myself as Recipient
- Remove myself
- Go to my profile
- Award Badge from there
- Assign myself a badge, try to re-assign it, see validation error.
Also, validation errors on dialog forms are ugly.
{F3495630}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10798, T12270
Differential Revision: https://secure.phabricator.com/D17447
Summary:
This is a consistency change to make this option consistent with `audio-mime-types`, `image-mime-types` and `icon-mime-types`, all of which are locked.
(They're locked because SVG is definitely dangerous, and other types might be dangerous or might become dangerous in the future, although I'm not aware of any actual dangers from video types today.)
Test Plan: Viewed `files.video-mime-types` in Config, saw it was locked.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17454
Summary:
Ref T12335. See that task for discussion. Here are the behavioral changes:
- Statuses can be flagged with `locked`, which means that tasks in that status are locked to further discussion and interaction.
- A new "CAN_INTERACT" permission facilitates this. For most objects, "CAN_INTERACT" is just the same as "CAN_VIEW".
- For tasks, "CAN_INTERACT" is everyone if the status is a normal status, and no one if the status is a locked status.
- If a user doesn't have "Interact" permission:
- They can not submit the comment form.
- The comment form is replaced with text indicating "This thing is locked.".
- The "Edit" workflow prompts them.
This is a mixture of advisory and hard policy checks but sholuld represent a reasonable starting point.
Test Plan: Created a new "Locked" status, locked a task. Couldn't comment, saw lock warning, saw lock prompt on edit. Unlocked a task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12335
Differential Revision: https://secure.phabricator.com/D17453
Summary:
Ref T12335. Fixes T11207. Edit-like interactions which are not performed via "Edit <object>" are a bit of a grey area, policy-wise.
For example, you can correctly do these things to an object you can't edit:
- Comment on it.
- Award tokens.
- Subscribe or unsubscribe.
- Subscribe other users by mentioning them.
- Perform review.
- Perform audit.
- (Maybe some other stuff.)
These behaviors are all desirable and correct. But, particularly now that we offer stacked actions, you can do a bunch of other stuff which you shouldn't really be able to, like changing the status and priority of tasks you can't edit, as long as you submit the change via the comment form.
(Before the advent of stacked actions there were fewer things you could do via the comment form, and more of them were very "grey area", especially since "Change Subscribers" was just "Add Subscribers", which you can do via mentions.)
This isn't too much of a problem in practice because we won't //show// you those actions if the edit form you'd end up on doesn't have those fields. So on intalls like ours where we've created simple + advanced flows, users who shouldn't be changing task priorities generally don't see an option to do so, even though they technically could if they mucked with the HTML.
Change this behavior to be more strict: unless an action explicitly says that it doesn't need edit permission (comment, review, audit) don't show it to users who don't have edit permission and don't let them take the action.
Test Plan:
- As a user who could not edit a task, tried to change status via comment form; received policy exception.
- As a user who could not edit a task, viewed a comment form: no actions available (just "comment").
- As a user who could not edit a revision, viewed a revision form: only "review" actions available (accept, resign, etc).
- Viewed a commit form but these are kind of moot because there's no separate edit permission.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12335, T11207
Differential Revision: https://secure.phabricator.com/D17452
Summary: Ref T12270. We don't really need these, timeline does it's own thing, badges is now a profile page, and hovercards have been removed.
Test Plan: Visit timeline, still see badges, visit my profile page, bask in the warmth of fake awards.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17448
Summary: Ref T12270. Adds the name of the badge to the subject, fixes the double description.
Test Plan: Edit lots of badges with and without descriptions, see good emails.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17449
Summary: Doesn't seem popular, will rethink dashboard editing again in the future at some point.
Test Plan: Review a dashboard, edit, install.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17450
Summary:
Ref T12314. Ref T6064. Ref T11580. If an install defines several different task create forms (like "Create Plant" and "Create Animal"), allow any of them to be created directly onto a workboard column.
This is just a general consistency improvement that makes Custom Forms and Workboards work together a bit better. We might do something fancier eventually for T6064 (which wants fewer clicks) and/or T11580 (which wants per-workboard control over forms or defaults).
Test Plan:
- Created several different types of tasks directly onto a workboard.
- Faked just one create form, saw the UI unchanged (except that it respects any renaming).
{F3492928}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314, T11580, T6064
Differential Revision: https://secure.phabricator.com/D17446
Summary:
Ref T12314. When we pick an "Edit" form for a subtyped object, only consider forms with the same subtype.
For example, editing an "Animal" uses the forms with subtype "animal" which are marked as edit forms.
This also makes "Create Subtask" carry the parent task's type.
Test Plan:
- Edited an Animal, got an animal edit form.
- Edited a normal task, got a normal task form.
- Edited a paste, got the normal workflow.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17445
Summary:
Ref T12314. Allow tasks to be queried by subtype using a typeahead.
Open to a better default icon. I'll probably let you configure them later.
Just hide this constraint if there's only one subtype.
Test Plan:
- Searched for subtypes.
- Verified that the control hides if there is only one subtype.
{F3492293}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17444
Summary:
Ref T12314. If you set a form to have the "plant" subtype, then create a task with it, save "plant" as the task subtype.
For Conduit, the default subtype is used by default, but a new "subtype" transaction is exposed. You can apply this transaction at create time to create an object of a certain subtype, or at any later time to change the subtype of an object.
This still doesn't do anything particularly useful or interesting.
Test Plan:
- Created a non-subtyped object (a Paste).
- Created "task" and "plant" tasks via different forms.
- Created "default" and "plant" tasks via Conduit.
- Changed the subtype of a task via Conduit.
- Tried to set a bad subtype.
{F3492061}
{F3492066}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17443
Summary:
Ref T12314. This adds a "Change Form Subtype" workflow to the EditEngine form configuration screen, for forms that edit/create objects which support subtyping (for now, only tasks).
For example, this allows you to switch a form from being a "task" form to a "plant" or "animal" form.
Doing this doesn't yet do anything useful or interesting. I'm also not showing it in the UI yet since I'm not sure what we should make that look like (presumably, we should just echo whatever UI we end up with on tasks).
Test Plan:
- Changed the subtype of a task form.
- Verified that the "Change Subtype" action doesn't appear on other forms (for example, those for Pastes).
{F3491374}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17442
Summary: Ref T12314. Provides a field on tasks for storing subtypes. Does nothing interesting yet.
Test Plan:
- Ran storage upgrade.
- Created some tasks.
- Looked in the database.
- Used Conduit to query some tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17441
Summary:
Ref T12314. Builds toward letting you define "animal" and "plant" tasks.
This just adds some configuration. I'll probably add some more quality-of-life options (like "icon") later but these are the only bits I'm sure I'll need.
Test Plan:
- Configured sensible subtypes.
- Tried to configure bad subtypes: bad key, missing "default", duplicate keys. Got sensible error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17440
Summary:
Ref T12314. This adds storage so EditEngine forms can later be marked as edit fields for particular types of objects (like an "animal edit form" vs a "plant edit form").
We'll take you to the right edit form when you click "Edit" by selecting among forms with the same subtype as the task.
This doesn't do anything very interesting on its own.
Test Plan:
- Ran `bin/storage upgrade`.
- Verified database got the field with proper values.
- Created a new form, checked the database.
- Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17439
Summary: Ref T12298. `phd launch` was missed in D17390 and thus broken by D17389.
Test Plan: Launched a daemon with great success.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17429
Summary: Fixes T12338. Resolves an issue where long pastes would be truncated before getting a line count, resulting in an inaccurate line count being returned.
Test Plan: Made a large paste, verified that it displayed the correct number of lines.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T12338
Differential Revision: https://secure.phabricator.com/D17438
Summary: Fixes T12330. Minor UI nit, since we use "disabled" to usually mean "no permission". Makes these links always normal looking.
Test Plan: Review a new project in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12330
Differential Revision: https://secure.phabricator.com/D17436
Summary: Ref T12331. These changes are intended to make it easier to debug T12331 since I'm having difficulty reproducing the issue locally.
Test Plan:
- Ran `bin/phd debug task --pool 4` and got an autoscaling pool.
- Ran `bin/worker flood --duration 3` and got some 3-second-long tasks to execute with `bin/worker execute ...`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12331
Differential Revision: https://secure.phabricator.com/D17431
Summary: Ref T12324. Adds back this query for search results in dashboards.
Test Plan: Use panel in Dashboard.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12324
Differential Revision: https://secure.phabricator.com/D17428
Summary: Ref T10319. This builds out a reasonably decent avatar generator. 256 colors x 74 images x 2 borders, 38k options. Not completely sure though how names disburse though, so likely half that number. I can add lowercase lettering to double the footprint if needed though.
Test Plan:
UIExamples. Color generator here: http://tools.medialab.sciences-po.fr/iwanthue/
{F3416622}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17418
Summary:
Fixes T12322. Allows you to search for commits using the `tagged(...)` repository function, so you can find "any commmit in any repository tagged with android" or similar.
I moved the function from Differential (which was the application using it) to Diffusion (which is more accurately the application which provides it).
I fixed a bug where searching for `tagged(xyz)` would have no effect (constraint was ignored) if there were no repositories tagged with "xyz". The fix isn't perfectly clean, but should work properly for the moment.
Test Plan:
- Searched with `tagged(...)` in Diffusion and Differential.
- Searched by repository.
- Searched with `tagged(...)` for a project with no tagged repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12322
Differential Revision: https://secure.phabricator.com/D17426
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 T12319. Ref T12270. Allow badges to be generated with `bin/lipsum`. These aren't hugely sophisticated but I'm not sure about the fate of T9010 yet or what's happening with the quality levels, and didn't want to make those changes more difficult.
Test Plan:
- Used `bin/lipsum generate badges --force --quickly` to generate badges.
- Made some coffee and came back to 20K badges.
{F3422200}
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12319, T12270
Differential Revision: https://secure.phabricator.com/D17422
Summary:
Ref T12319.
- Lipsum can trash an install by creating a lot of junk that's hard to get rid of, so we're cautious about letting you run it. Add a `--force` flag if you're sure you know what you're doing. This makes the edit/test cycle a bit easier when actually writing Lipsum generators.
- Lipsum normally sleeps for a second before creating objects, to give users more control over how much stuff they create and limit the amount of damage caused by mistakes. Sometimes, you want to generate a LOT of stuff because you want to reproduce a performance/scale issue (like T12319). Add a `--quickly` flag to generate objects as fast as possible.
- When loading random users (used as authors, assignees, etc), also load user settings so we can `ConduitCall` with them.
- Allow generators to return a PHID instead of an actual object (more convenient for Conduit-based generators).
Test Plan:
- With next change, ran `lipsum generate badges --force --quickly`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17421
Summary:
Ref T12319. Currently, `bin/lipsum` uses substring matches against human-readable text to chose which objects to generate.
Instead:
- Use separate selector keys which are guaranteed to be unique.
- When a match is exact, select only that generator.
- When a match is ambiguous, fail and warn the user.
Test Plan: Generated several types of objects, tried to generate ambiguous objects like "e".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17420
Summary: Ref T12319. The product name is misspelled in some methods, and a few places in the documentation.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17419
Summary: Looks nicer on profiles, cards. Added some additional colors.
Test Plan: change my avatar a few times
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: avivey, Korvin
Differential Revision: https://secure.phabricator.com/D17416
Summary: We moved to having "no data" strings render in italics, but sometimes it doesn't make sense. This renders out the panel a little more expected.
Test Plan: Clean install of Phabricator, read home page activity box.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17415
Summary: Ref T12270. Moves badges into their own page and menu item. Capable of displaying hundreds of useful tokens of appreciation and dedication.
Test Plan:
Test blank state, mobile, awards badges.
{F3284139}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17410
Summary:
Ref T12296. This cache is used to cache Git ref heads (branches, tags, etc). Reasonable repositories may have more than 2048 of these.
When we miss the cache, we need to single-get refs to check them, which is relatively expensive.
Increasing the size of the cache to 65535 should only require about 7.5MB of RAM.
Additionally, fill only as much of the cache as actually fits. The FIFO nature of the cache can get us into trouble otherwise.
If we insert "A, B, C, D" and then lookup A, B, C, D, but the cache has maximum size 3, we get this:
- Insert A, B, C, D: cache is now "B, C, D".
- Lookup A: miss, single get, insert, purge, cache is now "C, D, A".
- Lookup B: miss, singel get, insert, purge, cache is now "D, A, B".
Test Plan:
- Reduced cache size to 5, observed reasonable behavior on the `array_slice()` locally with `bin/repository update` + `var_dump()`.
- Used this script to estimate the size of 65535 cache entries as 7.5MB:
```
epriestley@orbital ~ $ cat size.php
<?php
$cache = array();
$mem_start = memory_get_usage();
for ($ii = 0; $ii < 65535; $ii++) {
$cache[sha1($ii)] = true;
}
echo number_format(memory_get_usage() - $mem_start)." bytes\n";
epriestley@orbital ~ $ php -f size.php
7,602,176 bytes
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12296
Differential Revision: https://secure.phabricator.com/D17409
Summary: Ref T6049. This moves Phurl to modular transactions.
Test Plan: Everything works here, add phurl, edit phurl, use phurl. Test various error states. Left a TODO on the validate dupe keys, not sure how to implement that in modular-land.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D17405
Summary:
Fixes T12304. If you have a Herald rule which tries to add a commit author as an auditor, it fails validation when trying to apply.
Stop trying to apply these transactions, and explicitly tell the user why. Differential already uses a similar ruleset around reviewers, but Audit was using older code.
Test Plan:
- Wrote a Herald rule to add A, B and C as auditors.
- Committed as A.
- After change, saw B and C added with transacript guidance that A was the author.
{F3235660}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12304
Differential Revision: https://secure.phabricator.com/D17404
Summary:
Fixes T12302. Currently, we aren't merging multiple "AddAuditors" transactions correctly.
This can occur when Herald triggers multiple auditor rules.
Instead, merge them.
Test Plan:
- Wrote two different Herald rules that add auditors.
- Pushed a commit which triggered them.
- After the change, saw all the auditors get added correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12302
Differential Revision: https://secure.phabricator.com/D17403
Summary: Ref T12270. This converts Badges to modular transactions for editing and awarding.
Test Plan: Add Badge, edit badge, award and revoke... Still going to test this some more but feel free to comment on anything obviously wrong?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17402
Summary: Ref T12297. This could be fancier, but should make pulling profiles off `admin.phacility.com` significantly more realistic.
Test Plan: Dragged and dropped some profiles to upload them, then reviewed them via web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17401
Summary:
Ref T12297. This slightly modernizes the XHProf UI. Not included here:
- Some of the code acts like samples have PHIDs, but they currently do not. I plan to add them in the next change.
- I've intentionally left the actual list untouched for now -- it has some old/buggy code (like `flag-6` is no longer an icon) that I'll fix in a future change.
Test Plan: {F3224264}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17400
Summary:
Fixes T12172. Fixes T12060. This allows runtime code building CSS for mail to read CSS variables, then makes all the code do that.
It reverts the non-colorblind red/green to the colors in use before T12060, which seem better for non-colorblind users since no one really complained?
Test Plan:
- Viewed code diffs in Web UI.
- Viewed prose diffs in Web UI.
- Viewed code diffs in email.
- Viewed prose diffs in email.
All modes respected the accessibility color scheme.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12172, T12060
Differential Revision: https://secure.phabricator.com/D17269
Summary: Ref T10390. Basically hides policy controls when creating a panel on a dashboard. Shows when you edit them or through normal workflow. I think we should maybe also get rid of view policy? Not sure the benefit since results will be filtered anyways. Maybe Text panels? Not sure the use case.
Test Plan: Add a panel, edit a panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: hskiba, Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17393
Summary:
Fixes T12301. In D17372, this changed to use generic EditEngines instead of the proper runtime engine. Normally this doesn't matter, but can in this case.
After loading the configurations normally, swap their attached engines for the specific configured runtime engine we're currently executing.
Test Plan: Clicked "Create Form" from the Maniphest form list, saw it go to "Create Maniphest Form", not "Create Generic Meta-Form".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12301
Differential Revision: https://secure.phabricator.com/D17398
Summary:
Fixes T12306. Currently, we warn about daemons not running even if they're in normal "alive" states, particularly "waiting to restart after a failure".
This check was made more strict in D12088, back when we tried to version check running daemons. Since we implemented auto-restart-after-config-change we don't do this anymore, so it should be fine to make this more lax again.
Test Plan:
- Faked an exception for all tasks.
- Before patch: reloading the daemon setup error sometimes raised a false positive ("waiting" daemon detected as dead).
- After patch: daemon setup error no longer triggers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12306
Differential Revision: https://secure.phabricator.com/D17397
Summary:
Ref T12298. This updates `bin/phd` for minor changes to daemon configuration. In particular:
- Every daemon now has an autoscale pool (for trigger/pull, the maximum pool size is 1).
- Pools now have labels to make debugging a little easier.
- Some minor structural changes.
Test Plan: See D17389.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17390
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
