Summary:
Ref T603. I got most of this earlier, but finish it up.
- Make a couple of controllers public; pretty much everything in Diffusion has implicit policy checks as a result of building a `DiffusionRequest`.
- Add an "Edit" capability to commits.
- Swap out the comment thing for commits.
- Disable actions if the user can't take them.
Test Plan: Viewed a bunch of interfaces while logged out, got appropriate results or roadblocks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7152
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.
- Provide as much detail as possible.
- Fix all the strings for i18n.
- Explain special rules to the user.
- Allow indirect policy filters to raise policy exceptions instead of 404s.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7151
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".
This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7150
Summary:
Ref T603. Make Differential behaviors for logged-out and underprivleged users more similar to other apps.
I'm going to drop this "anonymous access" thing at some point, but `reviews.fb.net` actually looks like it's running semi-modern code, so leave it alive until we have a more compelling replacement in the upstream.
Test Plan: As a logged out user, browsed Differential and clicked things and such.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7148
Summary: right now you get sent an email with a broken link 'cuz the email is plain text if you edit something with the edit policy being a project.
Test Plan: edited a legalpad document edit policy repeatedly to various projects. observed good emails via bin/mail debug tool. object page still looked good too
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7149
Summary:
Ref T603. When a diff is attached to a revision, try to guess the repository if possible. In cases where we succeed, this automatically gives us intuitive policy behavior (i.e., you can see a revision if you can see the repository the change is against).
I pulled this into a funky little "Lookup" class for two reasons:
- It's used in two places;
- I anticipate that we might need to add some sort of `explainWhy()` method if users find the heuristics confusing.
Test Plan: Created and updated revisions, saw them pick up the correct repository association. Ran Herald dry run against associable and nonassociable revisions, saw correct values populate.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7147
Summary: This ends up living in HeraldAdapter even though its "task only" stuff. Reason being There are 4 or 5 functions that have little hooks; see diff. Ref T1638.
Test Plan: made a rule to assign tasks to me if made on web - great success. made a rule to assign tasks to other guy and add a project if title contained "foobar" - great success, including some confusion as ther two herald rules fought each other for task ownership.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1638
Differential Revision: https://secure.phabricator.com/D7146
Summary: ...and deploy on Maniphest. Ref T1638.
Test Plan: created a herald rule to be cc'd for tasks created via web. made a task via web and another via email and was cc'd appropriately. edited the herald to be cc'd for tasks created via not web. made 2 tasks again and got cc'd appropriately
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1638
Differential Revision: https://secure.phabricator.com/D7145
Summary: This isn't too useful most of the time since we don't automatically populate this data yet, but works fine.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7144
Summary: Ref T603. I think T2222 is fraught with peril so I'm not going to try to sequence it ahead of T603 for Differential. Provide access to policy controls in Differential's edit view.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7142
Summary: Adds some padding to the right
Test Plan: Looked at a diff
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7143
Summary:
Ref T2222. This sequences //before// D7139 and sorts out keys on the table. In particular:
- There was a fairly silly `draft` key modeled after Pholio; drop it.
- Add a `revisionPHID` key. This is queried mostly-transitionally on the revision view screen.
- Add a `changesetID` key. This is queried by a bunch of interfaces that want more surgical results than `revisionPHID` provides.
- Add an `authorPHID, transactionPHID` key. This is queried on the list interface to find pending drafts.
- Add a `legacy` key. This is queried by the feed publisher.
Test Plan: Used the query analyzer to hit all (I think?) of the pages, saw keyed queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7140
Summary:
Ref T2217. This partially retreads the ground from D7115.
- We're rendering silly transactions about descriptions when creating tasks. Hide those.
- Move the "created" transaction back to status. This fixes two things that are otherwise more of a mess than I'd anticipated:
- It fixes Reports without making a mess (see <https://github.com/facebook/phabricator/issues/395>).
- It renders old transactions properly (i.e., "created" instead of "reopened" for tasks older than the migration).
- Be explicit about action strength, so emails always say the most important thing in the subject.
Test Plan: Created and edited tasks, looked at resulting transactions, saw a cleaner transaction record.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7141
Summary: Ref T603. Makes the majority of reads policy aware (and pretty much all the important ones).
Test Plan:
- Created a comment with `differential.createcomment`.
- Created a new revision with `arc diff` in order to exercise `differential.creatediff`.
- Created an inline comment with `differential.createinline`.
- Added a comment to a revision.
- Edited an inline comment.
- Edited a revision.
- Wrote "Depends on ..." in a summary, saved, verified link was created.
- Browsed a file in Diffusion.
- Got past the code I changed in the Releeph request thing.
- Edited a Releeph request.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7136
Summary: Ref T603. Moves policy information from a custom field to the header for revisions.
Test Plan: Looked at a revision.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7135
Summary:
Ref T603. Read policies out of policy columns.
When a revision is associated with a repository (which is currently never), require view access on the repository to see the revision (or, require the viewer to be the owner). This is a blanket "do the right thing" rule which should make Differential's default policies align with user expectations.
Future diffs will populate the `repositoryPHID` when a revision is created.
Test Plan: Tooled around Differential. None of this stuff does anything yet, so nothing very exciting happened.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7134
Summary: Ref T603. Paves the way for policy controls.
Test Plan: Ran storage upgrade, bumbled around in Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7133
Summary: Ref T603. Move to real Query classes.
Test Plan:
- Ran `phd debug pull X` (where `X` does not match a repository).
- Ran `phd debug pull Y` (where `Y` does match a repository).
- Ran `phd debug pull`.
- Ran `repository pull`.
- Ran `repository pull X`.
- Ran `repository pull Y`.
- Ran `repository discover`.
- Ran `repository delete`.
- Ran `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7137
Summary: Ref T603. This swaps almost all queries against the repository table over to be policy aware.
Test Plan:
- Made an audit comment on a commit.
- Ran `save_lint.php`.
- Looked up a commit with `diffusion.getcommits`.
- Looked up lint messages with `diffusion.getlintmessages`.
- Clicked an external/submodule in Diffusion.
- Viewed main lint and repository lint in Diffusion.
- Completed and validated Owners paths in Owners.
- Executed dry runs via Herald.
- Queried for package owners with `owners.query`.
- Viewed Owners package.
- Edited Owners package.
- Viewed Owners package list.
- Executed `repository.query`.
- Viewed "Repository" tool repository list.
- Edited Arcanist project.
- Hit "Delete" on repository (this just tells you to use the CLI).
- Created a repository.
- Edited a repository.
- Ran `bin/repository list`.
- Ran `bin/search index rGTESTff45d13dffcfb3ea85b03aac8cc36251cacdf01c`
- Pushed and parsed a commit.
- Skipped all the Drydock stuff, as it it's hard to test and isn't normally reachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7132
Summary:
We currently try to send Maniphest email "To" the owner and actor, but for unassigned tasks there is no owner.
Just filter the PHIDs in the parent, since it's reasonable for subclasses to be liberal about construction here.
Test Plan: Commented on an unassigned task, got an email without a bogus "To".
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7129
Summary:
Ref T603. Basically:
- Hide "Reports".
- Hide "batch edit" and "export to excel".
- Hide reprioritization controls.
- I left the edit controls, they show a "login to continue" dialog when hit.
- Allow tokenizer results to fill for public users.
- Fix a bug where membership in projects was computed incorrectly in certain cases.
- Add a unit test covering the project membership bug.
Test Plan: Viewed /maniphest/ when logged out, and while logged in.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7126
Summary:
Ref T603. Disable things the user can't use, allow logged-out users to get a reasonable version of the page.
Also allow logged-out users to view edit history of comments if they're able to see the object.
Test Plan: Viewed Maniphest detail as a logged-out user, got a largely sensible page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7124
Summary:
Ref T603. Adds policy controls to the task edit UI.
@chad, status + policy renders a little weird -- did I mess something up? See screenshot.
Test Plan: Edited policies, viewed a task.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7123
Summary:
Ref T603. Cleans up some obsolete stuff here:
- We no longer ever query by min/max priority (instead, `withPriorities(...)`).
- A parent class provides limit/offset.
- Result count is no longer reliable with policies. We could do "about X tasks" or something, but just drop it for now. There's only one remaining callsite anyway.
Test Plan:
- `grep`
- Viewed task list.
- Viewed a project page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7121
Summary:
Ref T603. Make almost every task read policy-aware. Notable exceptions are:
- Edge editor -- this stuff is prescreened and should be moved to ApplicationTransactions eventually anyway.
- Search/attach stuff -- this stuff needs some general work. The actual list should be fine since you can't pull handles. There may be a very indirect hole here where you could attach an object you can't see (but do know the ID of) to an object you can see. Pretty fluff.
- The "Tasks" field in Differential will let you reference objects you can't see. Possibly this is desirable, in the case of commandeering revisions. Mostly, it was inconvenient to get a viewer (I think).
Test Plan:
- Called `maniphest.info`.
- Called `maniphest.update`.
- Batch edited tasks.
- Dragged and dropped tasks to change subpriority.
- Subscribed and unsubscribed from a task.
- Edited a task.
- Created a task.
- Created a task with a parent.
- Created a task with a template.
- Previewed a task update.
- Commented on a task.
- Added a dependency.
- Searched for "T33" in object search dialog.
- Created a branch "T33", ran `arc diff`, verified link.
- Pushed a commit with "Fixes T33", verified close.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7119
Summary:
Ref T2217. Use `getLinkName()` instead of `getName()` so that we get, e.g.,
alincoln attached a revision: D123 Chop some logs
...instead of:
alincoln attached a revision: D123
Test Plan: Attached stuff, looked at the email, saw full object name.
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7127
Summary:
Currently, draggable lists (in Config and ApplicationSearch, for example) don't let you drag an item into the first position.
This is because the behavior is correct in Maniphest: the first position is above an initial header, like "High Prioirty", and shouldn't be targetable.
Permit the behavior in general; forbid it in Maniphest.
Test Plan:
- Dragged elements into the first position in ApplicationSearch.
- Failed to drag elements into the first position in Maniphest.
Reviewers: garoevans, btrahan
Reviewed By: garoevans
CC: aran
Differential Revision: https://secure.phabricator.com/D7128
Summary: adds padding to the maniphest status subheader
Test Plan: see padding in inspect element
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7125
Summary:
standing on the shoulders of the badass work to move Maniphest to ApplicationTransactions, this diff implements a few methods and adds an adapter class.
For now, we can add cc and flag tasks. I figure see what people ask for? Ref T1368.
Test Plan: created herald rules for title and description text hits. made tasks and verified CC and flags worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1368, T1638
Differential Revision: https://secure.phabricator.com/D7122
Summary: I'd like to reuse this for other content areas, renaming for now. This might be weird to keep setForm, but I can fix that later if we need.
Test Plan: reload a few forms in maniphest, projects, differential
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7120
Summary: Ref T2217. These checks are no longer necessary, ApplicationTransactions handle them for us.
Test Plan:
- Made a no-effect edit, verified no new transactions showed up.
- Made real edits, saw them happen and leave transactions.
- Made an edit which just reorders CCs, saw it detected as no-effect.
- As above, with projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7117
Summary: Ref T2217. Fixes T3866. We need to do a little more massaging before we can set strings as the value on datetime controls.
Test Plan: Set default to "1:23 AM", "2001/02/03".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217, T3866
Differential Revision: https://secure.phabricator.com/D7116
Summary:
Ref T2217. Fixes two issues:
# The "task created" email didn't include the task description, but should.
# We were treaging the "status" event as the "create", but that's kind of a mess. Treat the "title" event as the "create" instead. This makes initial emails say "[Created]".
Test Plan: Created some tasks, got better emails.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7115
Summary: These constants have moved to ManiphestTransaction. The other method only has one plausible callsite, just inline it.
Test Plan: Used Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7113
Summary:
There may be times when we don't want lines in some embeded source
code to be highlighted on click, this adds that functionality.
Also use the functionalty in `PasteEmbedView`
Test Plan:
- View paste, make sure everything works.
- Embed paste in comment, make sure everything works apart from click hl
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3881
Differential Revision: https://secure.phabricator.com/D7111
Summary: Name the token which was given in the feed story.
Test Plan: Gave/rescinded tokens. Looked at a feed story.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7110
Summary: Ref T2217. Render feed stories like "alincoln updated T123" instead of "alincoln updated this task.". Fix up some more translations.
Test Plan: Looked at feed, saw something a bit more reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7108
Summary: TimelineView on Maniphest is often kind of hard to parse because related/simultaneous transactions aren't visually grouped. Allow grouping. I'm going to clean this up a little bit more.
Test Plan: See screenshot.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7107
Summary: Removed shadow from white icons, made some new ones for maniphest, cleaned up a few old ones.
Test Plan: uiexamples, photoshop
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7106
Summary:
Fixes T3867. We currently show more empty custom field values on task detail pages than we should, for at least two reasons:
- `<select />` fields with an empty string option store `""`, but users reasonably expect this to mean "no value".
- Old fields may have stored empty strings, and migrated forward.
This fix generally aligns behavior with user expectations. We could get more extreme about not storing `""` in the database, but I think this is generally a less surpsing fix.
Test Plan: Made a select with a `"" : "None"` option, selected it, saw it vanish from task detail screen.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3867
Differential Revision: https://secure.phabricator.com/D7105
Summary: Ref T2217. Fixes T3865. Eventually we'll probably make more of this configurable, but for now shove custom fields down instead of sort of arbitrarily putting them in the middle.
Test Plan: Looked at a task with attached revisions and custom fields, saw custom fields last.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217, T3865
Differential Revision: https://secure.phabricator.com/D7103
Summary:
Ref T2217. Fixes T3876. We incorrectly have a unique key on `(authorPHID, transactionPHID)`, which prevents saving multiple versions of a comment.
I'm not entirely sure why this exists. I think it came from Pholio (where it works for inlines, because it has an additional component, but maybe should be adjusted) but we might need to wipe it out of more apps too.
Test Plan: Edited a comment in Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217, T3876
Differential Revision: https://secure.phabricator.com/D7102
Summary:
PHP_INT_MAX is rejected by ElasticSearch since it's outside of the representable integer range (see: <https://gist.github.com/JustinTulloss/c4ac0e1c93d6d1e91744>).
Just use 10K, as matching more than 10K results probably isn't useful to anyone.
Test Plan: Confirmed this fixes the issue in IRC. Ran a fulltext search.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7097
Summary: Ref T2217. Fixes T3877. Improves more behaviors in the presence of "no effect" detection and new comment storage.
Test Plan: Added CCs, added existing CCs, implicitly added CCs via reassign, added CCs with comment.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2217, T3877
Differential Revision: https://secure.phabricator.com/D7100
