Summary:
Ref T4788. Add links to jump to search results with a task's parents or subtasks. This allows relationships to remain useful if there are a zillion of them, and you can sort/filter stuff more easily.
Language might need some tweaking at some point, feeling a little un-brainy today with wordstuff.
Test Plan:
{F1740855}
{F1740856}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16343
Summary:
Ref T8126. Ref T4788. This adds a way to query by parent or subtask.
I plan to link to this from the task graph (e.g., {nav View > Search Subtasks} or similar, in a dropdown on the "Task Graph" element) as a way to let us bail out if tasks have 300 subtasks and send the user to a big query result list. That'll give us more flexibility to tailor the UI for reasonable numbers of tasks.
There's no UI for this unless you specify a query yourself, so the only ways to get to it are:
- Manually put `?parentIDs=...` into the URI.
- Use the API.
- Future link from task graphs.
It doesn't seem too useful to me on its own, outside of the context of links from tasks.
Test Plan:
- Manually put `?parentIDs=...` and `?subtaskIDs=...` into Maniphest query UI, got expected results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T8126
Differential Revision: https://secure.phabricator.com/D16341
Summary:
Ref T8126. See that task for discussion. This change:
- Updates language to be more consistent ("Parents", "Subtasks") since I moved us away from the often-confusing "Block" language in T4788.
- Fixes bugs with finding the wrong set of tasks if tasks have a mixture of open and closed subtasks or parents.
Test Plan:
- Created four tasks: no subtasks, one closed subtask, one open subtask, mixture of open and closed subtasks.
- Created four more tasks: no parents, one closed parent, one open parent, mixture of open and closed parents.
- Searched for all this stuff, got the proper results:
{F1740683}
{F1740684}
{F1740685}
{F1740686}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8126
Differential Revision: https://secure.phabricator.com/D16340
Summary: Ref T11326. This adds prev/next links for recurring events (ala D16179) and moves the "accept/decline" buttons closer to the invite list. This might need some fiddling, but should be a little more human-friendly.
Test Plan: {F1740541}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16339
Summary: Ref T8116. Puts a list of packages on the publisher page, and a list of versions on the package page.
Test Plan: Viewed a publisher, saw packages. Viewed a package, saw versions. Looked at list views.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16321
Summary: Ref T8116. Add search-by-name and per-package / per-publisher search to Packages.
Test Plan: Searched publishers, packages, versions by name. Searched packages by publisher. Searched versions by package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16320
Summary:
Ref T8116. This adds a control for creating publishers (default: administrators) and default publisher/package edit controls.
I've left the edit defaults at "no one" for now to force you to select a policy. This might be something to look at later.
Test Plan: Created publishers, packages. Tried to create publishers with "can create" policy set restrictively.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16319
Summary:
Ref T8116. A version has:
- a package (like "Arcanist") which it belongs to;
- a name (like "v3.1.5").
The name is immutable and unique, like the package key and publisher key.
Policy stuff:
- Versions have the exact same policies as their packages.
- You must be able to edit a package to create new versions of it.
This is still entirely uninteresting.
Test Plan: {F1731703}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16316
Summary:
Ref T8116. A package has:
- a publisher (like "Phacility"), from the previous revision;
- a name (like "Arcanist");
- a package key (like "arcanist").
The package key is immutable, like the publisher key.
This gives a package a full key like "phacility/arcanist".
Policy stuff:
- You must be able to view a publisher to view a package (currently, everyone can always see all publishers).
- You must be able to edit a publisher to create a new package inside it.
- Packages have separate view/edit permissions.
This still does nothing interesting.
Test Plan: {F1731663}
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16315
Summary:
Ref T8116. Partially scavenged from D14152. This roughs in a new Packages application for Arcanist extensions and third-party applications, and adds a "Publisher" object.
A "Publisher" represents an individual or entity who is publishing a package, like "Phacility". It's explicitly //not// necessarily the original author -- just the primary entity vouching for the safety of the code.
A publisher just has a name and a unique key for now. For example, Phacility might have "Phacility" and "phacility", respectively.
Unique keys are immutable, e.g., the package "phacility/arcanist" will always be exactly the same package by exactly the same publisher.
Test Plan: {F1731621}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8116
Differential Revision: https://secure.phabricator.com/D16314
Summary:
Ref T11326. This isn't perfect, but should be a little easier to use and less weird/confusing.
Generally, provide a "Query > Month > Day" crumb on day views, and a "Wed, July 3" header.
Generally, provide a "Query > Month" crumb on month views, and a "July 2019" header.
Also try to fix a bit of padding/spacing on the day view.
Test Plan: {F1739128}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16338
Summary:
Ref T11326. This doesn't go quite as far as the mock in T11326#185932, but gets rid of the easy margins.
Also cleans up some of the border rules so they're simpler and more consistent (no weird ragged edges on the far right).
Test Plan: {F1738951}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16335
Summary:
Ref T11326. When an event is all-day, hide the time controls for the start/end dates. These aren't used and aren't helpful/useful.
This got a little more complicated than it used to be because EditEngine forms may have only some of these controls present.
Test Plan: Edited an all-day event; edited a normal event; swapped an event between normal and all-day.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16327
Summary:
Fixes T11375. Some validation code was mishandling raw epoch timestamps.
For numeric values larger than 29999999 (e.g., 2999-12-25, christmas 2999), assume the value is a timestamp.
Test Plan: Used `maniphest.search` to query for `modifiedStart`, got a better result set and saw the `dateModified` constraint in the query.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11375
Differential Revision: https://secure.phabricator.com/D16326
Summary:
Finishes fixing T11365. rP28199bcb48 added the new numeric entry
control and used it for TOTP setup, but missed the case of entering
a factor when TOTP was already set up.
Test Plan:
Observe behaviour of TOTP setup and subsequent factor entry
in iOS browser, make sure they're consistent.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T11365
Differential Revision: https://secure.phabricator.com/D16325
Summary:
Fixes T11365. I tested these variants:
- `<input type="number" />`
- `<input type="text" pattern="\d*" />`
Of these, this one (using `pattern`) appears to have the best behavior: it shows the correct keyboard on iOS mobile and does nothing on desktops.
Using `type="number"` causes unwanted sub-controls to appear in desktop Safari, and a numbers + symbols keyboard to appear on iOS (presumably so users can type "." and "-" and maybe ",").
Test Plan: Tested variants in desktop browsers and iOS simulator, see here and T11365 for discussion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11365
Differential Revision: https://secure.phabricator.com/D16323
Summary:
Fix T11339.
Now, old and new are both simple lists of phids, and the rendering should make sense.
Test Plan: Viewed existing transaction with all 3 states.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T11339
Differential Revision: https://secure.phabricator.com/D16311
Summary:
Fixes T11358. Entering a too-long title/subtitle currently raises an unfriendly (database-level) error.
Raise a friendlier error.
Test Plan: {F1731533}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11358
Differential Revision: https://secure.phabricator.com/D16313
Summary:
Fixes T10750. Files have some outdated cache/key code which prevents recording an edit history on file comments.
Remove this ancient cruft.
(Users must `bin/storage adjust` after upgrading to this patch to reap the benefits.)
Test Plan:
- Ran `bin/storage adjust`.
- Edited a comment in Files.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10750
Differential Revision: https://secure.phabricator.com/D16312
Summary: Ref T11326. This makes it a little easier to jump back up to check out your day.
Test Plan: {F1725575}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16309
Summary: Ref T11326. This just cleans things up a little and removes some of the obvious layout/CSS issues.
Test Plan:
- Viewed day view before/after. Also viewed profile panel.
Before:
{F1725547}
After:
{F1725548}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16308
Summary:
Ref T11326. This just inches things forward a little bit:
- Make it easier to see current day.
- Line-through cancelled events.
- Don't colorize the whole event title, just use an Attending/Invited/Custom icon.
- Slightly subtler treatment for all-day events.
Test Plan: See screenshot in T11326.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16306
Summary:
Ref T11326. Normally, events occur at a specific epoch, independent of the viewer. For example, if we're having a meeting in 35 hours, every user who looks at the event will see that it starts 35 hours from now.
But when an event is "All Day", the start time and end time depend on the //viewer//. A day like "Christmas" does not start at the same time for everyone: it starts sooner if you're in a more-eastern timezone. Baiscally, an event on "July 15th" starts whenever "July 15th" starts for whoever is looking at it.
Previously, we stored these events by using the western-most and eastern-most timezones as the start and end times (the earliest possible start and latest possible end).
This worked OK, but we get into a bunch of trouble with EditEngine, mostly because each field can be updated individually now. We can't easily tell if an event is all-day or not when reading or updating the start time and end time, and making that easier would introduce a huge amount of complexity.
Instead, when we update the start or end time, we write //two// times:
- The epoch timestamp of the time the user entered, which is the start time we will use if the event is a normal event.
- The epoch timestamp of 12:00 AM in UTC on the same date as the //local// date the user entered. This is pretty much like just storing the date the user actually typed. This is what w'ell use if the event is an all-day event.
Then, no matter whether the event is later made all-day or not, we have all the information we need to display it correctly.
Test Plan:
- Created and edited all-day events.
- Migrated existing all-day events, which appeared to survive without problems. (Note that events all-day which were created or edited in the last couple of days `master` won't survive this mutation correctly and will need to be fixed.)
- Created and edited normal, recurring, and recurring all-day events.
- Swapped back to `stable`, created an event, specifically migrated it forward, made sure it survived with times intact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16305
Summary: Ref T11326. Align this stuff with "Host" and "hostPHID".
Test Plan: Searched for events by host.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16303
Summary:
Ref T11326. This gets rid of the old multi-paged form stuff used in the last version of Diffusion.
This incidentally removes a callsite for a date control to make it a little easier to simplify them.
Test Plan: Grepped for all removed classes, no more callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16302
Summary:
Ref T11326. These are last-generation and neither of these have callsites anymore.
(I nuked these since I'm trying to simplify date handling.)
Test Plan: Grepped for callsites.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16301
Summary: Ref T11326. Use modern methods instead of building this stuff separately.
Test Plan: Used `E123`, `{E123}`, saw references render normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16300
Summary:
Ref T11326. Try to make this a little more useful:
- Don't show entire attendee list (not useful?)
- Show host (useful?)
- Show your own status prominently (attending vs declined vs invited).
- Show cancelled events prominently.
Test Plan: {F1723550}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16299
Summary:
Ref T11326. Show this information with a subheader instead of in properties.
Also, slightly simplify the list view.
Test Plan: {F1723539}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16298
Summary:
Ref T11326. Currently, events show the icon as a property, like this:
> Icon: Default
This is boring and terrible. Show the icon in the header instead:
{F1723530}
Also minor cleanup on active/cancel states.
Test Plan: Viewed an event, saw icon.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16297
Summary:
Ref T11326. Currently, we render "E (99)" for ghost instances, which is meaningless and inconsistent.
Render these more sensibly and consistently.
Test Plan: Viewed event list, saw reasonable monograms / object names.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16296
Summary:
Ref T4788. One install has some particularly impressive task graphs which are thousands of nodes large.
The current graph is pretty broken in these cases. For now, just render a "too big to show" message. In the future, I'd plan to finesse this (e.g., show parents/children, show links to parents/children, etc).
Test Plan:
- Viewed a normal task.
- Set limit to 3, viewed a task with graph size 6, saw an error message.
- Viewed a revision stack graph (unaffected).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16295
Summary:
Fixes T9224. This adds:
- A "Default Edit Policy" and "Default View Policy" to Calendar, similar to other applications.
- "Event Host" and "Event Invitees" objects policies.
These policies often end up being redundant (the host can always view/edit, the invitees can always view), but they can be more clear than setting "No One", and "Editable By: Event Invitees" is a legitimately useful policy.
Test Plan:
- Created and edited events.
- Fiddled with defaults.
- Tried to remove myself as the event host for an "Editable By: Host" event, got an error ("you wouldn't be able to edit").
- Tried to remove myself as host/invitee for an "Editable By: Invitees" event, got an error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9224
Differential Revision: https://secure.phabricator.com/D16294
Summary: Modular transactions have slightly more modern ways to express values now.
Test Plan: Looked at transaction record of a paste.
Reviewers: chad, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D16293
Summary:
Fixes T10909. I think this is a generally reasonable sort of capability to expose, although I've made it edit-only for now (when creating an event, you're always the host).
Also clean up some minor leftovers in the code, and a couple of little bugs with recurrence frequencies.
Test Plan: Created an event, edited the host of an event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10909
Differential Revision: https://secure.phabricator.com/D16292
Summary: Ref T10909. Ref T9224. We label this field "Host" in the UI; make the storage format consistent.
Test Plan:
- Viewed month view, day view, detail view of an event.
- Created a new event, saw myself as the host.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9224, T10909
Differential Revision: https://secure.phabricator.com/D16291
Summary:
Fixes T8911. This corrects several issues which could crop up if a calendar event query matched more results than the query limit:
- The desired order was not applied by the SearchEngine -- it applies the first builtin order instead. Provide a proper builtin order.
- When we generate ghosts, we can't do limiting in the database because we may select and then immediately discard a large number of parent events which are outside of the query range.
- For now, just don't limit results to get the behavior correct.
- This may need to be refined eventually to improve performance.
- When trimming events, we could trim parents and fail to generate ghosts from them. Separate parent events out first.
- Try to simplify some logic.
Test Plan: An "Upcoming" dashboard panel with limit 10 and the main Calendar "Upcoming Events" UI now show the same results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8911
Differential Revision: https://secure.phabricator.com/D16289
Summary: Ref T7944. The search method is a bit bare-bones for now, but these substantially work.
Test Plan: Edited events via API; queried events via API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7944
Differential Revision: https://secure.phabricator.com/D16288
Summary:
Fixes T10633. When generating email about a transaction which adjusts a date, render the offset explicitly (like "UTC-7").
This makes it more clear in cases like this:
- mail is being sent to multiple users, and not necessarily using the viewer's settings;
- you get some mail while travelling and aren't sure which timezone setting it generated under.
Test Plan: Rendered in text mode, saw UTC offset.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10633
Differential Revision: https://secure.phabricator.com/D16287
Summary:
Ref T9275. Swaps Calendar over to modular transactions. Theoretically, this has almost no effect on anything.
Ref T10633. I didn't actually do anything here yet, but this gets us ready to put timestamps in email.
Test Plan: Created and edited a bunch of events, nothing seemed catastrophically broken.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275, T10633
Differential Revision: https://secure.phabricator.com/D16286
Summary: Ref T9275. I waffled back and forth on these transactions a bit, but put these back here in better working order.
Test Plan: Tried to schedule an event on "taco".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16285
Summary:
Ref T9275. We were rendering too many transactions and/or over-rendering invitees.
Clean this logic up a bit:
- List all before/after invitees.
- Simplify the lists before rendering.
Test Plan: Viewed an event, edited invitees, got sensible human-readable transactions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16284
Summary:
Ref T9275. This throws away the old EditController and switches fully to EditEngine.
There's still some sketchy behavior (particularly, no JS stuff yet) but I think all the basics work properly.
Test Plan: Created and edited events via EditEngine, everything seemed to work alright.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16283
Summary: Ref T9275. This gets things roughly into shape for a cutover to EditEngine, mostly by fixing some problems with "recurrence end date" not being nullable while editing events.
Test Plan: Edited events with EditPro controller, nothing was obviously broken.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16282
Summary:
Ref T9275. This still has a number of rough edges and other minor problems (no JS on the controls, some date handling control bugs) but I'll smooth those over in future changes.
It does make all the editable transaction types available from EditEngine, technically speaking.
Test Plan: Created and edited events with the "pro" controller, which mostly worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16281
Summary:
Ref T9275. Now that TYPE_ACCEPT and TYPE_DECLINE have been separated out, we can simplify TYPE_INVITE.
This now just takes a list of invited PHIDs, uninvites ones that were removed and invites ones that were added. This is simpler, lets more logic live in the Editor, and makes EditEngine/API access easier.
Test Plan: Created events, added and removed invitees. Used comment stacked action and "pro" editor to adjust invitees.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16280
Summary:
Ref T9275. Currently, the "Start Date", "End Date", and "Recurrence End Date" transcations take a complex value (AphrontFormDateControlValue) and reduce it to an epoch.
Do this a little earlier, since the API will be much more usable if it just passes in epoch timestamps.
Events also have some logic where they rewrite the from date and to date on the actual object for all day events, then undo the changes later. Specifically, if you have an all-day event on "July 24th", the exact start and end times vary based on who is looking at it. Instead of overwriting the persistent `dateFrom` and `dateTo` properties, add separate `viewer` properties to make it easier to keep this stuff straight.
Since this means all-day events get stored in UTC, we need to query/fetch (and then discard) slightly more events. This is perfectly and much simpler to do.
The one weird "UTC" hack in here will get nuked when this moves to EditEngine properly.
Test Plan: Edited times for normal events and all-day events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16274
Summary:
Ref T9275. Currently, there's a single "invite" transaction type for managing Calendar invites, and it takes a map of invitees to status.
This isn't great for EditEngine or API access, since it lets you set anyone else to any status and we can't reuse as much code as we can with a simpler API.
Make "Accept" and "Decline" separate actions which affect the actor's invite, so "invite" can be a simpler transaction which just invites or uninvites people.
Test Plan:
- Joined/accepted/declined an event invitation.
- Edited event invitees.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16272
Summary:
Ref T9275. This moves description, icon, and cancel/uncancel to EditEngine.
It removes TYPE_SEQUENCE_INDEX and TYPE_INSTANCE_OF_EVENT. These are currently never generated and I do not expect to genereate them (instead, these changes happen automatically when you edit a stub).
Test Plan: Edited an event with normal and pro edit forms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16264
Summary:
Ref T9275. When you create a recurring event which recurs forever, we want to avoid writing an infinite number of rows to the database.
Currently, we write a row to the database right before you edit the event. Until then, we refer to it as `E123/999` or whatever ("instance 999 of event 123").
This creates a big mess with trying to make recurring events work with EditEngine, Subscriptions, Projects, Flags, Tokens, etc -- all of this stuff assumes that whatever you're working with has a PHID.
I poked at letting this stuff work without a PHID a little bit, but that looked like a gigantic mess.
Instead, generate an event "stub" a little sooner (when you look at the event detail page). This is basically just an ID/PHID to refer to the instance.
Then, when you edit the stub, "materialize" it into a real event.
This still has some issues, but I think it's more promising than the other approach was.
Also:
- Removes dead user profile calendar controller.
- Replaces comments with EditEngine comments.
Test Plan:
- Commented on a recurring event.
- Awarded tokens to a recurring event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16248
Summary:
Ref T9275. This builds a Calendar EditEngine which only edits "name".
I'll add more fields, Conduit, etc., and move to modular transactions in future changes.
Test Plan: Used `editpro/` URI manually to edit the name of an event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9275
Differential Revision: https://secure.phabricator.com/D16235
Summary:
Fixes T11307. Fixes T8124. Currently, builtin files are tracked by using a special transform with an invalid source ID.
Just use a dedicated column instead. The transform thing is too clever/weird/hacky and exposes us to issues with the "file" and "transform" tables getting out of sync (possibly the issue in T11307?) and with race conditions.
Test Plan:
- Loaded profile "edit picture" page, saw builtins.
- Deleted all builtin files, put 3 second sleep in the storage engine write, loaded profile page in two windows.
- Before patch: one of them failed with a race.
- After patch: both of them loaded.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8124, T11307
Differential Revision: https://secure.phabricator.com/D16271
Summary:
Fixes T10907. As written, this workflow will incorrectly reuse a temporary file if one exists.
Instead, make a new permanent file.
(Storage is still shared, so this usually will not actually create a copy of the file's data.)
Test Plan:
- Set a project's icon by clicking first button in "Use Picture" row.
- Before patch: temporary image was reused.
- After patch: new permanent file is generated.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10907
Differential Revision: https://secure.phabricator.com/D16270
Summary:
Fixes T11309. When checking if a repository was fully imported, we incorrectly allow unreachable, un-imported commits to prevent the repository from moving to "Imported".
This can happen if you delete branches from a repository while it is importing.
Instead, ignore unreachable commits when checking for remaining imports, and when reporting status via `bin/repository importing`.
Test Plan:
- Stopped daemons.
- Created a new repository and activated it.
- Ran `bin/repository update Rxx`.
- Deleted a branch in the repository.
- Ran `bin/repository update Rxx`.
- Ran daemons to flush queue.
Now:
- Ran `bin/repository importing`. Old behavior: showed unreachable commits as importing. New behavior: does not show unreachable commits.
- Ran `bin/repository update`. Old behavior: failed to move repository to "imported" status. New behavior: correctly moves repository to "imported" status.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11309
Differential Revision: https://secure.phabricator.com/D16269
Summary:
Ref T11309. In that task, a user misunderstood two parts of this error:
- They took "exception" to mean "unexpected failure", when it was intended to mean "rare circumstance".
- They intereted the internal ID number of a commit to mean that Phabricator was malfunctioning.
Make the language of this condition more direct, explaining what the situation means in greater detail.
Additionally, we would previously re-throw this exception, which would make the daemon exit, wait a moment, and restart. This was normal and expected.
When //unexpected// failures occur, it's important do to this: it prevents a daemon failing in a loop from causing too many side effects (e.g., limit of 1 email per 5 seconds instead of thousands per second).
When expected, permanent failures occur, we do not need to do this: the task will not be retried. I just did it because it was slightly more consistent ("failures restart daemons") and we had few permanent failure types at the time.
We have more now, and restarting the daemons generates some additional logs which have the potential to confuse. Cycling the daemon also (intentionally) reduces the rate at which we process tasks, which can be bad for permanent failures like "deleted commit" because users can delete a huge number of commits and possibly clog up the queue with cycle-after-failure actions.
Test Plan:
Tried to process a deleted commit, saw a new message:
```
2016-07-11 9:30:22 AM [STDE] <VERB> PhabricatorTaskmasterDaemon Task 1428658 was cancelled: Commit "R55:6c46b7d0fb82a859ca3f87a95dc8dcceef8088c9" (with internal ID "282161") is no longer reachable from any branch, tag, or ref in this repository, so it will not be imported. This usually means that the branch the commit was on was deleted or overwritten.
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11309
Differential Revision: https://secure.phabricator.com/D16268
Summary:
Fixes T11304. Prior to this change, we did an unnecessary write on every "*.search" call (this write didn't always actually write a row, since we only save //unique// saved queries, but still doesn't do anything useful ever, currently).
Instead, change this to not-write by default. We could add an "oh, and also I want you to do a write" option later, which would let us implement something like `arc query-stuff` which says "To see more results, view this URI in your browser: ...".
(It's possible to run one of these methods with an existing SavedQuery by using the key, so we still sometimes have a queryKey to return.)
Test Plan: Ran `almanac.service.search`, used DarkConsole to verify that no serachengine writes occurred.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11304
Differential Revision: https://secure.phabricator.com/D16263
Summary:
Ref T10423. This flag can cause `git diff` to take an enormously long time (the problem case was a 5M line, 20K file commit).
Instead:
- Run without the flag first.
- If that shows that the diff is definitely small, try again with the flag.
- If that works, return the slower, better output.
- If the fast diff affects too many paths or generating the slow diff takes too long, return the faster, slightly worse output.
The quality of the output differs in how well Git is able to detect "M" and "C" (moves and copies of files).
For example, if you copy `src/` to `srcpro/`, the fast output may not show that you copied files. The slow output will.
I think this is rarely useful for large copies anyway: it's interesting if a 1-2 file diff is a copy, but usually obvious/uninteresting if a 500-file diff is a copy.
Test Plan:
- Ran `bin/repository reparse --change rXnnn` on Git changes.
- Saw fast and slow commands execute normally.
- Tried on a large diff, saw only the fast command execute.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10423
Differential Revision: https://secure.phabricator.com/D16266
Summary: Fixes T11305, Ref T7754. Makes this menu dropdown act like actions and collapse to a fa-bars menu.
Test Plan:
View on mobile, desktop, browser. Click an action, spawn new page.
{F1717953}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7754, T11305
Differential Revision: https://secure.phabricator.com/D16265
Summary I broke this in D16237: that made the CLI workflow work, but we attach the repository earlier in the web workflow and won't have one when we arrive here.
Test Plan: Created a new repository URI from the web UI.
Auditors: chad
Summary: fix T11290.
Test Plan: Paste language type, view in web and in emails (It uses quotes in HTML emails, which I think is something else).
Reviewers: epriestley, chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T11290
Differential Revision: https://secure.phabricator.com/D16252
Summary:
Fixes T11097. Currently, popup notifications show a useless timestamp with the current time, after D16041 made some things more consistent.
Strip these from the popup bubbles.
Test Plan:
- Saw a popup bubble, no timestamp.
- Viewed main notification list, saw timestamps.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11097
Differential Revision: https://secure.phabricator.com/D16258
Summary: Fixes T11278. Also mention `svnsync`, since we have some evidence that it works.
Test Plan: {F1716250}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11278
Differential Revision: https://secure.phabricator.com/D16255
Summary: Fixes T11146. Allow no slug in the URI.
Test Plan: Previewed root document in Phriction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11146
Differential Revision: https://secure.phabricator.com/D16257
Summary:
Fixes T11295. Prior to this change, the "404 page" for daemon tasks fatals.
This page is special cased a little bit and not a normal 404 page, because it's possible for you to click a valid link and the task to get GC'd by the time you load the page, or similar. It tries to be a little more user-friendly than a bare 404.
Test Plan:
- Visited `/daemon/task/1428348920328/` (any invalid ID).
- Now got a nice "no such task" page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11295
Differential Revision: https://secure.phabricator.com/D16254
Summary:
This is the quickest and dirtiest fix I could come up with.
`PhabricatorApplicationTransaction::getTitleForMail()` is using `clone $this`, which doesn't actually effect `implementation`.
Ref T9789.
Test Plan: update paste comment, get plaintext mail.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16251
Summary:
Fixes T11285. We can end up loading panel handles while processing edits (e.g., disabling menu items on a project). We probably started loading these after the modular transaction changes in T9789, which load the handle for the transaction object unconditionally.
The handles aren't too useful, but they currently fail to load/build because panels don't have a URI. We could give them some sort of method here, but just nuke it for now since they don't appear anywhere and this unclogs the daemon queue.
Test Plan:
- Disabled a menu item on a project.
- Ran publish task with `bin/worker execute --id <id>`.
- Before patch: fatal on getURI() with stack trace similar to T11285.
- After patch: clean execution.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11285
Differential Revision: https://secure.phabricator.com/D16249
Summary: See Z2352#28072. Expose this flag to allow callers to take actions after an import finishes, which is generally reasonable.
Test Plan: Ran query from console, saw `isImporting` flag in results.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16247
Summary:
Ref T9360. Old docs felt a little weird to me (particularly very-old text like "favoring the individual rather than the collective").
Try a simpler tone focused more on use cases and examples?
Test Plan:
Read documentation.
Also, viewed a post list and saw monograms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9895, T9360
Differential Revision: https://secure.phabricator.com/D16246
Summary: Show the J monogram when internally linked, but nothing externally (cleaner UI). Ref T9360
Test Plan: View post live and internal.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16245
Summary: Ref T9360, forces https if we say the blog is https.
Test Plan: Fake an https, get redirected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16241
Summary: Ref T9360. These weren't getting set properly, also make them nullable since they're optional.
Test Plan: run upgrade, make a new blog with and without a parent domain. Edit a current blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16242
Summary:
Ref T9360. Moves PhamePost to CommentEditEngine.
[x] HTTP Parameters dropdown on New Post goes to 404
[x] Implement EditEngine Comments
Test Plan: Make Post, Make Comment, Laugh.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16222
Summary: Fixes T11280. I extracted this at the last minute and got the constant flipped.
Test Plan: Archived, then activated a paste. Observed correct timeline stories/icons/etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11280
Differential Revision: https://secure.phabricator.com/D16240
Test Plan: Tested with a transactionType from an extension.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16236
Summary: Fixes T11275. This search query doesn't actually have any options so these links are a little pointless, but generate valid links instead of 404s.
Test Plan: Clicked "Advanced Search" and "Edit Queries" from `/settings/`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11275
Differential Revision: https://secure.phabricator.com/D16238
Summary: Fixes T11276. This feels slightly iffy (we `attachRepository()` here, and also when applying the TYPE_REPOSITORY transaction) but simpler than trying to reorder things.
Test Plan: Created a repository URI with transactions in `["uri", "repository"]` order.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11276
Differential Revision: https://secure.phabricator.com/D16237
Summary:
Fixes T11269. The basic issue is that `git log` in an empty repository exits with an error message.
Prior to recent Git (2.6?), this message reads:
> fatal: bad default revision 'HEAD'
This message was somewhat recently changed by <ce11360467>. After that, it reads:
> fatal: your current branch 'master' does not have any commits yet
This change isn't //technically// a //complete// fix because you could still hit this issue like this:
- Create an empty repository.
- Push some stuff to `master`.
- Delete `master`.
However, this is very rare and even in this case the repository will fix itself once you push something again. We can try to fix that if any users ever actually hit it.
Test Plan:
- Created a new empty Git repository.
- Ran `bin/repository update Rxx`.
- Before patch: "git log" error because of the empty repository.
- After patch: clean update.
- Also ran `repository update` on a non-empty repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11269
Differential Revision: https://secure.phabricator.com/D16234
Summary: Ref T9640. Fixes T9888. Decline to support PHP 7 until the async signal handling issue in T11270 is resolved.
Test Plan: Faked local version, got helpful error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9640, T9888
Differential Revision: https://secure.phabricator.com/D16231
Summary: Fixes T11243. Seems reasonable to open this stuff in a new window so you don't put any application state in Herald, etc., at risk -- looking in this menu for help with a currently-executing workflow is reasonable and normal.
Test Plan: Clicked a help menu link, saw it open in a new page.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11243
Differential Revision: https://secure.phabricator.com/D16230
Summary: Fixes T11267. This data was coming back weird (in reverse order relative to the graph itself). Previously it worked OK anyway, but the new logic is a little more sensitive to the input.
Test Plan: Viewed a Mercurial repository with linear history, saw linear history.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11267
Differential Revision: https://secure.phabricator.com/D16229
Summary: Ref T11244. 8 more tokens. Probably need better math on the selector?
Test Plan: Award Dat Boi.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: putnam, Korvin
Maniphest Tasks: T11244
Differential Revision: https://secure.phabricator.com/D16228
Summary:
Ref T10855. This can't replace the old edit flow yet, but get the basics in place.
(This is actually much closer to just being able to swap than I anticipated since CustomFields sort of just work, but the exiting flow has some "clone existing panel" / "place directly on dashboard" stuff that this doesn't yet.)
Test Plan: Created and edited a panel by manually using the "editpro" flow.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10855
Differential Revision: https://secure.phabricator.com/D16226
Summary:
Ref T4788. As it turns out, our tasks are very tightly connected.
Instead of loading every parent/child task, then every parent/child of those tasks, etc., etc., only load tasks in the "same direction" that we're already heading.
For example, we load children of children, but not parents of children. And we load parents of parents, but not children of parents.
Basically we only go "up" and "down" now, but not "out" as much. This should reduce the gigantic multiple-thousand-node graphs currently shown in the UI.
I still discover the whole graph for revisiosn, because I think it's probably more useful and always much smaller. That might need adjustment too, though.
Test Plan: Seems fine locally??
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16218
Summary: Ref T9360. This makes these transactional.
Test Plan: Set new header, delete header. Set new profile image, reset profile image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16217
Summary: Ref T4788. This seems reasonable locally, but not sure how it will feel on real data. Might need some tweaks, or might just be a terrible idea.
Test Plan: {F1708059}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16214
Summary:
Ref T4788. This separates the revision graph view into a base class with core logic and a revision class with Differential-specific logic, so I can subclass it in Maniphest, etc., and try using it in other applications to show similar graphs.
Not sure if we'll stick with it, but even if we don't this makes the code a bit cleaner and gets custom rendering logic out of the RevisionViewController, which is nice.
Test Plan: Viewed revisions, saw the stack UI completely unchanged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16213
Summary: New tokens, slightly larger (18x18 vs 16x16). I think these all feel decent, I might tweak the thumbs icons a little more color-wise.
Test Plan:
Use Tokens.
{F1707411}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11244
Differential Revision: https://secure.phabricator.com/D16211
Summary: Ref T10628. Turn these into tabs in a single box, since "local commits" and "similar revisions" are of particularly rare use.
Test Plan: {F1707196}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10628
Differential Revision: https://secure.phabricator.com/D16209
Summary: Ref T10628. This moves everything else over. I'll clean up the cruft in the next diff.
Test Plan:
- Viewed Conduit API page, toggled tabs.
- Viewed Harbormaster build, toggled tabs.
- Viewed a Drydock lease, swapped tabs.
- Viewed a Drydock resource, swapped tabs.
- Viewed mail, swapped tabs.
- Grepped for `addPropertyList(...)`, looked for any remaining calls with a second argument.
- Also checked rSAAS for any calls, but we don't have anything there that uses tabs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10628
Differential Revision: https://secure.phabricator.com/D16207
Summary:
Ref T10628. Switch this to be nicer and more modern.
- When there's only one tab, add an option to hide it.
Test Plan:
- Viewed normal revisions (no tabs).
- Viewed X vs Y revisions (two tabs, rightmost tab selected by default).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10628
Differential Revision: https://secure.phabricator.com/D16206
Summary:
Ref T10628. Currently, tabs are part of ObjectBoxes. However, the code is a bit of a mess and I want to use them in some other contexts, notably the "prose diff" dialog to show "old raw, new raw, diff".
Pull them out, and update Files to use the new stuff. My plan is:
- Update all callsites to this stuff.
- Remove the builtin-in ObjectBox integration to simplify ObjectBox a bit.
- Move forward with T10628.
This is pretty straightforward. A couple of the sigils are a little weird, but I'll update the JS later. For now, the same JS can drive both old and new tabs.
Test Plan: Viewed files, everything was unchanged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10628
Differential Revision: https://secure.phabricator.com/D16205
Summary:
Fixes T11242. See that task for detailed discussion.
Previously, it didn't particularly matter that we don't MIME detect chunked files since they were all just big blobs of junk (PSDs, zips/tarballs, whatever) that we handled uniformly.
However, videos are large and the MIME type also matters.
- Detect the overall mime type by detecitng the MIME type of the first chunk. This appears to work properly, at least for video.
- Skip mime type detection on other chunks, which we were performing and ignoring. This makes uploading chunked files a little faster since we don't need to write stuff to disk.
Test Plan:
Uploaded a 50MB video locally, saw it as chunks with a "video/mp4" mime type, played it in the browser in Phabricator as an embedded HTML 5 video.
{F1706837}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11242
Differential Revision: https://secure.phabricator.com/D16204
Summary:
Ref T4788. When closing a task as a duplicate of another task, you can only select one task, since it doesn't really make sense to merge one task into several other tasks (this operation is //possible//, but probably not what anyone ever wants to do, I think?).
Make the UI understand this: after you select a task, disable all of the "select" buttons in the UI to make this clear.
Test Plan:
- Used "Close as Duplicate", only allowed to select 1 task.
- Used other editors like "Merge Duplicates In", allowed to select lots of tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16203
Summary: Ref T9360. This adds ability to search posts by blog(s) and by type better.
Test Plan:
Create some posts, search for them.
{F1705961}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16199
Summary:
Ref T4788. Fixes T10703.
In the longer term I want to put this on top of ApplicationSearch, but that's somewhat complex and we're at a fairly good point to pause this feature for feedback.
Inch toward that instead: provide more appropriate filters and defaults without rebuilding the underlying engine. Specifically:
- No "assigned" for commits (barely makes sense).
- No "assigned" for mocks (does not make sense).
- Default to "open" for parent tasks, subtasks, close as duplicate, and merge into.
Also, add a key to the `search_document` table to improve the performance of the "all open stuff of type X" query. "All Open Tasks" is about 100x faster on my machine with this key.
Test Plan:
- Clicked all object relationships, saw more sensible filters and defaults.
- Saw "open" query about 100x faster locally (300ms to 3ms).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T10703
Differential Revision: https://secure.phabricator.com/D16202
Summary: Fixes T11240. Also simplify things a little and share a bit more code.
Test Plan:
- Viewed revisions and tasks, opened submenu.
- Viewed as a user without edit permission, saw the menus greyed out.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11240
Differential Revision: https://secure.phabricator.com/D16201
Summary:
Ref T4788. Fixes T7820. This updates the "Merge Duplicates In" interaction, and adds a "Close as Duplicate" action.
These are the last interactions that were using the old code, so it removes that code.
Merges are now recorded as real edges, so we can show them in the UI later on (originally from T9390, etc).
Also provides more general support for relationships which need EDIT permission, not-undoable relationships like merges, preventing relating an object to itself, and relationship side effects like merges.
Finally, fixes a couple of behaviors around typing an exact object name (like `T123`) to find the related object.
Test Plan:
- Merged tasks into the current task.
- Closed the current task as a duplicate of another task.
- Edited other relationships.
- Searched for tasks, commits, etc., by object monogram.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T7820
Differential Revision: https://secure.phabricator.com/D16196
Summary:
The first dialog was being given the wrong user (`$user`, should be `$viewer`), leading to a CSRF issue.
(The CSRF token it generated was invalid in all validation contexts, so this wasn't a security problem or a way to capture CSRF tokens for other users.)
Use `newDialog()` instead.
(This seems completely unrelated to the vaguely-similar-looking issues we saw earlier this week.)
Test Plan:
- Added a new email address.
- Clicked "Done" on the last step.
- Completed workflow instead of getting a CSRF error.
Reviewers: chad, tide
Reviewed By: tide
Differential Revision: https://secure.phabricator.com/D16200
Summary:
Ref T9360.
[x] View Live useless on archived blogs
[x] Edit Blog Image treatment like profiles
[x] Pager next/prev should keep you on whatever view you're on
[x] Unset user titles aren't falling back properly
[x] Add captions to edit fields for better clarification
Test Plan: Archive a blog, Edit a photo, verify pager on live and internal blogs, check empty titles, and view new edit form instructions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D16197
Summary: Ref T4788. This moves everything except "merge" to the new code.
Test Plan:
- Edited relationships in Differential, Diffusion, and Pholio.
- Uninstalled Pholio, made sure "Edit Mocks..." actions vanished.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16193
Summary:
Ref T4788. Fixes T9232. This moves the "search for stuff to attach to this object" flow away from hard-coding and legacy constants and toward something more modular and flexible.
It also adds an "Edit Commits..." action to Maniphest, resolving T9232. The behavior of the search for commits isn't great right now, but it will improve once these use real ApplicationSearch.
Test Plan: Edited a tasks' related commits, mocks, tasks, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T9232
Differential Revision: https://secure.phabricator.com/D16189
Summary: Fixes T11223. I missed a few of these; most of them kept working anyway because we have redirects in place, but make them a bit more modern/not-hard-coded.
Test Plan:
- Generated and revoked API tokens for myself.
- Generated and revoked API tokens for bots.
- Revoked temporary tokens for myself.
- Clicked the link to the API tokens panel from the Conduit console.
- Clicked all the cancel buttons in all the dialogs, too.
In all cases, everything now points at the correct URIs. Previously, some things pointed at the wrong URIs (mostly dealing with stuff for bots).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11223
Differential Revision: https://secure.phabricator.com/D16185
Summary:
Ref T9838.
Add a Properties field to Revision, and update a `wasAcceptedBeforeClose` when closing a revision.
Test Plan:
A quick run through the obvious steps (Close with commit/manually, with or w/o accept) and calling `differential.query` shows the `wasAcceptedBeforeClose` property was setup correctly.
Pushing closed + accepted passes the relevant herald, which was my immediate issue; Pushing un-accepted is blocked.
Test the "commit" rule (Different from "pre-commit") by hacking the DB and running the "has accepted revision" rule in a test-console.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T9838
Differential Revision: https://secure.phabricator.com/D15085
Summary: Ref T9897. This moves "Domain" to "DomainFullURI" to allow setting of https or for some reason, a port. I guess.
Test Plan: Try to break by setting a path, or fake protocol. Set to http, or https, see correct redirects. Verify domain still gets written.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16173
Summary:
Ref T11208. See that task for a more detailed description of revprops.
This allows revprop changes in a hosted Subversion repository if the repository has the "allow dangerous changes" flag set.
In the future, we could expand this into real Herald support, but the only use case we have for now is letting `svnsync` work.
Test Plan:
Edited revprops with `svn propset --revprop -r 2 propkey propvalue repositoryuri`:
- Tried before patch, got a "configure a commit hook" error.
- Tried after patch, got a "dangerous change" error.
- Allowed dangerous changes.
- Did a revprop edit.
- Prevented dangerous changes.
- Got an error again.
- Made a normal commit to an SVN repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11208
Differential Revision: https://secure.phabricator.com/D16174
Summary: Fixes T11164. At least, this fixes it locally for me. I don't know how to code. Copy Pasta!
Test Plan: Change name, don't see extra timeline entry on quality set anymore.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11164
Differential Revision: https://secure.phabricator.com/D16169
Summary: Fixes T11166. Adds some class, and space to the preview widget.
Test Plan: Test Maniphest, Ponder, etc, without a footer.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11166
Differential Revision: https://secure.phabricator.com/D16168
Summary: Fixes T11198. These are confusing or premature if you aren't an activated user: disabled or unapproved accounts won't be able to act on them.
Test Plan: Changed timezone, went through flow to correct it
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11198
Differential Revision: https://secure.phabricator.com/D16167
Summary:
Ref T11179. This splits "Edit Blocking Tasks" into two options now that we have more room ("Edit Parent Tasks", "Edit Subtasks").
This also renames "Blocking" tasks to "Subtasks", and "Blocked" tasks to "Parent" tasks. My goals here are:
- Make the relationship direction more clear: it's more clear which way is up with "parent" and "subtask" at a glance than with "blocking" and "blocked" or "dependent" and "dependency".
- Align language with "Create Subtask".
- To some small degree, use more flexible/general-purpose language, although I haven't seen any real confusion here.
Fixes T6815. I think I narrowed this down to two issues:
- Just throwing a bare exeception (we now return a dialog explicitly).
- Not killing open transactions when the cyclec check fails (we now kill them).
Test Plan:
- Edited parent tasks.
- Edited subtasks.
- Tried to introduce graph cycles, got a nice error dialog.
{F1697087}
{F1697088}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6815, T11179
Differential Revision: https://secure.phabricator.com/D16166
Summary:
Ref T11179. Ref T4768. Currently, on `master`, if two users open "Edit Revisions" at the same time, then add revisions A and B, only the last state wins (just "B").
Instead, apply these as "add A" and "add B" so they merge in a natural way.
Test Plan:
- Opened edit dialog in two windows.
- Added "A" in one, "B" in the other.
- Saved both.
- Saw "Added A" and "Added B" transactions, instead of "Added A" and "Removed A, added B".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4768, T11179
Differential Revision: https://secure.phabricator.com/D16164
Summary: Ref T11179. This is basically a "pro" controller to replace the SearchAttach controller. It does basically the same stuff, just in a (mostly) more modern and modular way.
Test Plan:
- Added and removed mocks.
- Added and removed revisions.
- Everything worked just like it did before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16163
Summary:
Ref T11179. This generates the Maniphest menu items in a modular way. It doesn't change any of the underlying code yet.
Searching for commits doesn't work particularly well so I've just hidden that for now, but the item itself works fine.
Test Plan: {F1696849}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16162
Summary: Fixes T11193. Assume this is the correct place to check for permissions before attaching edges.
Test Plan: Create a task and set edit policy to Admins, log into test account. Try to Edit Subtasks, Merge Duplicates, Attach a Diff, or Attach a Mock, get a Policy Dialog explaing why.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11193
Differential Revision: https://secure.phabricator.com/D16161
Summary: Fixes T11190. The div with all the stuff in it was sometimes ending up on top of the "select" button, making it unclickable.
Test Plan: Clicked "select" in several browsers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11190
Differential Revision: https://secure.phabricator.com/D16160
Summary: This makes it more natural to write Herald rules about commits that appear on any or no branches.
Test Plan: Wrote a commit rule for commits on any branch, ran it with `bin/repository reparse --herald <commit>`, saw expected results in web UI.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16158
Summary:
Ref T11179. Alternative to D16152. I think this turned out a bit better than the other one did.
Currently, we render two copies of the menu (one for mobile, one for desktop). A big chunk of this is sharing the nodes instead: when you open the mobile dropdown menu, it steals the nodes from the document. When you close it, it puts them back. Magic! Sneaky!
Test Plan:
{F1695499}
{F1695500}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16157
Summary: Ref T11034. Ref T4788. This allows you to resize the typeahead browse dialog if you want. I plan to let you resize the object selector dialog in the future.
Test Plan: {F1695433}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788, T11034
Differential Revision: https://secure.phabricator.com/D16156
Summary:
Ref T11034. This seems a little more promising. Two problems at the moment:
- This doesn't actually provide any useful information at all right now.
- Many object types have no profile images.
Test Plan:
{F1695254}
{F1695255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11034
Differential Revision: https://secure.phabricator.com/D16155
Summary:
Ref T11179. One issue I'm getting with trying to turn actions into dropdowns is that we currently render this menu very late, which can cause us to try to add more metadata after we start resolving metadata. This won't work right now (and making it work seems unreasonably complicated), so stop doing it and fatal if something tries.
(This might make some things fatal but //should// be safe -- anything that fatals should have been broken already.)
Test Plan:
Browsed around looking for fatals, didn't see any.
(This primarily avoids a broken state / fatal in a future diff.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11179
Differential Revision: https://secure.phabricator.com/D16151
Summary: Ref T9897. Adds a Parent Site and Parent Domain field to allow external sites to link back to parent.
Test Plan: Set up ```local.blog.phacility.com```, set parent site to "Phacility" and parent domain to "local.www.phacility.com". Get new crumbs at Blog and Post levels.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16150
Summary:
Fixes T11180. In Git, it's possible to tag a tag (????). When you do, we try to log the tag-object, which automatically resolves to the commit and fails.
Just skip these. If "A" points at "B" which points at "C", it's fine to ignore "A" and "B" since we'll get the same stuff when we process "C".
Test Plan:
- Tagged a tag.
- Pushed it.
- Discovered it.
- Before patch: got exception similar to the one in T11180.
- After patch: got tag-tag skipped. Also got slightly better error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11180
Differential Revision: https://secure.phabricator.com/D16149
Summary: Reading the code, this seems correct, but I don't have a local test. Ref T9897
Test Plan: read carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16148
Summary: Makes the crumbs background and border disappear in the live view of Phame.
Test Plan: Go live, see no crumb bg. Test blog, post, mobile, desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16146
Summary: Adds a new header layout for Phame Blog. Subtitles now also.
Test Plan:
With Image, With Subtitle, Without Image, Without Subtitle. Mobile, Tablet, Desktop.
{F1691506}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16147
Summary:
Ref T11153. If you have a build plan like this:
- Lease machine A.
- Lease machine B.
- Run client-tests on machine A.
- Run server-tests on machine B.
...and we get machine A quickly, then finish the tests, we currently do not release machine A until the whole plan finishes.
In the best case, this wastes resources (something else could be using that machine for a while).
In a worse case, this wastes a lot of resources (if machine B is slow to acquire, or the server tests are much slower than the client tests, machine A will get tied up for a really long time).
In the absolute worst case, this might deadlock things.
Instead, release artifacts as soon as no waiting/running steps take them as inputs. In this case, we'd release machine A as soon as we finished running the client tests.
In the case where machines A and B are resources of the same type, this should prevent deadlocks. In all cases, this should improve build throughput at least somewhat.
Test Plan:
I wrote this build plan which runs a "fast" step (10 seconds) and a "slow" step (120 seconds):
{F1691190}
Before the patch, running this build plan held the lease on the "fast" machine for the full 120 seconds, then released both leases at the same time at the very end.
After this patch, I ran this plan and observed the "fast" lease get released after 10 seconds, while the "slow" lease was held for the full 120.
(Also added some `var_dump()` into things to sanity check the logic; it appeared correct.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11153
Differential Revision: https://secure.phabricator.com/D16145
Summary: Adds a view live and view internal link to the blog and crumbs manage page.
Test Plan: Click on new links.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16142
Summary: Fixes T10901. Allows blogs to have headers. I've built this in a basic way, any file, max-height is 240. Should bleed into top crumbs, so any spacing you want you should add to the file itself. Might have to see how users break this.
Test Plan: Set a blog header, see blog header, remove blog header, see no blog header. Check mobile, tablet, desktop break points.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10901
Differential Revision: https://secure.phabricator.com/D16141
Summary: This is the backend half of uploading an image as a header for Phame Blogs. Allows you to upload image, or delete it. Ref T10901
Test Plan:
Go to Manage Blog, visit Edit Header Image, Upload snarky file. See snarky file on Manage page. Edit Header Image, click delete, save, see file goes away.
{F1690966}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10901
Differential Revision: https://secure.phabricator.com/D16140
Summary:
This test has been failing occasionally in a way that does not reproduce, and only when no one is looking at it.
Try to add some extra assertions to maybe get more information.
Test Plan: `arc unit`
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16137
Summary: Ref T9028. When selecting refs, pretend refs in "refs/remotes/" that we don't otherwise recognize don't exist, since it looks like these are probably remotes //of the remote// we're observing, and who knows what state they're in.
Test Plan: Used `bin/repository discover --verbose` to verify that these named refs no longer appear in the list.
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16136
Summary:
Ref T9028. Mostly, this gives them a strikethru style.
(I think this is probably the right definition of "closed" for commits. Another definition might be "audited", but I don't think completing audits really "closes" a commit.)
Test Plan: {F1689662}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16135
Summary: Ref T9028. Ref T6878. This rule should probably be refined in the long term, but for now just ignore "phabricator/diff/12424" and similar staging area tags.
Test Plan: Ran `bin/repository discover --verbose` on a repository with staging area refs, saw Phabricator ignore those refs as untracked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6878, T9028
Differential Revision: https://secure.phabricator.com/D16134
Summary:
Ref T9028. This allows us to detect when commits are unreachable:
- When a ref (tag, branch, etc) is moved or deleted, store the old thing it pointed at in a list.
- After discovery, go through the list and check if all the stuff on it is still reachable.
- If something isn't, try to follow its ancestors back until we find something that is reachable.
- Then, mark everything we found as unreachable.
- Finally, rebuild the repository summary table to correct the commit count.
Test Plan:
- Deleted a ref, ran `pull` + `refs`, saw oldref in database.
- Ran `discover`, saw it process the oldref, mark the unreachable commit, and update the summary table.
- Visited commit page, saw it properly marked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16133
Summary:
Ref T9028. This corrects the reachability of existing commits in a repository.
In particular, it can be used to mark deleted commits as unreachable.
Test Plan:
- Ran it on a bad repository, with bad args, etc.
- Ran it on a clean repo, got no changes.
- Marked a reachable commit as unreachable, ran script, got it marked reachable.
- Started deleting tags and branches from the local working copy while running the script, saw greater parts of the repository get marked unreachable.
- Pulled repository again, everything automatically revived.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16132
Summary:
Ref T9028. This improves the daemon behavior for unreachable commits. There is still no way for commits to become marked unreachable on their own.
- When a daemon encounters an unreachable commit, fail permanently.
- When we revive a commit, queue new daemons to process it (since some of the daemons might have failed permanently the first time around).
- Before doing a step on a commit, check if the step has already been done and skip it if it has. This can't happen normally, but will soon be possible if a commit is repeatedly deleted and revived very quickly.
- Steps queued with `bin/repository reparse ...` still execute normally.
Test Plan:
- Used `bin/repository reparse` to run every step, verified they all mark the commit with the proper flag.
- Faked the `reparse` exception in the "skip step" code, used `repository reparse` to skip every step.
- Marked a commit as unreachable, ran `discover`, saw daemons queue for it.
- Ran daemons with `bin/worker execute --id ...`, saw them all skip + queue the next step.
- Marked a commit as unreachable, ran `bin/repository reparse` on it, got permanent failures immediately for each step.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16131
Summary:
Ref T9028. This is the easy part of dealing with deleted commits:
- Add a flag for unreachable commits (nothing sets this flag yet).
- Ignore unreachable commits when querying for known commits during discovery, so we pretend they do not exist.
- When recording a commit, try just reviving an existing unreachable commit first. If that works, bail out.
Test Plan:
- Artificially marked a commit as unreachable with raw SQL.
- Verified it said "deleted: unreachable" in the UI.
- Ran `repository discover --trace --verbose`.
- Saw the discovery process ignore the commit when filling the cache.
- Saw the discovery process revive the commit instead of trying to record it again.
- Web UI now shows the commit as normal.
- Running `repository discover` again doesn't make any further changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9028
Differential Revision: https://secure.phabricator.com/D16130
Summary:
Ref T9028. Fixes T6878. Currently, we only fetch and discover branches. This is fine 99% of the time but sometimes commits are pushed to just a tag, e.g.:
```
git checkout <some hash>
nano file.c
git commit -am '...'
git tag wild-wild-west
git push origin wild-wild-west
```
Through a similar process, commits can also be pushed to some arbitrary named ref (we do this for staging areas).
With the current rules, we don't fetch tag refs and won't discover these commits.
Change the rules so:
- we fetch all refs; and
- we discover ancestors of all refs.
Autoclose rules for tags and arbitrary refs are just hard-coded for now. We might make these more flexible in the future, or we might do forks instead, or maybe we'll have to do both.
Test Plan:
Pushed a commit to a tag ONLY (`vegetable1`).
<cf508b8de6>
On `master`, prior to the change:
- Used `update` + `refs` + `discover`.
- Verified tag was not fetched with `git for-each-ref` in local working copy and the web UI.
- Verified commit was not discovered using the web UI.
With this patch applied:
- Used `update`, saw a `refs/*` fetch instead of a `refs/heads/*` fetch.
- Used `git for-each-ref` to verify that tag fetched.
- Used `repository refs`.
- Saw new tag appear in the tags list in the web UI.
- Saw new refcursor appear in refcursor table.
- Used `repository discover --verbose` and examine refs for sanity.
- Saw commit row appear in database.
- Saw commit skeleton appear in web UI.
- Ran `bin/phd debug task`.
- Saw commit fully parse.
{F1689319}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T6878, T9028
Differential Revision: https://secure.phabricator.com/D16129
Summary:
Ref T11140. This makes encryption actually work:
- Provide a new configuation option, `keyring`, for specifying encryption keys.
- One key may be marked as `default`. This activates AES256 encryption for Files.
- Add `bin/files generate-key`. This is helps when generating valid encryption keys.
- Add `bin/files encode`. This changes the storage encoding of a file, and helps test encodings and migrate existing data.
- Add `bin/files cycle`. This re-encodes the block key with a new master key, if your master key leaks or you're just paraonid.
- Document all these options and behaviors.
Test Plan:
- Configured a bad `keyring`, hit a bunch of different errors.
- Used `bin/files generate-key` to try to generate bad keys, got appropriate errors ("raw doesn't support keys", etc).
- Used `bin/files generate-key` to generate an AES256 key.
- Put the new AES256 key into the `keyring`, without `default`.
- Uploaded a new file, verified it still uploaded as raw data (no `default` key yet).
- Used `bin/files encode` to change a file to ROT13 and back to raw. Verified old data got deleted and new data got stored properly.
- Used `bin/files encode --key ...` to explicitly convert a file to AES256 with my non-default key.
- Forced a re-encode of an AES256 file, verified the old data was deleted and a new key and IV were generated.
- Used `bin/files cycle` to try to cycle raw/rot13 files, got errors.
- Used `bin/files cycle` to cycle AES256 files. Verified metadata changed but file data did not. Verified file data was still decryptable with metadata.
- Ran `bin/files cycle --all`.
- Ran `encode` and `cycle` on chunked files, saw commands fail properly. These commands operate on the underlying data blocks, not the chunk metadata.
- Set key to `default`, uploaded a file, saw it stored as AES256.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16127
Summary:
Ref T11140. This doesn't do anything yet since there's no way to enable it and no way to store master keys.
Those are slightly tougher problems and I'm not totally satisfied that I have an approach I really like for either problem, so I may wait for a bit before tackling them. Once they're solved, this does the mechanical encrypt/decrypt stuff, though.
This design is substantially similar to the AWS S3 server-side encryption design, and intended as an analog for it. The decisions AWS has made in design generally seem reasonable to me.
Each block of file data is encrypted with a unique key and a unique IV, and then that key and IV are encrypted with the master key (and a distinct, unique IV). This is better than just encrypting with the master key directly because:
- You can rotate the master key later and only need to re-encrypt a small amount of key data (about 48 bytes per file chunk), instead of re-encrypting all of the actual file data (up to 4MB per file chunk).
- Instead of putting the master key on every server, you can put it on some dedicated keyserver which accepts encrypted keys, decrypts them, and returns plaintext keys, and can send it 32-byte keys for decryption instead of 4MB blocks of file data.
- You have to compromise the master key, the database, AND the file store to get the file data. This is probably not much of a barrier realistically, but it does make attacks very slightly harder.
The "KeyRing" thing may change once I figure out how I want users to store master keys, but it was the simplest approach to get the unit tests working.
Test Plan:
- Ran unit tests.
- Dumped raw data, saw encrypted blob.
- No way to actually use this in the real application yet so it can't be tested too extensively.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16124
Summary: Fixes T11156. These were never correct, but also never actually used until I made timelines load object handles unconditionally in D16111.
Test Plan: Viewed an auth provider with transactions, no more fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11156
Differential Revision: https://secure.phabricator.com/D16128
Summary: Uses PHUITwoColumnView in Blog Manage and Blog Picture. Ref T9897
Test Plan: Use each page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16126
Summary: Adds some ordering options to PhamePost queries. Works on search, PhameHome, BlogHome
Test Plan: Try searching with Order By set to Date Published in application search, get correct order. Check a blog home page, check PhameHome.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16125
