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
