Summary:
- Fixes line height when many long tasks are attached to a task.
- Tightens up mobile layout of timeline and object box
- Clean up aphront context bar
Test Plan: Tested all the changes, made tasks, stared at pixels.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3891
Differential Revision: https://secure.phabricator.com/D7169
Summary: Missed this case in my sandbox
Test Plan: Reload a test diff
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7168
Summary: This adds the 'PHUIObjectBox' to nearly every place that should get it. I need to comb through Diffusion a little more. I've left Differential mostly alone, but may decide to do it anyways this weekend. I'm sure I missed something else, but these are easy enough to update.
Test Plan: tested each new layout.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7162
Summary:
Fixes T3887. Two issues:
- Macros were generating entirely before the render cache, so audio macros worked fine in previews and the first time the cache was populated, but not afterward.
- Instead, parse them before the cache but drop them in after the cache. Clean up all the file querying, too. This makes cached remarkup generate the correct audio beahviors.
- Safari sends an HTTP request with a "Range" header, and expects a "206 Partial Content" response. If we don't give it one, it sometimes has trouble figuring out how long a piece of audio is (mostly for longer clips? Or mostly for MP3s?). I'm not exactly sure what triggers it. The net effect is that "loop" does not work when Safari gets confused. While looping a short "quack.wav" worked fine, longer MP3s didn't loop.
- Supporting "Range" and "206 Partial Content", which is straightforward, fixes this problem.
Test Plan:
- Viewed a page with lots of different cached audio macros and lots of different uncached preview audio macros, they all rendered correctly and played audio.
- Viewed a macro with a long MP3 audio loop in Safari. Verified it looped after it completed. Used Charles to check that the server received and responded to the "Range" header correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7166
Summary: Fixes T3883. This is already supported in the query, expose it in the UI.
Test Plan: Ran some queries with and without subscribers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3883
Differential Revision: https://secure.phabricator.com/D7161
Summary:
Fixes T3887. Basically:
- Macros with audio get passed to the `audio-source` behavior.
- This keeps track of where they are relative to the viewport as the user scrolls.
- When the user scrolls a "once" macro into view, and it reaches roughly the middle of the screen, we play the sound.
- When the user scrolls near a "loop" macro, we start playing the sound at low volume and increase the volume as the user scrolls.
This feels pretty good on both counts.
Test Plan: Tested in Safari, Chrome, and Firefox. FF seems a bit less responsive and doesn't support MP3, but it was fairly nice in Chrome/Safari.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7160
Summary: Ref T3887. Implements storage and editors, but not the actual audio part.
Test Plan: Edited audio, audio behaviors of macros. Transactions and email looked good. Hit error cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7159
Summary: we were bad at displaying phid-based values nicely. Now we are good at it.
Test Plan: made a herald rule where if the author was a or b, the task should be assigned to c and have projects x, y, z added to it. this displayed nicely.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7158
Summary: Ref T3887. `300px` is a little too wide on devices.
Test Plan: Viewed on a phone.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7157
Summary: Ref T3887. Similar to how we render images with `<img />`, render audio with `<audio />` if possible.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7156
Summary: Add a couple more resources that we need on most pages.
Test Plan: Regenerated resources, viewed homepage.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7155
Summary: Ref T603. We have a real policy app now, so put the config options there. Revise the description of the public policy switch to make it clear that enabling it immediately opens up the user directory and various other interfaces.
Test Plan: Viewed/edited config setting.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7154
Summary:
Ref T603. If an install allows acccess by logged-out users, show search.
(A lot of the search typeahead results, although visible to the user, don't lead anywhere interesting right now. We can clean this up in the future.)
Test Plan: As a logged out user, searched for some stuff. It worked. Also, I only found results I could see, which is quite heartening.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7153
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
Summary: Ref T2217. Cleans up some of the "attached %d file(s)" stuff.
Test Plan: Generated some of these transactions and verified they render more naturally.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7096
Summary:
Ref T2217. Since we aren't actually using subscriptions yet, the "transactions have no effect" detection can trigger for `@mention`s of users who are already CC'd on a task.
Be more conservative about generating a CC transaction.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7095
Summary: Ref T2217. Cleans up the table names. Moves old data to `maniphest_transaction_legacy`. We'll drop that eventually once it's more clear that I didn't break the world.
Test Plan: Did reads/writes to/from these tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7094
Summary: Ref T2217. Pro is the new standard.
Test Plan: Lots of `grep`, made a pile of Maniphest views/edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7093
Summary: Ref T2217. The preview had the last callsite, nuke it.
Test Plan: Used preview. Grepped for `ManiphestTransaction(`, `ManiphestTransaction::`, `'ManiphestTransaction'`, `"ManiphestTransaction"`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7092
Summary:
`getArcanistProjectName()` has some logic which gets messy with the `self::ATTACHABLE` mechanism. This makes `differential.getdiff` and similar Conduit methods throw an exception when querying a diff which doesn't have a project. See <http://pastebin.com/Czzrd0Jz>.
Instead, unconditionally attach a project (possibly `null`) when loading diffs if they need projects.
Test Plan: Ran `differential.getdiff` against a `arc diff --raw` diff with no project, got a result instead of an exception.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, sttwister
Differential Revision: https://secure.phabricator.com/D7101
Summary: Adds status icons and colors to Maniphest and Differential. Also minor tweaks to them in hovercards. Probably some other stuff too.
Test Plan: Test many diff and task states.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7098
Summary:
Ref T2217. Removes most of the code from ManiphestTransactionEditor.
- Provides mail tag support in ManiphestTransactionEditorPro.
- There was one more write (subscribe/unsubscribe button) that I'd missed; modernize that.
Test Plan:
- Clicked subscribe/unsubscribe.
- Made some edits, verified mail had appropriate mail tags.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7091
Summary: Ref T2217. This is essentially the last writer, should be able to start deleting code now.
Test Plan: Used "Edit Task" to make a bunch of task edits.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7090
Summary:
"Branch" really means "repository main screen, with some branch selected", so a branch isn't actually required since we can just take you to the default.
Fixes an issue where new crumbs would throw an exception in SVN repositories.
Test Plan: Browed an SVN repo.
Reviewers: btrahan, mbishopim3
Reviewed By: mbishopim3
CC: aran
Differential Revision: https://secure.phabricator.com/D7099
Summary: When loading the cursor repository, we need to load the most recent
commit too if we're paging by commit date. This fixes a fatal for installs
with more than 100 repositories.
Auditors: btrahan
Summary: Ref T2217. When you add comments (or use that interface to make updates), ship it through the new code.
Test Plan: Added comments, made other changes to Maniphest tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7088
Summary: Ref T2217. Ship these through the new stuff.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7087
Summary: Ref T2217. Mail writes go through the new code now.
Test Plan: Shipped a bunch of mail in with `./bin/mail`, got reasonable edits as a result.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7086
Summary: Ref T2217. Nothing too surprising here. This transaction type is weird and should be replaced with the mainstream EDGE type at some point after things clear up more.
Test Plan: Attached and detached revisions and mocks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7085
Summary:
Ref T2217. Ship "Merge in Duplicates" through the new editor. The only notable thing here is `setContinueOnMissingFields()`.
The problem this solves is that if you add a custom field and mark it as required, all existing tasks are "invalid" since they don't have a value, and trying to edit them will raise an error like "Some Custom Field is required!". This is fine for normal edits via the UI, since the user can just select/provide a value, but surgical edits to specific fields should just ignore these errors. Add the ability to ignore these errors and use it on all the field-speific editors.
Test Plan: Merged duplicates, including "invalid" duplicates with missing fields.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7084
Summary:
Ref T2217. Swaps batch edits to modern editor.
Also, fix some issues with required fields and viewers being required to render certain standard fields (notably, date).
Test Plan: Made various batch edits, verified they went through properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7083
Summary:
Ref T2217. Drive reports out of the new table. Nothing too magical going on here.
Also fixes a bug with one of the links from reports.
Test Plan: Viewed reports.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7081
Summary: Ref T2217. All the reads route through new code already, start swapping writes over. This is the simplest writer, used when the user drag-and-drops stuff on the task list.
Test Plan: Dragged and dropped stuff across priorities. Got a transaction and some email. Verified the email and transaction looked OK, threaded properly, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7080
Summary: Ref T2217. No remaining callsites. Also get rid of some methods on ManiphestTransaction that nothing calls anymore.
Test Plan: `grep`, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7079
Summary: Ref T2217. Nuke this legacy callsite.
Test Plan: Loaded a task, looked at it. Looked the same as before.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7078
Summary: Ref T2217. These are mostly me making stuff up rather than some bird's-eye view of color and iconography across applications, yell if any of these seem off once this rolls out.
Test Plan:
- Looked at a bunch of transactions, saw reasonable looking colors and icons.
- Sent email, saw appropriate subject line actions.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7075
Summary: Ref T2217. Get rid of this rendering pathway's internals and move them to the modern stuff.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7072
Summary:
Ref T2217. This showed the text diff when you updated the description of a task, but is now obsolete.
Remove flags and methods related to rendering this pathway.
Test Plan: Clicked the fancy new "Show Details" instead.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7071
Summary: Ref T2217. Route transaction rendering through modern code. This just affects the detail page. Some rough edges but nothing significant.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7070
Summary:
Ref T2217. Move this off `LegacyQuery` and on to the real deal.
The rest of this patch mostly just replaces some gymnastics to get accurate-ish timestamps for CCs/Owners with `time()`. The search feature where edge time is stored was never really used and isn't necessarily of much value -- most indexers don't bother computing it exactly, and possibly we should get rid of it entirely. If it surfaces in the product again at some point, it's easy enough to make the time data more accurate and reindex.
Test Plan: Ran `bin/search index T12`, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7069
Summary:
Ref T2217. This is the risky, hard part; everything after this should be smooth sailing. This is //mostly// clean, except:
- The old format would opportunistically combine a comment with some other transaction type if it could. We no longer do that, so:
- When migrating, "edit" + "comment" transactions need to be split in two.
- When editing now, we should no longer combine these transaction types.
- These changes are pretty straightforward and low-impact.
- This migration promotes "auxiliary field" data to the new CustomField/StandardField format, so that's not a straight migration either. The formats are very similar, though.
Broadly, this takes the same attack that the auth migration did: proxy all the code through to the new storage. `ManiphestTransaction` is now just an API on top of `ManiphestTransactionPro`, which is the new storage format. The two formats are very similar, so this was mostly a straight copy from one table to the other.
Test Plan:
- Without performing the migration, made a bunch of edits and comments on tasks and verified the new code works correctly.
- Droped the test data and performed the migration.
- Looked at the resulting data for obvious discrepancies.
- Looked at a bunch of tasks and their transaction history.
- Used Conduit to pull transaction data.
- Edited task description and clicked "View Details" on transaction.
- Used batch editor.
- Made a bunch more edits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7068
Summary:
Ref T2217. I'm going to do the fake-double-writes ("double reads"?) thing where we proxy the storage that worked pretty well for auth. That is:
- (Some more cleanup diffs next, maybe?)
- Move all the data to the new storage, and make `ManiphestTransaction` read and write by wrapping `ManiphestTransactionPro`.
- If nothing breaks, it's a straight shot to nuking ManiphestTransaction callsite by callsite.
I think Maniphest is way easier than Differential, because there are very few query sites and no inline comments.
Test Plan: `grep` to find callsites. Loaded task view, called Conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7067
Summary: Ref T2217. Add the tables and comment class for the new stuff. Not used yet.
Test Plan: Ran storage upgrade, browsed Maniphest.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2217
Differential Revision: https://secure.phabricator.com/D7066
