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
Summary:
Ref T11140. When reading and writing files, we optionally apply a "storage format" to them.
The default format is "raw", which means we just store the raw data.
This change modularizes formats and adds a "rot13" format, which proves formatting works and is testable. In the future, I'll add real encryption formats.
Test Plan:
- Added unit tests.
- Viewed files in web UI.
- Changed a file's format to rot13, saw the data get rotated on display.
- Set default format to rot13:
- Uploaded a small file, verified data was stored as rot13.
- Uploaded a large file, verified metadata was stored as "raw" (just a type, no actual data) and blob data was stored as rot13.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16122
Summary: Ref T11142. H264 video in a Quicktime container works in Safari and Firefox for me (although not Chrome), so include it in the default video mime types.
Test Plan: Uploaded video file from T11142 locally, saw it render with `<video />` properly in Safari.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11142
Differential Revision: https://secure.phabricator.com/D16121
Summary: Flips the bits from true to false in transaction editor.
Test Plan: update a post, search for new term
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16120
Summary: Ref T9897, makes blogs searchable
Test Plan: Make a blog, index it, search for it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16119
Summary:
Ref T11098. Mixture of issues here:
- Similar problem to D16112, where users with no settings at all could fail to fall back to the global defaults.
- I made `UserPreferencesQuery` responsible for building defaults instead to simplify this, since we have 4 or 5 callsites which need to do it and they aren't easily reducible.
- Handle cases where `metamta.one-mail-per-recipient` is off (and thus users can not have any custom settings) more explicitly.
- When `metamta.one-mail-per-recipient` is off, remove the "Email Format" panel for users only -- administrators can still access it in global preferences.
Test Plan:
- Deleted a user's preferences, changed globals, purged cache, made sure defaults reflected global defaults.
- Changed global mail tags, sent mail to the user, verified it was dropped in accordinace with global settings.
- Changed user's settings to get the mail instead, verified mail was sent.
- Toggled user's Re / Vary settings, verified mail subject lines reflected user settings.
- Disabled `metamta.one-mail-per-recipient`, verified user "Email Format" panel vanished.
- Edited "Email Format" in single-mail-mode in global prefs as an administrator.
- Sent more mail, verified mail respected new global settings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16118
Summary: Ref T11098. There is no reason to maintain these as separate values now that they can be configured in global settings.
Test Plan:
- Hit and read setup issue.
- Fiddled with settings.
- I'll vet this more throughly in the next diff since I need to fix an issue with global defaults in mail and can explicitly test this at the same time.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16117
Summary: Adds PhamePost object to fulltextsearch index. Some issue searching just "Open" though? Also "closed" objects search fine but don't display as disabled.
Test Plan:
bin/search index --type POST
{F1687043}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16116
Summary: Ref T9789. Falling back to `parent::` is better, and fixes older-style feed stories for Pastes, like "added a comment".
Test Plan: Viewed a comment feed story about a paste.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16114
Summary:
The option names for `Vary Subjects` are copypasta from the `Add "Re:" Prefix` option. Fix their names to refer to `Vary Subjects` instead.
Fixes T11148
Test Plan: Verify option names for `Vary Subjects` refer to `Add "Re:" Prefix` before apply. Verify they no longer do after apply.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T11148
Differential Revision: https://secure.phabricator.com/D16113
Summary:
Ref T9789. `Transaction` and `Editor` classes are the last major pieces of infrastructure that haven't been fully modularized.
Some of the specific issues are:
- `Editor` classes rely on a bunch of `instanceof` stuff in the base class to pick up transaction types like "subscribe", "projects", etc. Instead, applications should be adding these, and third-party applications should be able to add them.
- Code is spread across `Transaction` and `Editor` classes somewhat oddly. For example, generating old/new values would probably make more sense at the `Transaction` level, but it currently exists at the `Editor` level.
- Both types of classes have a lot of functions based on `switch()` statements, which require a ton of boilerplate and are just generally kind of hard to work with.
This creates classes for each type of transaction, and moves almost all of the logic to them. These classes are simpler and more focused than the old stuff was, and can organize related code better.
This starts inching toward defining `CoreTransactions` for features shared across applications. It only defines the "Create" transaction so far, but at some point I plan to move all the other shared transactions to Core and let them control which objects they're available for.
Test Plan:
- Created pastes with web UI and API.
- Edited all paste properites.
- Archived/activated.
- Verified files got reasonable names.
- Reviewed timeline and feed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16111
Summary:
Ref T11098. Users with at least one setting set correctly fall back to the defaults, but users with no settings at all currently do not.
Make them fall back to global defaults properly.
Test Plan:
- Set global defaults to some non-default setting.
- Completely delete a user's settings.
- `bin/cache purge --purge-all` or `--purge-user`.
- View settings as the user.
- Before change: showed hard-coded defaults instead of global defaults until you save anything.
- After change: properly shows global defaults from the start.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16112
Summary:
Ref T11035. This only fixes half of the issue: comment editing has been fixed, but normal transactions which edit things like descriptions haven't yet.
The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order.
Test Plan:
- Mentioned `@dog` in a comment.
- Removed `@dog` as a subscriber.
- Edited the comment, adding some unrelated text at the end (e.g., fixing a typo).
- Before change: `@dog` re-added as subscriber.
- After change: no re-add.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11035
Differential Revision: https://secure.phabricator.com/D16108
Summary: Adds a quick edit link to PhamePosts in ApplicationSearch
Test Plan: Review a few searches, click on pencil.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16109
Summary:
When having lots of repos, seeing "all revisions in this project" is hard, and we ended up adding herald rules to basically copy project tags to the revisions on a per-project basis. Adding a "tagged: project" function to the Repositories search field allows users to find differentials within a project.
Fix T10850.
Test Plan: search differentials by tagging project and repository in the Repository field
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: avivey, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10850
Differential Revision: https://secure.phabricator.com/D16096
Summary: Forgot to save this file locally. Adds isArchived to same hidden features as isDraft
Test Plan: test mail on archived posts
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16106
Summary: Ref T9897. Adds ability to Archive a Phame Post (only visible under ApplicationSearch).
Test Plan: Archive a post, re-publish it, search for it, archive it again. View Home, Blog, Live pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16104
Summary: Fixes T11139. We missed this years ago when we moved to PhutilUTF8StringTruncator.
Test Plan: {F1686072}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11139
Differential Revision: https://secure.phabricator.com/D16105
Summary:
Ref T11137. This class is removed in D16099. Depends on D16099.
`PhutilURI` now attempts to "just work" with Git-style URIs, so at least in theory we can just delete all of this code and pretend it does not exist.
(I've left "Display URI" and "Effective URI" as distinct, at least for now, because I think the distinction may be relevant in the future even though it isn't right now, and to keep this diff small, although I may go remove one after I think about this for a bit.)
Test Plan:
- Created a new Git repository with a Git URI.
- Pulled/updated it, which now works correctly and should resolve the original issue in T11137.
- Verified that daemons now align the origin to a Git-style URI with a relative path, which should resolve the original issue in T11004.
- Grepped for `PhutilGitURI`.
- Also grepped in `arcanist/`, but found no matches, so no patch for that.
- Checked display/conduit URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11137
Differential Revision: https://secure.phabricator.com/D16100
Summary:
Ref T7643. When a large block of prose text is edited (like a wiki page), summarize the diff when sending mail.
For now, I'm still showing the whole thing in the web UI, since it's a bit more manageable there.
Also try to fix newlines in Airmail.
Test Plan:
This web diff:
{F1682591}
..became this mail diff:
{F1682592}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16098
Summary:
Ref T8510. Sort prefix matches above non-prefix matches, so that "Ape Discovery" does not match "discovery" better than "Discovery".
Sort functions last.
Rename function internal strings so they don't get over-promoted the prefix-match rules.
Add kind of a hack to get "Project X" sorting above all the "Project X (Milestone 1)" results.
Test Plan:
Created "Ape Discovery", "Baboon Discovery", "Chimpanzee Discovery", etc.
Main project now sorts above milestones:
{F1681773}
Prefix matches now sort above other matches:
{F1681774}
Function results (rarely used) are now less prominent:
{F1681775}
Better function results here:
{F1681776}
More function results:
{F1681777}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8510
Differential Revision: https://secure.phabricator.com/D16094
Summary: Ref T11120. If this works, I'll just remove this option completely.
Test Plan: ¯\_(ツ)_/¯
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11120
Differential Revision: https://secure.phabricator.com/D16095
Summary: "All Tasks" is bad in the long run and not clearly better for new installs.
Test Plan: Created a new smiple template, saw open tasks only.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16093
Summary: Ref T10227. When we perform `git` http operations (fetch, mirror) check if we should use a proxy; if we should, set `http_proxy` or `https_proxy` in the environment to make `git` have `curl` use it.
Test Plan:
- Configured a proxy extension to run stuff through a local instance of Charles.
- Ran `repository pull` and `repository mirror`.
- Saw `git` HTTP requests route through the proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10227
Differential Revision: https://secure.phabricator.com/D16092
Summary: Ref T11123. This implements a very basic skeleton for modern revision search.
Test Plan: Viewed and executed Conduit API method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11123
Differential Revision: https://secure.phabricator.com/D16089
Summary: Ref T6523. Allows you to click stuff instead of using drag-and-drop.
Test Plan: On iOS simulator, created and updated a mock.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6523
Differential Revision: https://secure.phabricator.com/D16088
Summary: Fixes T10886. This should get more formal some day, but just fix it for now.
Test Plan: Reloaded mock with other unpublished draft inlines, saw accurate count.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10886
Differential Revision: https://secure.phabricator.com/D16087
Summary: Fixes T11115, but unclear how to test this. I think I've asked this in the past.
Test Plan:
- Visit Applications -> Ponder
- Configure external email
- Test External Email
- See new Question
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11115
Differential Revision: https://secure.phabricator.com/D16084
Summary: Ref T4280. At some point (probably D15732) we started getting anchor parsing wrong. Just pop the anchor off before doing all the logic, then put it back on at the end.
Test Plan:
Tested various forms like:
```
[[ x ]]
[[ x | z ]]
[[ x#y | z ]]
[[ ./x#y | z ]]
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4280
Differential Revision: https://secure.phabricator.com/D16083
Summary: Fixes T11113. On the 2nd+ page, we could end up with an ambiguous `id` WHERE clause because we don't define a primary table alias on this query. Define one.
Test Plan:
Changed SearchEngine to return pages of size 5, searched for my threads, toggled to second page, no exception.
Used DarkConsole to examine that second-page query, saw that it had `thread.id` explicitly instead of `id` implicitly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11113
Differential Revision: https://secure.phabricator.com/D16080
Summary:
Via HackerOne. This page fatals if accessed directly while logged out.
The "shouldRequireLogin()" check is wrong; this is a logged-in page.
Test Plan:
Viewed the page while logged out, no more fatal.
Faked my way through the actual verification flow.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16077
Summary: Fixes T11107. The URI change here meant we were dropping the "key" parameter, which allows you to set a new password without knowing your old one.
Test Plan: Reset password, didn't need to provide old one anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11107
Differential Revision: https://secure.phabricator.com/D16075
Summary: Ref T6916. Added video to remarkup using D7156 as reference.
Test Plan:
- Viewed video files (MP4, Ogg) in Safari, Chrome, Firefox (some don't work, e.g., OGG in Safari, but nothing we can really do about that).
- Used `alt`.
- Used `autoplay`.
- Used `loop`.
- Used `media=audio`.
- Viewed file detail page.
Reviewers: nateguchi2, chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: asherkin, ivo, joshuaspence, Korvin, epriestley
Tags: #remarkup
Maniphest Tasks: T6916
Differential Revision: https://secure.phabricator.com/D11297
Summary:
Ref T11098. This primarily fixes Conduit calls to `*.edit` methods failing when trying to access user preferences.
(The actual access is a little weird, since it seems like we're building some UI stuff inside a policy query, but that's an issue for another time.)
To fix this, consolidate the "we're about to run some kind of request with this user" code and run it consistently for web, conduit, and SSH sessions.
Additionally, make sure we swap things to the user's translation.
Test Plan:
- Ran `maniphest.edit` via `arc call-conduit`, no more settings exception.
- Set translation to ALL CAPS, got all caps output from `ssh` and Conduit.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16066
Summary:
Ran into this while fixing T11098#179088.
The "Transaction Type" details in the conduit autogenerated documentation for `*.edit` endpoints still wraps incorrectly.
Test Plan: Purged remarkup cache, reloaded page, got full-width text.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16065
Summary:
Ref T7643.
- When a transaction edits a text block, add a link to the changes (for HTML mail).
- Also, inline the changes in the mail (for HTML mail).
- Do nothing for text mail since I don't think we really have room? And I don't know how we can make the diff look any good.
Test Plan:
Edited a task description, generated mail, examined mail.
- It contained a link leading to a prose diff.
- It had a more-or-less reasonable inline text diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16063
Summary:
Ref T10785. Around the time we launched Phacility SAAS we implemented this weird autologin hack. It works fine, so clean it up, get rid of the `instanceof` stuff, and support it for any OAuth2 provider.
(We could conceivably support OAuth1 as well, but no one has expressed an interest in it and I don't think I have any OAuth1 providers configured correctly locally so it would take a little bit to set up and test.)
Test Plan:
- Configured OAuth2 adapters (Facebook) for auto-login.
- Saw no config option on other adapters (LDAP).
- Nuked all options but one, did autologin with Facebook and Phabricator.
- Logged out, got logout screen.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10785
Differential Revision: https://secure.phabricator.com/D16060
Summary:
Ref T10856. The rendering logic was already there, but it was expecting the information under `properties`
field, whereas arc puts it under `metadata`. Not sure if that something that changed a long time ago or if
it was always like this.
Test Plan: {F1252657 size=full}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T10856
Differential Revision: https://secure.phabricator.com/D15828
Summary:
Ref T3353. This hooks the prose engine up to the UI and throws away the hard-wrapping hacks.
These are likely still very rough in many cases, but are hopefully a big step forward from the old version in the vast majority of cases.
Test Plan: {F1677809}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3353
Differential Revision: https://secure.phabricator.com/D16056
Summary:
Fixes T11088. When a task is removed from a project, we don't normally delete its column positions. If you accidentally remove a project and then restore the project, it's nice for the task to stay where you put it.
However, we do need to remove its positions in proxy columns to avoid the issue in T11088.
Test Plan:
- Added a failing unit test, made it pass.
- Added a task to "X > Milestone 1", loaded workboard, used "Edit Projects" to move it to "X" instead, loaded workboard.
- Before, it stayed in the "Milestone 1" column.
- After, it moves to the "Backlog" column.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11088
Differential Revision: https://secure.phabricator.com/D16052
Summary:
Ref T11098.
- Allow "Editor" to be set to the empty string.
- Don't match a validation error to a field unless the actual settings for the field and error match.
Test Plan:
- Tried to set "Editor" to "", success.
- Tried to set "Editor" to "javascript://", only that field got marked "Invalid".
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16051
Summary: Ref T11098. Template preferences don't have a user, but this codepath didn't get fully updated to account for that.
Test Plan: Saved mail tags in global prefernces.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16050
Summary:
Ref T11098. We have a fair number of these, including links in email, which we can't turn into explicit `/user/` URIs.
Just redirect them to the modern places.
Test Plan: Clicked "Customize Menu..." on home page.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16049
Summary:
Ref T4103. This just adds a single global default setting group, not full profiles.
Primarily, I'm not sure how administrators are supposed to set profiles for users, since most ways user accounts get created don't really support setting roles.. When we figure that out, it should be reasonably easy to extend this. There also isn't much of a need for this now, since pretty much everyone just wants to turn off mail.
Test Plan:
- Edited personal settings.
- Edited global settings.
- Edited a bot's settings.
- Tried to edit some other user's settings.
- Saw defaults change appropriately as I edited global and personal settings.
{F1677266}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16048
Summary:
Ref T4103. If the database has `""` (empty string) for select/option settings, we can let that value be effective in the UI right now.
One consequence is that timestamps can vanish from the UI.
Instead, be stricter and discard it as an invalid value.
Test Plan:
- Forced `time-format` setting to `''`.
- Saw timestamps vanish before change.
- Saw timestamps return to the default value after change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16047
Summary:
Ref T4103. Two small improvements:
- Don't work as hard to validate translations. We just need to know if a translation exists, we don't need to count how many strings it has and build the entire menu.
- Allow `getUserSetting()` to work on any setting without doing all the application/visibility checks. It's OK for code to look at, say, your "Conpherence Notifications" setting even if that application is not installed for you.
Test Plan: Used XHProf and saw 404 page drop from ~60ms to ~40ms locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16046
