Summary:
To set this up:
- alice accepts a revision.
- Something adds a package or project she has authority over as a reviewer.
- Because alice has already accepted, she can not re-accept, but she should be able to (in order to accept on behalf of the new project or package).
Test Plan:
- Created a revision.
- Accepted as user "dog".
- Added "dog project".
- Re-accepted.
- Could not three-accept.
- Removed "dog project.
- Rejected.
- Added "dog project".
- Re-rejected.
- Could not three-reject.
Reviewers: chad, eadler
Reviewed By: chad, eadler
Differential Revision: https://secure.phabricator.com/D17226
Summary: Ref T10978. Handle loads can be batched a bit more efficiently by doing them upfront.
Test Plan: Queries dropped a bit locally, but I mostly have the same autors/auditors. I'm seeing 286 queries on my account in production, so I'll check what happens with that.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17225
Summary:
See D17222. D17209 accidentally broke setting IDs on ActionListView by converting it into a TagView: TagView already has an `id` property, and this new `id` property on the subclass shadows it.
Materially, the "Actions" mobile button in the headers of objects (for example: Maniphest Task -> shrink browser window -> click "Actions" next to task name) relies on setting IDs on list views.
Test Plan:
- Viewed a task.
- Made browser window narrow.
- Clicked `[= Actions]` button.
- After patch: saw a dropdown menu.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17223
Summary: Builds out more UI to reinforce just who you are in this world... A perfect person.
Test Plan:
Look at myself a lot.
{F2435202}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17224
Summary: Fixes T5889. You can't write a rule like "if no other Herald rules did anything...", but you can use this rule to check for Owners or an explicit "Auditors" field doing things.
Test Plan: Using the test console, ran an "Auditors" rule against a commit with and without an auditor. Got expected pass/fail outcomes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5889
Differential Revision: https://secure.phabricator.com/D17221
Summary: Ref T5867. The `executeOne()` currently raises a policy exception if the application isn't visible to the viewer, or we fatal if the application has been uninstalled.
Test Plan:
- Viewed pages with the application uninstalled, saw working pages with no favorites menu.
- Viewed pages with the application restricted, saw working pages with no favorites menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17219
Summary: Fixes T12117. I typed or copy/pasted this constant wrong while refactoring during T10978.
Test Plan: Called `audit.query`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12117
Differential Revision: https://secure.phabricator.com/D17218
Summary:
- Attach objects when showing configuration screen
- Fix "Forms" to make more sense
- Alter EditEngine title to load correct name by loading object
Fixes T12116
Test Plan: Load up Apps/Projects/Forms on a configure menu, see proper names
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12116
Differential Revision: https://secure.phabricator.com/D17217
Summary: Ref T5867. Instead of hard-coding projects, tasks and repositories, let EditEngines say "I want a quick create item" so third-party code can also hook into the menu without upstream changes.
Test Plan: Saw same default items in menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17215
Summary: Ref T5867. I sure love Javascript.
Test Plan: Navigated between Home, Diffusion and Differential, opening the user profile menu. Saw appropraite help items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17214
Summary: Fixes the persistent chat hyperlinks showing.
Test Plan: Open persistent chat, no longer see the hyperlinks showing. Close and open chat. Edit room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17216
Summary: Ref T5867. Use a single query to load both personal and global items, then reorder them and add a divider if both groups have some stuff.
Test Plan: Viewed menu, edited personal and global items, viewed/edited existing project menus.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17213
Summary:
Still lots to fix here, punting up since I'm running into a few roadblocks.
TODO:
[] Sort Personal/Global correctly
[] Quicksand in Help Items correctly on page changes
Test Plan: Verify new menus work on desktop, tablet, mobile. Test logged in menus, logged out menus. Logging out via a menu, verify each link works as expected. Help menus get build when using an app like Maniphest, Differential. Check that search works, preferences still save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12107
Differential Revision: https://secure.phabricator.com/D17209
Summary:
Fixes T12114. There were a couple of bugs here:
- We could draw too many joining lines if a node had a parent with multiple descendants.
- We could incorrectly ignore columns because of an `unset()`.
I //think// this fixes both things without collateral damage. This whole thing is a little hard to understand/debug and has grown beyond its original scope, so I'll probably rewrite it if there are more issues.
Test Plan:
- Unit tests.
- My local repro is clean now:
{F2424920}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12114
Differential Revision: https://secure.phabricator.com/D17211
Summary:
In D16157, dropdown menus got an overly-broad check for not closing when an item is clicked.
Specifically, we don't want to close the menu if the item is really opening a submenu, like "Edit Related Objects..." does on mobile.
The check for this is too broad, and also doesn't close the menu if the item has workflow.
Instead, use a narrower check.
Test Plan:
- Menu still stays open when toggling submenus like "Edit Related Objects".
- Menu now closes properly when using workflow items like "Edit Comment" or "Remove Comment".
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17210
Summary: Fixes T6660. Uses the new stuff in Audit to build an EditEngine-aware icon.
Test Plan: {F2364304}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6660
Differential Revision: https://secure.phabricator.com/D17208
Summary:
Ref T11114. Ref T12085. I missed a few pieces of cleanup when moving all this stuff over.
In particular, load all fields which use Custom Field storage before doing commit-message-related stuff, instead of just the ones that claim they appear on commit messages.
Test Plan: Edited revisions and made API calls without apparent issues. See followup on T12085, shortly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12085, T11114
Differential Revision: https://secure.phabricator.com/D17207
Summary: Never really used this to full potential and takes up a lot of code and space. Remove option for now and make all profile nav menus small by default.
Test Plan: Review user, project, workboard. Set new menus.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17206
Summary:
Fixes T12095. Ref T6660. The old code for this was specific to Differential, using the `DifferentialDraft` table.
Instead, make the `EditEngine` / `VersionedDraft` code create and remove a `<objectPHID, authorPHID>` edge when a particular author creates drafts.
Some applications have drafts beyond `VersionedDrafts`, notably inline comments. Before writing "yes, draft" or "no, no draft", ask the object if it has any custom draft stuff we need to know about.
This should fix all the yellow bubble bugs I created in T11114 and allow us to bring the feature to Audit fairly easily.
Test Plan: Created and deleted comments and inlines, reloading the list view after each change. Couldn't find a way to break the list view anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12095, T6660
Differential Revision: https://secure.phabricator.com/D17205
Summary:
Ref T12098.
We have two methods (`supportsEditEngineConfiguration()` and `isEngineConfigurable()`) which sort of do the same thing and probably should be merged.
For now, just swap which one we override to get "Create Revision" out of the Quick Create menu.
Test Plan: No more "Create Revision" in Quick Create menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12098
Differential Revision: https://secure.phabricator.com/D17204
Summary:
Ref T9482. These may need a little more work (feel free to shoot me a counter-diff) but try to:
- Never use only color to distinguish between states (for colorblind, etc users).
- Give the "nothing needs to be done" state a more obvious "okay" icon (instead of a question mark).
Test Plan: Looked at some linked commits in Maniphest, the icons made a bit more sense?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9482
Differential Revision: https://secure.phabricator.com/D17203
Summary:
Ref T9640. This is no longer true.
I'm assuming PHP7 vs 7.1 issues won't be common (everyone on the cutting edge is probably on 7.1, and Ubuntu 16 is on 7.1) but we could make this more granular in the future.
Test Plan: Careful reading.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9640
Differential Revision: https://secure.phabricator.com/D17202
Summary: Ref T9640. APCu 5.0+ (for PHP7) uses `apcu_*` functions instead of `apc_` functions. Test for function existence and call the appropriate functions.
Test Plan: {F2352695}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9640
Differential Revision: https://secure.phabricator.com/D17198
Summary: Ref T9640. On 7.0 we had signal handling issues so we can never support it, but async signals should resolve them on 7.1 or newer.
Test Plan: On PHP 7.1, got through the setup warning.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9640
Differential Revision: https://secure.phabricator.com/D17197
Summary: Ref T9640. This option was removed in PHP7, so there's no reason to warn about it.
Test Plan: No longer saw a setup warning on PHP7.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9640
Differential Revision: https://secure.phabricator.com/D17196
Summary:
Fixes T9482. Historically, Audit was somewhat confused about whether queries and views should act on the viewer's status or the object's status.
This realigns Audit to work like Differential: we show overall status for the commit, just like we show overall status for revisions. This better aligns with expectation and isn't weird/confusing, and bucketing should handle all the "what do //I// need to do" stuff now (or, at least, seems to have in Differential).
This is also how every other type of object works in every other application, AFAIK (all of them show object status, not viewer's-relationship-to-the-object status).
Test Plan:
- Viewed commit lists in Owners and Audit.
- Saw commit overall statuses, not my personal status.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9482
Differential Revision: https://secure.phabricator.com/D17195
Summary: Ref T10978. This is bare bones, but the SearchEngine is at least mostly in reasonable shape now, so get it in place and freeze the old stuff. I previously froze `audit.query`, which did much the same thing.
Test Plan: Issued some queries with the API, technically got results back.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17194
Summary:
Fixes T7076. This could probably use some tweaking but should get the basics in place.
This shows overall object state (e.g., "Needs Review"), not individual viewer state (e.g., "you need to review this"). After the bucketing changes it seems like we're mostly in a reasonable place on showing global state instead of viewer state. This makes the overall change much easier than it might otherwise have been.
Test Plan: {F2351867}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7076
Differential Revision: https://secure.phabricator.com/D17193
Summary:
Fixes T9430. Fixes T9362. Fixes T9544. This changes the default view of Audit to work like Differential, where commits you need to audit or respond to are shown in buckets.
This is a bit messy and probably needs some followups. This stuff has changed from a compatibility viewpoint:
- The query works differently now (but in a better, modern way), so existing saved queries will need to be updated.
- I've removed the counters from the home page instead of updating them, since they're going to get wiped out by ProfileMenu soon anyway.
- When bucketed queries return too many results (more than 1,000) we now show a warning about it. This isn't greaaaat but it seems good enough for now.
Test Plan: {F2351123}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9430, T9362, T9544
Differential Revision: https://secure.phabricator.com/D17192
Summary: Ref T11114. Ref T10978. These hadn't made it over to EditEngine yet.
Test Plan:
- Took various actions on revisions and commits.
- Used `bin/mail show-outbound --id ...` to examine the "Vary Subject", saw it properly generate "[Accepted]", "[Resigned]", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114, T10978
Differential Revision: https://secure.phabricator.com/D17191
Summary: Ref T10978. Until T10448 makes mail tags modular, keep the old tags working.
Test Plan: Made some commit edits, ran `bin/phd debug task` to process mail for them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17190
Summary: Ref T10978. Since "Resigned" is a status in Audit, you could repeatedly resign. This is confusing; prevent it.
Test Plan: Tried to resign twice; was only allowed to resign once.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17187
Summary:
Fixes T6630. Long ago, "Audit", "Diffusion" and "Repositories" were three totally separate applications.
This separation isn't useful and the three rapidly became intertwined. Ideally, they would all be one application.
This doesn't take us quite that far, but Audit no longer has any controllers and has little actual behavior.
The "Audit" screen has always just been a SearchEngine view of commits with some filters on it, and this formalizes that and puts a link to it in Diffusion. (This view has other uses, too.)
Test Plan:
- Accessed audit from home page.
- Accessed audit/commits from Diffusion.
- Could no longer uninstall Audit on its own.
- Grepped for `/audit/` and `AuditApplication`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6630
Differential Revision: https://secure.phabricator.com/D17186
Summary: Fixes T7504. I think that task legitimately describes a bug and that the current behavior is counterintuitive.
Test Plan: Manually added an auditor to a commit with none; saw it become "Audit Required" as an overall state.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7504
Differential Revision: https://secure.phabricator.com/D17185
Summary:
Fixes T8739. Currently, Diffusion inline comments in the timeline are sorted arbitrarily, mostly by creation order.
Instead, sort them by line number, like Differential.
Test Plan:
Made comments in "C", "B", "A" order, saw them in line order after submit:
{F2343032}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8739
Differential Revision: https://secure.phabricator.com/D17184
Summary:
Ref T10978. This prepares for swapping the comment UI to stacked actions.
These are only accessible via the API.
Test Plan: Used the API to accept, raise concern with, and reject commits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17182
Summary: Ref T10978. Ref T7676. Make auditors work more like reviewers, so they can be freely added or removed.
Test Plan:
- Interacted with auditors via "Edit Commit" and API.
- Comment area is still oldschool and doesn't work yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978, T7676
Differential Revision: https://secure.phabricator.com/D17181
Summary: Ref T11957, builds out `/home/menu/` as a basic structure for adding/editing the homepage menu.
Test Plan: visit `/home/menu/` and add items to global and personal. Not wired to anything.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17180
Summary: Ref T10978. This currently does almost nothing, but gets it in place so I can add stuff to it.
Test Plan: Made a comment on a commit using the API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17178
Summary: Ref T10978. The new controller now does everything the old one did, so swap 'em and nuke the old one.
Test Plan: Edited a commit, hit the new controller, things worked real good.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17177
Summary: Ref T10978. The current "Edit" flow has some autoclose info. This isn't necessarily the best place to put it in the long run, but preseve it for now since the documentation refers to it.
Test Plan: {F2340658}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17176
Summary:
Fixes T12097. In D16413, I simplified this code but caused us to load the //commit's// projects instead of the //repository's// projects, which is incorrect.
Normally, commits don't have any project tags when Herald evaluates, so using the commit's projects is generally meaningless.
Test Plan:
- Tagged a repository with `#X`.
- Created a Herald object rule for commits with `#X` as the object ("Always ... do nothing.")
- Ran a commit from the repository.
- Before patch: rule failed to evaluate.
- After patch: rule evaluated and passed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12097
Differential Revision: https://secure.phabricator.com/D17179
Summary:
Ref T10978. After T11114, we have some features (like the old code for the haunted comment panel) which are only used by Diffusion. I want to modernize it so I can nuke them. T10978 also describes many bugs which are only fixable after modernizing.
This adds very basic EditEngine support for commits/audit. You can't create new commits with this workflow, just tag/update existing ones.
Test Plan: {F2340347}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17175
Summary: Add in some basic defaults, Tasks, Projects, Repositories... anything else? Also switches "manage" context if you are an admin or user. Hides link if you are not logged in.
Test Plan: Review Global/Personal in Favorites app, click on each link.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17174
Summary: Moves basic colors into the processor.
Test Plan: Review a diff in sandbox with and without change.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17173
Summary:
Fixes T12092. D17164 made `DiffQuery` more strict about arguments using modern conventions, but `differential.querydiffs` uses bizarre ancient conventions.
Give it more modern conventions instead.
Test Plan: Made a `querydiffs` call with only revision IDs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12092
Differential Revision: https://secure.phabricator.com/D17172
Summary:
Fixes T12087. When transitioning into a clustered configuration for the first time, the documentation recommends using a one-device cluster as a transitional step.
However, installs may not do this for whatever reason, and we aren't as clear as we could be in warning about clusterizing directly into a multi-device cluster.
Roughly, when you do this, we end up believing that working copies exist on several different devices, but have no information about which copy or copies are up to date. //Usually// they all were already synchronized and are all up to date, but we can't make this assumption safely without risking data.
Instead, we err on the side of caution, and require a human to tell us which copy we should consider to be up-to-date, using `bin/repository thaw --promote`.
Test Plan:
```
$ ./bin/repository clusterize rLOCKS --service repos001.phacility.net
Service "repos001.phacility.net" is actively bound to more than one device
(local002.local, local001.phacility.net).
If you clusterize a repository onto this service it will be unclear which
devices have up-to-date copies of the repository. This leader/follower
ambiguity will freeze the repository. You may need to manually promote a
device to unfreeze it. See "Ambiguous Leaders" in the documentation for
discussion.
Continue anyway? [y/N]
```
Read other changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12087
Differential Revision: https://secure.phabricator.com/D17169
Summary: Ref T5867. Rough in a Favorites application, not wired to anything.
Test Plan: tbd. currently 404s so... I messed up something. Tossing up to read.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17160
Summary: Fixes T12086. This got dropped by accident while cleaning up haunting.
Test Plan: Loaed a revision, hit "?", hit n/j/p/etc
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12086
Differential Revision: https://secure.phabricator.com/D17166
Summary:
See D17160. Previously, the `/edit/` route was never linked, but fataled when accessed. Make it 404 instead.
Also, fix an issue where editing "Application" menu items would fail because they didn't have a viewer.
Test Plan:
- Hit `/edit/`, got a 404.
- Edited an "Application" item.
- Moved, added, deleted, and edited other items.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17165
Summary:
Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail.
When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too.
Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it.
Test Plan:
- Used `bin/differential attach-commit <commit> <revision>` to hit this code.
- Simulated a filesystem write failure, saw the diff get reused.
- Also did a normal update, which worked properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10968
Differential Revision: https://secure.phabricator.com/D17164
Summary:
Ref T12074. This uses more consistent Conduit keys for constraint names.
This is a minor compatibility break on watchers/members but since these methods are more useful now this is probably a good time to try to get away with it, and a more consistent API is better in the long run. I need to issue compatibility guidance for the milestones thing anyway and that one isn't avoidable, so try to rip the bandage off all in one go.
Test Plan: Reviewed new constraint names from console, called methods using them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17161
Summary:
Fixes T12082. Ref T11114. When modular transaction render a handle list, they use HandleListView, which has a text mode.
However, the HandleListView is a TagView, and currently TagViews always render a tag of some kind. Allow them to return `null` to decline to render any tag.
Test Plan:
- Added a pile of debugging stuff to `ApplicationTransactionEditor` to throw during mail generation.
- Added a reviewer to a revision.
- Used `bin/worker execute --id ...` to hit the mail generation repeatedly.
- Before patch: mail generated with a <span>, even in text mode.
- After patch: clean mail generation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12082, T11114
Differential Revision: https://secure.phabricator.com/D17162
Summary: Ref T11114. After evaluating typeahead tokens, we could process blocking reviewer removals incorrectly: we may get structures back.
Test Plan: Removed blocking reviewers from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17163
Summary:
Ref T12074. The "v3" API methods (`*.search`, `*.edit`) are currently marked as "unstable", but they're pretty stable and essentially all new code should be using them.
Although these methods are seeing some changes, almost all changes are additive (support for new constraints or attachemnts) and do not break backward compatibility. We have no major, compatibility-breaking changes planned.
I don't want to mark the older methods "deprecated" yet since `arc` still uses a lot of them and there are some capabilities not yet available on the v3 methods, but introduce a new "frozen" status with pointers to the new methods.
Overall, this should gently push users toward the newer methods.
Test Plan: {F2325323}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17158
Summary:
Ref T12074. Provide a basic but functional v3 API endpoint for reading workboard column information.
There is no equivalent to this in the UI yet, although there may be some day (perhaps adjacent to T5024).
Test Plan:
- Queried for all columns.
- Queried for columns on a particular board using `projectPHIDs`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17157
Summary:
Ref T12074. This allows callers to identify which columns an object appears in (currently, always tasks).
There are a few major cases:
- Object is in a normal column: we return column information.
- Object is in a proxy column (subproject or milestone). For example, when you look at the board for "Some Parent Project", the task might show up in a milestone column. I've chosen to not return anything in this case: you can figure out that the task is there by looking at the project structure, and this is kind of an internal artifact of the implementation and probably not useful to callers.
- Project does not have a workboard: we return nothing.
These seem fairly reasonable, I think?
Test Plan:
- Queried for tasks, using the "columns" attachment.
- Dragged a task across a board, querying it repeatedly. Got expected results for normal column (the column), subprojects with no board (nothing), milestones with no board (nothing) and mielstones/subprojects with a board (the column on //that// board, only, not the proxy column on the parent).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17156
Summary:
Ref T12074. Allows querying for project by direct parent (find only immediate children) or any ancestor (find all descendants) using the API.
There's no proper web UI for this since I'm not sure how useful it is, but you can `/project/?parent=PHID-PROJ-...` or `/project/?ancestor=...` for now. We can add UI later if/when use cases arise, but it's not immediately clear to me that this is useful to do from the web.
Test Plan:
- From API, queried with `parentPHIDs` and `ancestorPHIDs`, finding direct children only and all descendants, respectively.
- From web UI, fiddled with `?parent=...` and `?ancestor=...` to make sure they work too. This isn't intended to be a user-facing feature.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17155
Summary:
Ref T12074.
- Adds a new "parent" property on main results. This shows an abbreviated version of the project's parent, or `null` if the project is a root project.
- Adds a new "ancestor" attachment to pull the entire ancestor list.
- Adds a new "depth" property on main results.
- You can use "parent" or "depth" to tell if a project is a subproject or not.
These attempt to balance convenience, power, and performance: the full ancestor list can be big so I made it an attachment, but the other stuff isn't too big and is cheap and seems reasonable to always include.
Test Plan:
In API results:
- Saw null parent (root projects) and non-null parent (subprojects/milestones).
- Used "ancestors" attchment, got full list of ancestors.
- Saw appropriate "depth" values.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17154
Summary:
Ref T12074.
- `project.search` now returns milestones by default.
- A new constraint, `isMilestone`, allows filtering to milestones, non-milestones, or both (API and web UI).
- `project.search` now returns a milestone number for milestones, or `null` for non-milestones.
NOTE: Existing custom saved queries in projects which previously did not return milestones now will. I expect this to have little-to-no impact on users, and these queries are easy to correct, but I'll note this in changelogs.
Test Plan:
- Ran various queries with `project.search` and in the web UI, searching for milestones, non-milestones, and both.
- Web UI default behavior (no milestones) is unchanged, but you can now get milestones if you want them.
- Queried a milestone by ID/PHID via API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12074
Differential Revision: https://secure.phabricator.com/D17153
Summary:
Ref T9058. The stricter filtering is over-filtering Handles. For example, in the Phacility cluster, users can not see Almanac services.
So this filtering happens:
- The AlmanacServiceQuery filters the service beacuse they can't see the application.
- The HandleQuery generates a "you can't see this" handle.
- But then the HandleQuery filters that handle! It has a "service" PHID and the user can't see Almanac.
This violates the assumption that all application code makes about handles: it's OK to query handles for objects you can't see, and you'll get something back.
Instead, don't do application filtering on handles.
Test Plan:
- Added a failing test and made it pass.
- As a user who can not see Almanac, viewed an Instances timeline.
- Before patch: fatal on trying to load a handle for a Service.
- After patch: smooth sailing.
Reviewers: chad
Maniphest Tasks: T9058
Differential Revision: https://secure.phabricator.com/D17152
Summary: Fixes T12080. This was missing a "/", but stop hard-coding these URIs.
Test Plan: Clicked both links with Quickling as a logged-in and logged-out user, ended up in the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12080
Differential Revision: https://secure.phabricator.com/D17151
Summary: Ref T5867, adds a customPHID field, nullable, and lets you query by it... i think? Not fully able to grok all the EditEngine stuff, but I think this is the right place for the query.
Test Plan: Not wired to anything, but pulling up project menu, editing, all still works.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17149
Summary: Ref T11816. The underlying format of recurrence end dates swapped around a bit and we now try to compare `null` to a valid date if you're setting it for the first time.
Test Plan:
- On a new event, set a recurrence end date.
- Then, removed a recurrence end date.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17150
Summary:
Ref T11114. When you comment, we try to upgrade your review status to "commented".
This can conflict with upgrading it to "accepted" or "rejected", or removing it entirely.
For now, just avoid making this update. After T10967, I expect "you commented" to be orthogonal to accepted/rejected so it should stop conflicting on its own.
Test Plan:
- As an "added" reviewer, accepted a revision with a comment in the same transaction.
- Before patch: accept didn't stick.
- After patch: accept sticks.
This may be somewhat magical/order-dependent but I was able to reproduce it locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17146
Summary: Fixes T12068. These are inbound messages, not outbound.
Test Plan: Read carefully.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12068
Differential Revision: https://secure.phabricator.com/D17144
Summary: Minor color saturation here, ideal for low quality monitors.
Test Plan:
Review new colors in various scenarios.
{F2305178}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17141
Summary:
Fixes T10136. This reinforces ongoing or failed builds in the comment action area.
We already emit a similar message for unit test failures from `arc unit`. This should probably obsolete that, eventually.
Test Plan:
{F2304809}
{F2304810}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10136
Differential Revision: https://secure.phabricator.com/D17140
Summary: When a user queries by package monogram explicitly, search by package ID.
Test Plan: {F2305075}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17142
Summary:
Fixes T9276. Fixes T8650. The story so far:
- We once published build updates to Revisions.
- An unrelated fix (D10911) sent them to the Diffs instead of Revisions, which isn't useful, since you can't see a diff's timeline anywhere.
- This also caused a race condition, where the RevisionEditor and DiffEditor would update the diff simultaneously (T8650).
- The diff update was just disabled to avoid the race (part of D13441).
- Instead, allow the updates to go somewhere else. In this case, we send commit updates to the commit but send diff updates to the revision so you can see 'em.
- Since everything will be using the revision editor now, we should either get proper lock behavior for free or it should be easy to add if something whack is still happening.
- Overall, this should pretty much put us back in working order like we were before D10911.
This behavior is undoubtedly refinable, but this should let us move forward.
Test Plan:
Saw a build failure in timeline:
{F2304575}
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T9276, T8650
Differential Revision: https://secure.phabricator.com/D17139
Summary: Adds a FormEditEngine MenuItem for adding forms to Projects, Home, QuickCreate. Also adds an EditEngine typeahead that has token rendering issues currently.
Test Plan: Set a normal form as a menu item, edit it, set the name. Set a custom form as a menu item, edit it, set a name.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17098
Summary: Tweaks the diff colors here a bit, as well as making full diffs slightly easier to read in full. Ref T12060
Test Plan:
Tested prose diffs, email prose diffs, and a regular Differential revision.
{F2304056}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12060
Differential Revision: https://secure.phabricator.com/D17138
Summary:
Fixes T12062. Like the commits from the year 3500, you can artificially build commits with no date information.
We could explicitly store these as `null` to fully respect the underlying datastore. However, I think it's very unlikely that these commits are intentional/meaningful or that this is valuable.
Additionally, "git show" interprets these commits as "Jan 1, 1970". Just store a `0` to mimic its behavior.
Test Plan:
- Following the process in T11537#192019, artificially created a commit with //no// date information (I deleted all date information from the message).
- Used `git show` / `git log --format ...` to inspect it: "Jan 1, 1970" on `git show`, no information at all on `%aD`, `%aT`, etc.
- Pushed it.
- Saw exception for trying to insert empty string into epoch colum from `bin/repository update`.
- Applied patch.
- Got a clean import.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12062
Differential Revision: https://secure.phabricator.com/D17136
Summary:
Fixes T12058. When the user visits `/maniphest`, for example, we redirect to `/maniphest/`.
Since this redirect is very low-level (at the Aphront level, below the Site level) we need to preserve the request Host rather than correct it to `PhabricatorEnv::getURI()` or similar -- the request may be hiting a different Site like a blog domain.
Currently, we do not preserve the port. Instead, preserve the port if it is not a standard port for the protocol (80 for http, 443 for https).
Test Plan:
- Made a request with a missing slash and a normal port in my browser, got redirected normally.
- Made a request with a missing slash and a nonstandard port, got redirected on the same port.
```
$ curl -H 'Host: local.phacility.com:123' -v http://local.phacility.com/diffusion
* Trying 127.0.0.1...
* Connected to local.phacility.com (127.0.0.1) port 80 (#0)
> GET /diffusion HTTP/1.1
...
>
< HTTP/1.1 302 Found
...
< Location: http://local.phacility.com:123/diffusion/
...
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12058
Differential Revision: https://secure.phabricator.com/D17134
Summary: Ref T3612. Doesn't render correctly, need help please. Adds a download icon into the renderfilelinkview to allow easier downloads.
Test Plan: Click on link, get download, click on file, get lightbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16980
Summary: Ref T11114. Move email/command actions, like "!reject", to modular transactions + editengine.
Test Plan: Used `bin/mail receive-test` to pipe "!stuff" to an object, saw appropraite effects in web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17133
Summary:
Ref T11114. When a user selects "Accept", and then selects "Reject", remove the "Accept". It does not make sense to both accept and reject a revision.
For now, every one of the "actions" conflicts: accept, reject, resign, claim, close, commandeer, etc, etc. I couldn't come up with any combinations that it seems like users are reasonably likely to want to try, and we haven't received combo-action requests in the past that I can recall.
Test Plan:
- Selected "Accept", then selected "Reject". One replaced the other.
- Selected "Accept", then selected "Change Subscribers". Both co-existed happily.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17132
Summary:
Ref T11114. Recent changes broke the links to jump to inline comments from the previews because they get hooked up with JS.
Restore the linking behavior.
Test Plan: Clicked "View" on an inline comment preview, jumped to that comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17131
Summary:
Fixes T9058. Normally, "Query" classes apply an application check and just don't load anything if it fails.
However, in some cases (like email recipient filtering) we run policy checks without having run a Query check first. In that case, one user (the actor) loads the object, then we filter it against other users (the recipeints).
Explicitly apply the application check during normal filtering.
Test Plan: Added a failing test case and made it pass.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9058
Differential Revision: https://secure.phabricator.com/D17127
Summary:
Fixes T11344. In the web UI, if a field like "Subscribers" on an object (like a task) contains values you don't have permission to see, you see tokens for them (like "Restricted Project") but not their names.
Make commit messages work the same way: you see the PHID, and can remove it or leave it there, but can't see the underlying name.
(We have to render an actual PHID rather than just "Restricted Thing" because we have to be able to figure out what edit the user is actually trying to make.)
Test Plan: Interacted with a revision via the CLI that had project reviewers I couldn't see.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11344
Differential Revision: https://secure.phabricator.com/D17124
Summary:
Fixes T9648. Diffs currently use `return $this->getRevision()->getViewPolicy();` to inherit their revision's view policy.
After the introduction of object policies, this is wrong for policies like "Subscribers", because it means "Subscribers to this object, the diff". Since Diffs have no subscribers, this always fails.
Instead, use extended policies so that the object policy evaluates in the context of the correct object (the revision).
Test Plan:
- Create a revision.
- Subscribe `alice` to it.
- Set view policy to "Subscribers".
- View revision as `alice`.
- Before patch: nonsense fatal about missing diff because of policy error.
- After patch: `alice` can see the revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9648
Differential Revision: https://secure.phabricator.com/D17123
Summary: Fixes T10312. If your first line is "Reviewers: xyz", it's a title, not a "Reviewers" field.
Test Plan: Added unit test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10312
Differential Revision: https://secure.phabricator.com/D17122
Summary:
Fixes T8360. We will now parse revisions out of "Differential Revision: X" followed by other ad-hoc fields which we do not recognize. Previously, these fields would be treated as part of the value.
(In the general case, other fields may line wrap so we can't assume that fields are only one line long. However, we can make that assumption safely for this field.)
Also maybe fix whatever was going on in T9965 although that didn't really have a reproduction case.
Test Plan: Added unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8360
Differential Revision: https://secure.phabricator.com/D17121
Summary: Ref T11114. Fixes T10323.
Test Plan:
- Marked comments as done only: no warning about not leaving a comment.
- Did nothing: warning about posting an empty comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114, T10323
Differential Revision: https://secure.phabricator.com/D17120
Summary: Ref T11114. Although I plan to rewrite this system eventually (T10448) it's easy enough to punt for now.
Test Plan: punt
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17119
Summary:
Ref T11114. This restores:
- Commandeering should exeucte Herald.
- Commandeering should swap reviewers.
- "Request Review" on an "Accepted" revision should downgrade reviewers so they have to accept again.
Test Plan:
- Commandeered, saw Herald run and reviewers swap.
- Requested review of an accepted revision, saw it drop down to "Needs Review" with "Accepted Prior" on the reviewer.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17118
Summary: Ref T11114. This restores warnings (e.g., failing unit tests) and fixes "Quote" behavior for comments.
Test Plan:
- Quoted a comment.
- Viewed a warning.
{F2283275}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17117
Summary: Ref T11114. This comments nearly working on EditEngine. Only significant issue I caught is that the "View" link doesn't render properly because it depends on JS which is tricky to hook up. I'll clean that up in a future diff.
Test Plan: {F2279201}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17116
Summary:
Ref T11114. See D17114 for some discussion.
For review actions: accept, reject, resign.
For revision actions, order is basically least-severe to most-severe action pairs: plan changes, request review, close, reopen, abandon, reclaim, commandeer.
Test Plan: Viewed revisions as an author and a reviewer, saw sensible action order within action groups.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17115
Summary:
Ref T11114. Differential has more actions than it once did, and may have further actions in the future.
Make this dropdown a little easier to parse by grouping similar types of actions, like "Accept" and "Reject".
(The action order still needs to be tweaked a bit.)
Test Plan: {F2274526}
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17114
Summary:
Ref T11114. Some rough edges, but this largely makes Accept, Reject and Resign work in the new EditEngine comment area.
Ref T11050. This lays a little bit of groundwork for having "resign" mean "I don't want to review this, even if projects or packages I'm a member of need to", not just "remove me personally as a user reviewer".
Test Plan: Accepted, rejected and resigned from revisions without any major state issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114, T11050
Differential Revision: https://secure.phabricator.com/D17113
Summary:
Ref T11114. This has two pieces of side-effect logic which I've noted locally:
- Commandeer needs to apply Herald rules.
- Commandeer needs to move the old author to become a reviewer and remove
the actor as a reviewer.
Test Plan: Commandeered some revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17111
Summary:
Ref T11114. This restores these actions.
One behavior is incomplete: "Request Review" on an accepted revision does not downgrade reviewers properly. I've noted this locally.
Test Plan: Planned changes and requested review of a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17109
Summary:
Ref T11114. This restores these actions as selectable in the comment area.
This does not implement one special rule ("Closing a revision in response to a commit is OK from any status.") but I have a note about that separately.
Test Plan: Closed and reopened revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17108
Summary: Ref T11114. This begins restoring comment actions to Differential, but on top of EditEngine.
Test Plan: {F2263148}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17107
Summary:
Ref T11114. This is a transitional change that breaks a bunch of stuff. I'll hold it until I've restored features.
This stuff works:
- Commenting.
- Subscribers/tags/reviewers.
- Pinning.
- Drafts.
This stuff does not work yet:
- Preview of inline comments.
- Probably submitting inlines, whatsoever.
- Comment-area warnings like "There are failing tests."
- All meaningful actions (accept, reject, etc).
Test Plan: Commented on a revision. Essentially nothing else works yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17106
Summary:
Fixes T12049. This expands "Haunted" comment panels to EditEngine, and by extension to all EditEngine applications.
Eventual goal is to remove custom commenting code in Differential and replace it with EditEngine code.
Changes from current "haunt" mode:
- This only has one mode ("pinned"), not two ("pinned", "pinned with preview"). There's an inline preview and scroll behavior is a little better.
- Now has a UI action button.
Slightly tricky stuff:
- This interacts with "Fullscreen" mode since it doesn't make sense to pin a full-screen comment area.
- This should only be available for comments, not for remarkup fields like "Description" in "Edit Task".
Test Plan:
- Pinned/unpinned in Maniphest.
- Pinned/fullscreened/unfullscreened/unpinned.
- Checked that "Edit Task" doesn't allow pinning for "Description", etc.
- Pressed "?", read about pressing "Z".
- Pressed "Z".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12049
Differential Revision: https://secure.phabricator.com/D17105
Summary: See D17058.
Test Plan: Ran `arc diff`, which parsed fields as a side effect.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17112
Summary:
Fixes T12040. In T12039, a user running local patches followed the report instructions as far as grabbing version information, but didn't update or revert their local changes or try against a clean install before reporting.
This obviously isn't ideal for us, but it's understandable (grabbing version information is much easier than upgrading/reverting), and we can do better about making this information useful: when compiling version information, try to figure out the branchpoint from a known upstream `master` branch by listing remotes, then running `git merge-base` against them.
Additionally, explicitly document that we want upstream hashes. We have to have a fallback case in this document anyway (for when you can't get to Config) so hopefully this makes it more likely that we get useful information in initial reports.
Test Plan: {F2229574}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12040
Differential Revision: https://secure.phabricator.com/D17103
Summary:
Fetches cause output in `/var/tmp/phd/log/daemons.log` as
follows:
```
PHLOG: 'Unexpected output while updating repository "rREPONAME": No entry for terminal type "unknown";
using dumb terminal settings.
' at [/path/to/phabricator/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php:455]
```
These warnings come from PHP itself. Silence these warnings by providing a
known value for `TERM` before shelling out to the PHP script.
See also D9744 (reverted in D11644) and T4990/T7119, which are a similar issue,
but in the pre-receive hooks, not the pull daemons.
Test Plan:
Enabled in production, observed errors to be silenced and
no SSH hangs
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17100
Summary:
Fixes T12035. Normally, the "abc" -> "abc/" redirect is handled automatically when "abc" hits a 404.
However, in this case, "source/x" does not 404. We route this to a valid controller because some VCS requests omit the slashes, then manually perform the redirect if we aren't serving a VCS request.
Allow this controller to serve public resources so we can serve the redirect to logged-out users instead of prompting them to login so they can be redirected.
Test Plan: Visited `/source/x` as a logged-out user, where `x` is a public repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12035
Differential Revision: https://secure.phabricator.com/D17097
Summary: Built similar to Projects, allows setting of a Dashboard to MenuItem.
Test Plan: Add a dashboard with and without a name / icon to a Project.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17092
Summary: Ref T11114. We seem to be in reasonable shape here and I don't think anything needs to revert, so rename this back to boring old "edit".
Test Plan: Created, updated, edited a revision via web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17091
Summary: Build ngram indexs, adds search by name capability.
Test Plan: Search for a dashboard by partial name, search for a panel by partial name.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17090
Summary: Ref T11114. This is still mostly in use, but toss a few commit message parsing things.
Test Plan: Viewed/edited/upated blame rev from CLI/web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17089
Summary:
Ref T11114. Keep UI, throw everything else away.
Includes an imperfect-but-not-too-awful fix to keep the field actually working.
Test Plan: Edited tasks from CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17088
Summary: Ref T11114. Keep mail and UI, toss the rest.
Test Plan: Edited/viewed repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17087
Summary: Ref T11114. Keep rendering and mail, toss the rest.
Test Plan: Edited and viewed reviewers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17086
Summary: Ref T11114. Keep UI stuff and mail stuff, toss editing.
Test Plan: Viewed and edited revision summaries.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17085
Summary: Ref T11114. This leaves mail integration and UI integration, but strips all the editing (now handled by EditEngine) and commit message stuff (now handled by CommitMessageField).
Test Plan: Viewed and edited test plans and test plan transactions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17084
Summary: Ref T11114. Obsoleted by Modular Transactions + EditEngine + CommitMessageField + we just "hard code" the title of revisions into the page because we're craaazy.
Test Plan:
- Made an edit on `stable`.
- Viewed the edit on this change, it still had the proper UI strings.
- Edited/created/updated revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17083
Summary: Ref T11114. Obsoleted by EditEngine.
Test Plan: Edited the view policy of a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17082
Summary: Ref T11114. This is obsoleted by `DifferentialSubscribersCommitMessageField` and EditEngine.
Test Plan: Edited a revision's subscribers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17081
Summary: Ref T11114. Obsoleted by `DifferentialRevisionIDCommitMessageField`.
Test Plan:
- Grepped for removed class.
- Created a new revision, verified that the amended message included a proper revision ID.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17080
Summary: Ref T11114. This is replaced by `DifferentialReviewedByCommitMessageField.php`.
Test Plan:
- Used `differential.getcommitmessage` to query an accepted revision, saw "Reviewed By".
- Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17079
Summary: Ref T11114. This is entirely obsoleted by EditEngine.
Test Plan: Edited projects on a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17078
Summary: Ref T11114. This was obsoleted by UI changes and hacked around for performance in T11404. It no longer does anything.
Test Plan: Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17077
Summary: Ref T12027. This is purely a UI hint for new users that I'd like to integrate into "Land Revision" in the future instead.
Test Plan: Grepped for removed class, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12027
Differential Revision: https://secure.phabricator.com/D17076
Summary: Ref T11114. This is obsolted by the narrower `DifferentialGitSVNIDCommitMessageField`.
Test Plan: Browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17075
Summary: Ref T11114. This is now entirely handled by EditEngine and standard policy code.
Test Plan: Edited the edit policy of a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17074
Summary: Ref T11114. This is a pure paring field and now entirely handled by `DifferentialConflictsCommitMessageField`.
Test Plan: Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17073
Summary: Ref T11114. This was obsoleted by the "Stack" graph and does nothing.
Test Plan: Viewed revisions, still saw dependency graphs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17072
Summary: Ref T11114. This hasn't done anything since we moved author information to the subheader.
Test Plan: Browsed Differential, still saw author information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17071
Summary: Ref T11114. This field just stores the value of "Auditors" so you can trigger auditors explicitly later on if you want.
Test Plan: Created and edited revisions with "Auditors".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17070
Summary: Ref T12026. This simplifies the UI and makes T11114 easier. I plan to integrate this into "Download Raw Diff" in the future.
Test Plan:
- Browsed revisions.
- Grepped for removed class name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12026
Differential Revision: https://secure.phabricator.com/D17069
Summary: Ref T11114. This makes the unusual stored custom fields ("Blame Rev", "Revert Plan", etc) work somewhat correctly (?) with EditEngine.
Test Plan:
- Created, updated and edited revisions with unusual stored custom fields like "Blame Rev".
- Observed that these fields now populate in "differential.revision.edit" when available.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17068
Summary:
Ref T11114. This creates `differential.revision.edit` (a modern, v3 API method) and redefines the existing methods in terms of it.
Both `differential.createrevision` and `differential.updaterevision` are now internally implemented by building a `differential.revision.edit` API call and then executing it.
I //think// this covers everything except custom fields, which need some tweaking to work with EditEngine. I'll clean that up in the next change.
Test Plan:
- Created, updated, and edited revisions via `arc`.
- Called APIs manually via test console.
- Stored custom fields ("Blame Rev", "Revert Plan") aren't exposed yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17067
Summary:
Ref T11114. This probably still has some bugs, but survives basic sanity checks.
Continue pulling commit message logic out of CustomField so we can reduce the amount of responsibility/bloat in the classtree and send more code through EditEngine.
Test Plan:
- Called `differential.getcommitmessage` via API console for various revisions/parameters (edit and create mode, with and without fields, with and without revisions).
- Used `--create`, `--edit` and `--update` modes of `arc diff` from the CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17066
Summary: Allows you to set forms via typeahead
Test Plan: `/typeahead/browse/PhabricatorEditEngineDatasource/`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17065
Summary: This is just some housekeeping - see note in D16287. Basically, "isTextMode" doesn't convey enough information.
Test Plan: `git grep isTextMode | grep -v Remarkup`, and visit all callsites; There are 4 of them left.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17063
Summary: Allows you to name and set a project as a menu item navigation element.
Test Plan: Add a project, no name, see project. Remove. Add a project and give it a short name (bugs) and see project link.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17021
Summary:
Fixes T11660. Currently, if you try to diff a path with more than 255 total characters, we fail to create the diff because we have a `text255` column.
There are actually two issues here:
- File names may be arbitrarily long (T11660).
- File names may not be UTF8 (T6633, etc). This is much more complicated and has other issues -- largely that we can't JSON-encode non-UTF8 filenames. I'm punting on that for now and will deal with it later. This doesn't specifically address non-UTF8 paths, although it is a change that's (probably?) required to eventually support them.
This will cause some potentially slow migrations, but better to do them now, if possible, so we have fewer complicated/slow upgrades overall.
Test Plan:
Created a change touching file: //very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_directory_name/very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_long_filename.txt//
{F2137737}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11660
Differential Revision: https://secure.phabricator.com/D17062
Summary: Ref T11114. Missed this while converting.
Test Plan: Tried to create a revision with no test plan. Before: fatal; after: helpful message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17061
Summary:
Ref T11114. See that task for some discussion.
Overall, Differential custom fields ended up with too many responsibilities. Later work in EditEngine provides a more promising model for achieving modularity with smaller, more consistent components.
In particular, we have some custom fields like `DifferentialGitSVNIDField` and `DifferentialConflictsField` which serve //only// to support the field parser.
This starts pulling commit message responsibilities out of the core list of custom fields and into simpler dedicated parsers.
Test Plan: Created and edited revisions from the CLI. Added a bit of test coverage.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17058
Summary:
Ref T11114. I want to move this step away from custom fields. To start with, isolate all the parsing in one class with a clearer API boundary.
Next, I'll make this class use new field objects to perform parsing, without CustomField interactions.
Test Plan: Created and edited revisions from the CLI, using valid and invalid commit messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17055
Summary:
Ref T12020. Ref T11114. If we continue here on a mention, we try to generate `$old`, which requires reviewers to be attached. They won't be for simple codepaths like mentions.
Instead, just bail early: we don't need to do anything anyway since we can't possibly find any more errors with zero transactions.
Test Plan: Mentioned a revision on a task.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11114, T12020
Differential Revision: https://secure.phabricator.com/D17059
Summary: Reorgaizes the CSS here a bit, by object list style, adds in a new drag ui class, which will be used in menu ordering.
Test Plan:
Workboards, Home Apps.
{F2126266}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17057
Summary: Fixes T12015. This is weird and probably got copy/pasted from something else that was also being weird, since the methods were empty and I previously removed them.
Test Plan: Edited a book in Diviner.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12015
Differential Revision: https://secure.phabricator.com/D17056
Summary:
Ref T11114. This replaces the old edit controller with a new one based entirely on EditEngine.
This removes the CustomFieldEditEngineExtension hack for Differential, since remaining field types are fairly straightforward and work with existing EditEngine support, as far as I can tell.
Test Plan:
- Created a revision via web diffs.
- Updated a revision via web diffs.
- Edited a revision via web.
- Edited nonstandard custom fields ("Blame Revision", "JIRA Issues").
- Created a revision via CLI.
- Updated a revision via CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17054
Summary: Ref T11114. Much of this is around making the "comment-while-updating" flow work correctly.
Test Plan:
- Created new diffs by copy/pasting, then:
- used one to create a new revision;
- used one to update an existing revision, with a comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17053
Summary:
If you put "echo" or "print" statements into the code at random places (as I frequently do during development), they would emit before we enabled compression.
This would confuse the compression mechanism and browser. I tried using `headers_sent()` to selectively disable compression but that didn't appear to fix this interaction (I think emitting this text does not cause headers to send, but does let contet escape into some buffer which the compressor can not access).
Instead, push the header down a little bit so it renders after we activate compression.
Also make it slightly fancier / more hideous. WOW.
Test Plan: {F2122927}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17052
Summary: We're throwing here when we actually want to return `null` so we make it into custom field handling code. See Conpherence.
Test Plan: Found a failing task and re-executed it with `bin/worker execute --id <id>`; after this change, it didn't fatal.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17051
Summary: Ref T11114. This one is a bit more complex, but I think I covered everything.
Test Plan:
- Added reviewers.
- Removed reviewers.
- Made reviewers blocking.
- Made reviewers nonblocking.
- Tried to make the author a reviewer.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17050
Summary: Ref T11114. The only real trick here is that we respect configuration in `differential.fields`.
Test Plan: Turned plan on and off, tried to remove the plan, edited the plan.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17048
Summary: Ref T11114. These are unambiguous and always-enabled.
Test Plan: {F2117777}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17047
Summary:
Ref T11114. Currently, all of Differential is extremely custom CustomFields. I want to back away from that somewhat and leverage more EditEngine / ModularTransactions infrastructure.
This allows EditEngine, ModularTransactions, and CustomFields to coexist in an uneasy peace. The "EditPro" controller applies a //different edit// than the CustomFields do, but everything works out in the end. I think.
Hopefully the horrible mess I am creating here will be short-lived.
Test Plan:
- Edited a revision with the normal editor.
- Edited a revision with the pro editor.
- Created a revision with `arc diff`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17044
Summary: Ref T11114. This doesn't really support anything yet, but technically works if you manually go to `/editpro/`.
Test Plan: {F2117302}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17043
Summary: Ref T10967. This makes room for a `DifferentialReviewer` object which can be a real storage table.
Test Plan: Grepped for `DifferentialReviewer`, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17041
Summary: Ref T8475. This gets rid of most of the old "legacy hunk" code. I'll nuke the rest (and drop the old table) once we're more sure that we're in the clear.
Test Plan: Browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8475
Differential Revision: https://secure.phabricator.com/D17040
Summary:
Fixes T7899. If you create or update a revision and type an enormously long first line, we currently fatal trying to insert it into the database.
This text is only used to show a single-line summary of the diff in the "History" tab, which should probably be updated anyway. For now, stop fataling.
Test Plan:
Uploaded a diff with the description "MMMM..." (thousands of them).
Before patch: fatal on description being too long.
After patch: beautiful "MMMM" summary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7899
Differential Revision: https://secure.phabricator.com/D17038
Summary:
Fixes T12013. Send either "Content-Length" or enable output compression, but not both.
Prefer compression for static resources (CSS, JS, etc).
Test Plan: Ran `curl -v ...`, no longer saw responses with both compression and `Content-Length`.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T12013
Differential Revision: https://secure.phabricator.com/D17045
Summary: Allows users set an icon (for reuse on upcoming home) for their dashboard based on 16 descriminating choices.
Test Plan: Create a new dashboard, set new icon. Edit an existing dashboard, set icon.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17042
Summary: Adds authorPHID to panels so we can default to the panels you made.
Test Plan: Run upgrade, visit manage panels, see my panels. Create a new panel. Edit a panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17036
Summary: Fixes T12001. I think we're consistent about using bytes everywhere, but users won't necessarily know that and this documentation could certainly be more clear.
Test Plan: Read new text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12001
Differential Revision: https://secure.phabricator.com/D17037
Summary: Adds an authorPHIDs, populates olds ones.
Test Plan: Make a new Dashboard, see that I created it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17022
Summary: Ref T571. This was accidentally left behind in D12266.
Test Plan: Used {key command F} to search for "bulk".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T571
Differential Revision: https://secure.phabricator.com/D17034
Summary: Fixes T11997. This lifts the targets out of the containing `overflow: hidden;` div so Chrome is willing to target them.
Test Plan: In Chrome, visited direct comment links and ended up in the right place. Clicked comment anchors, saw browser jump around again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11997
Differential Revision: https://secure.phabricator.com/D17033
Summary: Fixes T11999. These are actual panels (SettingsPanel) which are panelley so it's OK.
Test Plan: Clicked "Customize Menu..." on Home.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11999
Differential Revision: https://secure.phabricator.com/D17032
Summary: We're currently applying these styles to labels, restrict them to only list elements that have an href.
Test Plan: Test a label, an anchor, and a button as an action item.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17031
Summary: Cleans up the UI on the page here, uses two column layout, places actions as actionlist instead of dropdown. Changes edit pages to dialogs.
Test Plan: Add an application, divider, link, and facts to a menu page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17030
Summary:
I'm about 90% sure this fixes the intermittent test failure on `testObjectSubscribersPolicyRule()` or whatever.
We use `spl_object_hash()` to identify objects when passing hints about policy changes to policy rules. This is hacky, and I think it's the source of the unit test issue.
Specifically, `spl_object_hash()` is approximately just returning the memory address of the object, and two objects can occasionally use the same memory address (one gets garbage collected; another uses the same memory).
If I replace `spl_object_hash()` with a static value like "zebra", the test failure reproduces.
Instead, sneak an object ID onto a runtime property. This is at least as hacky but shouldn't suffer from the same intermittent failure.
Test Plan: Ran `arc unit --everything`, but I never got a reliable repro of the issue in the first place, so who knows.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17029
Summary: Ref T11957.
Test Plan:
- Viewed an existing project profile.
- Viewed a user profile.
- Created a new project.
- Edited a profile menu.
- Added new profile items.
- Grepped for renamed symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17028
Summary:
Ref T11957. This renames the Configuration storage, transaction, query, and PHID type.
No rename on the actual menu item types yet, that's next (and should be the end of this, I think).
Test Plan:
- Viewed projects.
- Viewed profiles.
- Edited a project menu.
- Grepped for all renamed symbols, I think?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17027
Summary:
- Fixes T11995. This got moved but I missed renaming this callsite.
- Fixes T11993. If you have valid credentials, but haven't run `storage upgrade` yet, we can hit this exception during setup. Just ignore it instead.
Test Plan:
- Saved global settings, no more fatal.
- Changed `storage-namespace` to junk, loaded web UI with valid database credentials.
{F2106358}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11993, T11995
Differential Revision: https://secure.phabricator.com/D17024
Summary: Allows applications to be added as profile menu items
Test Plan: Add an application to a project, see menu item, click on menu. Uninstall application, see menu without application.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17016
Summary: Fixes T11982. If an install is not public, the registering user may not be able to see the inviting user.
Test Plan: {F2097656}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11982
Differential Revision: https://secure.phabricator.com/D17015
Summary: Fixes T11544. Attempt to detect if we're on a tiny, burstable-CPU AWS instance and complain.
Test Plan:
- Completely faked this locally.
- Hit the URI on an EC2 instance to check that it's correct (got back "m3.large", since that was the instance class).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11544
Differential Revision: https://secure.phabricator.com/D17014
Summary:
Fixes T11955. Currently, milestones have an internal name of "Parent (Milestone) ...".
This makes them look like they're prefix matches for "Parent", but they're actually prefix matches for "Milestone".
Reorder the names so that the internal name is "Milestone Parent ...".
Test Plan: Created a project "AAA" with milestone "BBB". Searched for "AAA", found "AAA" and milestone "AAA (BBB)".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11955
Differential Revision: https://secure.phabricator.com/D17013
Summary:
Fixes T11955. Milestone names are currently tokenizing and indexing awkwardly. For example, "A (B C D)" becomes the tokens "A", "(B", "C" and "D)".
The token "(B" can't be searched for since "(" is tokenized on the client.
Instead, tokenize "A (B C D)" into "A", "B", "C", "D".
Test Plan:
- Added unit tests.
- Used `bin/search index --type project --force` to reindex.
- Searched for "A", "B", "C", "D", etc., for real examples.
- Now, found milestones more consistently.
- Also serached for `viewer()`, `members()`, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11955
Differential Revision: https://secure.phabricator.com/D17012
Summary: Fixes T11982. Currently, configuring a custom logo and then setting the policy restrictively locks off the whole install.
Test Plan:
- Configured `ui.logo`.
- Searched for the file PHID in global search to find the underlying file.
- Set the policy to something restrictive ("only me").
- Purged cache (`bin/cache purge --purge-all`).
- Restarted webserver to nuke APC.
- Loaded a page as a different user.
- Before change: policy exception while trying to load the logo.
- After change: fallback to default logo.
- Loaded page as user who can see the logo, got custom logo.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11982
Differential Revision: https://secure.phabricator.com/D17011
Summary:
Ref T11553. With some regularity, users make various configuration mistakes which we can detect by making a request to ourselves.
I use a magical header to make this request because we want to test everything else (parameters, path).
- Fixes T4854, probably. Tries to detect mod_pagespeed by looking for a header. This is a documentation-based "fix", I didn't actually install mod_pagespeed or formally test this.
- Fixes T6866. We now test for parameters (e.g., user somehow lost "QSA").
- Ref T6709. We now test that stuff is decoded exactly once (e.g., user somehow lost "B").
- Fixes T4921. We now test that Authorization survives the request.
- Fixes T2226. Adds a setup check to determine whether gzip is enabled on the web server, and attempts to enable it at the PHP level.
- Fixes `<space space newline newline space><?php` in `preamble.php`.
Test Plan: Tested all of these setup warnings, although mostly by faking them.
Reviewers: joshuaspence, chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T4854, T4921, T6709, T6866, T11553, T2226
Differential Revision: https://secure.phabricator.com/D12622
Summary:
Ref T11954. In cluster configurations, we get repository information by making HTTP calls over Conduit.
These are slower than local calls, so clustering imposes a performance penalty. However, we can use futures and parallelize them so that clustering actually improves overall performance.
When not running in clustered mode, this just makes us run stuff inline.
Test Plan:
- Browsed Git, Mercurial and Subversion repositories.
- Locally, saw a 700ms wall time page drop to 200ms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17009
Summary:
Fixes T11960. In D16998 I removed some code which validated settings on read to improve performance, but lost this replacement validation in shuffling the patch stack.
This restores similar validation before we write the cache. This has the same effect, it's just faster.
Also, bump the cache key to wipe out anything that got bitten (like my account on `secure` rendering dates wrong).
Test Plan:
- Edited settings, verified the edits held.
- Faked invalid settings, saw the check throw exceptions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11960
Differential Revision: https://secure.phabricator.com/D17008
Summary:
Ref T11954. Especially with higher-latency file stores like S3, we can spend a lot of time reading README data and then pulling it out of file storage.
Instead, cache it.
Test Plan: Browsed a repostory with a README, saw faster pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17002
Summary:
Ref T11954. This is kind of complex and I'm not sure I want to actually land it, but it gives us a fairly good improvement for clustered repositories so I'm leaning toward moving forward.
When we make (or receive) clustered repository requests, we must first load a bunch of stuff out of Almanac to figure out where to send the request (or if we can handle the request ourselves).
This involves several round trip queries into Almanac (service, device, interfaces, bindings, properties) and generally is fairly slow/expensive. The actual data we get out of it is just a list of URIs.
Caching this would be very easy, except that invalidating the cache is difficult, since editing any binding, property, interface, or device may invalidate the cache for indirectly connected services and repositories.
To address this, introduce `PhabricatorCacheEngine`, which is an extensible engine like `PhabricatorDestructionEngine` for propagating cache updates. It has two modes:
- Discover linked objects (that is: find related objects which may need to have caches invalidated).
- Invalidate caches (that is: nuke any caches which need to be nuked).
Both modes are extensible, so third-party code can build repository-dependent caches or whatever. This may be overkill but even if Almanac is the only thing we use it for it feels like a fairly clean solution to the problem.
With `CacheEngine`, make any edit to Almanac stuff propagate up to the Service, and then from the Service to any linked Repositories.
Once we hit repositories, invalidate their caches when Almanac changes.
Test Plan:
- Observed a 20-30ms performance improvement with `ab -n 100`.
- (The main page making Conduit calls also gets a performance improvement, although that's a little trickier to measure directly.)
- Added debugging code to the cache engine stuff to observe the linking and invalidation phases.
- Made invalidation throw; verified that editing properties, bindings, etc, properly invalidates the cache of any indirectly linked repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17000
Summary:
Ref T11954. This reduces how much work we need to do to load settings, particularly for Conduit (which currently can not benefit directly from the user cache, because it loads the user indirectly via a token).
Specifically:
- Cache builtin defaults in the runtime cache. This means Phabricator may need to be restarted if you change a global setting default, but this is exceptionally rare.
- Cache global defaults in the mutable cache. This means we do less work to load them.
- Avoid loading settings classes if we don't have to.
- If we missed the user cache for settings, try to read it from the cache table before we actually go regenerate it (we miss on Conduit pathways).
Test Plan: Used `ab -n100 ...` to observe a ~6-10ms performance improvement for `user.whoami`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16998
Summary:
Ref T11954. I want to store some lists/arrays in the mutable (database) cache, but it only supports string storage.
Provide a serializing wrapper which flattens when values are written and expands them when they're read.
Test Plan: Used by D16997. See that revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16999
Summary:
Ref T11954. When we query for Conduit tokens, we load the associated objects (users) by PHID.
Currently, querying objects by PHID requires us to load every PHIDType class, when we can know which specific classes we actually need (e.g., just `UserPHIDType`, if only user PHIDs are present in the query).
Use PhabricatorCachedClassMapQuery to reduce the number of classes we load on this pathway.
Test Plan:
- Used `ab -n100` to roughly measure a ~5% performance improvement?
- This measurement feels a little flimsy but the XHProf profile is cleaner, at least.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16997
Summary: Ref T11954. This old query class can use slightly more modern code.
Test Plan: Ran Conduit methods, verified results are unchanged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16996
Summary: Ref T11954. Depends on D16994. This implements the Conduit method cache described in that revision for a small global Conduit performance improvement.
Test Plan: Verified Conduit has the same behavior at lower cost. See D16994 for details.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16995
Summary:
Ref T11954. Depends on D16993. We have a couple of "look up the class for this key" queries which are costly enough to show up on a profile.
These aren't huge wins, but they're pretty easy. We currently do this like this:
```
$class_map = load_every_subclass();
return idx($class_map, $key);
```
However, we don't need to load EVERY subclass if we're only looking for, say, the Conduit method subclass which implements `user.whoami`. This allows us to cache that map and find the right class efficiently.
This cache is self-validating and completely safe even in development.
Test Plan:
- Used `curl` to make queries to `user.whoami`, verified that content was identical before and after the change.
- Used `ab -n100` to roughly measure 99th percentile time, which dropped from 74ms to 65ms. This is a small improvement (13% in the best case, here) but it benefits every Conduit method call.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16994
Summary:
Ref T11954. Depends on D16992. We have some data which can be generated and cached at runtime. Three examples are:
- Class map from Conduit method names to implementing classes.
- Class map from PHID types to implementing classes.
- The main routing map.
None of these are huge wins but they impose global costs and can be shaved down through caching without introducing an enormous amount of new complexity.
The cost to these maps is that sometimes you'll need to restart your webserver, even in development mode if these caches are active. However, in some cases these changes are very rare, and in other cases we can just leave the cache disabled in development mode without a huge complexity cost.
Specifically, the Conduit/PHID type class maps are self-validating and can not go bad, even in development mode.
The routing map will be able to, but I plan to just disable it in development mode.
This provides a general-purpose pure APC cache stack for storing this data.
Test Plan: See future changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16993
Summary: Ref T929. When viewing a branch, show a few recent differences from the default branch (usually, "master").
Test Plan: {F2079220}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T929
Differential Revision: https://secure.phabricator.com/D16991
Summary: Ref T929. We've made some UI updates since D15330.
Test Plan: {F2079125}
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T929
Differential Revision: https://secure.phabricator.com/D16990
Summary:
This shows the commits list only (Actual `git diff` will show up at a later date).
The inputs are left as text-fields, to allow the form to accept anything that can be resolved. The form is GET, to allow sharing URIs.
The conduit method response array is compatible with that of `diffusion.historyquery`, to make it easy to build
the "history" table.
The hardest part here was, of course, Naming. I think "from" and "onto" are unconfusing, and I'm fairly confident that the "to merge"
instructions are in sync with the actual content of the page.
Test Plan: Look at several "compare" views, with various values of "from" and "onto".
Reviewers: #blessed_reviewers!, epriestley
Subscribers: caov297, 20after4, Sam2304, reardencode, baileyb, chad, Korvin
Maniphest Tasks: T929
Differential Revision: https://secure.phabricator.com/D15330
Summary:
Persona is going to be decommed November 30th, 2016.
It is highly unlikely that anyone is currently using persona as a real
login method at this point.
Test Plan: tried locally to add auth adapter.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16371
Summary:
Fixes T11940. In 2.11.0, Git has made a change so that newly-pushed changes are held in a temporary area until the hook accepts or rejects them.
This magic temporary area is only readable if the appropriate `GIT_ENVIRONMENTAL_MAGIC` variables are available. When executing `git` commands, pass them through from the calling context.
We're intentionally conservative about which variables we pass, and with good reason (see "httpoxy" in T11359). I think this continues to be the correct default behavior.
Test Plan:
- Upgraded to Git 2.11.0.
- Tried to push over SSH, got a hook error.
- Applied patch.
- Pulled and pushed over SSH.
- Pulled and pushed over HTTP.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11940
Differential Revision: https://secure.phabricator.com/D16988
Summary:
Ref T11939. IPv4 addresses can normally only be written in one way, but IPv6 addresses have several formats.
For example, the addresses "FFF::", "FfF::", "fff::", "0ffF::", "0fFf:0::", and "0FfF:0:0:0:0:0:0:0" are all the same address.
Normalize all addresses before writing them to logs, etc, so we store the most-preferred form ("fff::", above).
Test Plan:
Ran an SSH clone over IPv6:
```
$ git fetch ssh://local@::1/diffusion/26/locktopia.git
```
It worked; verified that address read out of `SSH_CLIENT` sensibly.
Faked my remote address as a non-preferred-form IPv6 address using `preamble.php`.
Failed to login, verified that the preferred-form version of the address appeared in the user activity log.
Made IPv6 requests over HTTP:
```
$ curl -H "Host: local.phacility.com" "http://[::1]/"
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11939
Differential Revision: https://secure.phabricator.com/D16987
Summary:
Ref T11939. Depends on D16984. Now that CIDRLists can contain IPv6 addresses, blacklist all of the reserved IPv6 space.
This reserved blacklist is used to prevent users from accessing internal services via "Import Calendar" or "Add Macro".
They can't actually reach IPv6 addresses via these mechanisms yet because we need to do more work to support outbound IPv6 requests, but make sure reserved IPv6 space is blacklisted already when that support eventaully arrives.
Also, clean up some error messages (e.g., for trying to hit a bad URI in "Add Macro").
Test Plan:
- Loaded pages with default blacklist.
- Tried to make requests into IPv6 space.
- Currently, this is impossible because of `parse_url()` and `gethostynamel()` calls.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11939
Differential Revision: https://secure.phabricator.com/D16986
Summary: The default behavior of these methods is to return `true`, so these overrides have no effect.
Test Plan: `grep`; poked around.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Differential Revision: https://secure.phabricator.com/D16985
Summary:
Fixes T11946. When a logged-out viewer is loading a page on a non-public install, there are two policy issues which prevent them from loading global settings:
- They can not see the Settings application itself.
- They can not see the global settings object.
Allow them to see Settings by making mandatory applications always visible. (This doesn't make any application pages public.)
Allow them to see the global settings object explicitly.
Test Plan:
Changed default language, viewed logged-out page:
{F2076924}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11946
Differential Revision: https://secure.phabricator.com/D16983
Summary: Ref T3612, prevents lightbox from spawning from inside a lightbox.
Test Plan: Click on file lightbox, leave file comment, click file comment, get take to file page instead of another lightbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16978
Summary: Ref T3612, this adds a anchor around the large icon with hover state so you can download from here as well.
Test Plan: Hover over .ics file, click, get download.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16977
Summary:
Fixes T11938.
Note that there's a subcase here: if you `hg clone` or `svn checkout` a short `/source/` URI that ends in `.git`, we miss the lookup and don't get this far, so you still get a generic error message.
Hopefully it is clear enough on its own that `proto://.../blah.git` is, in fact, a Git repository, since it says ".git" at the end.
If that doesn't prove to be true, we can be more surgical about this.
Test Plan:
```
$ git clone ssh://local@localvault.phacility.com/source/quack.notgit/
Cloning into 'quack.notgit'...
phabricator-ssh-exec: This repository ("quack.notgit") is not a Git repository. Use "hg" to interact with this repository.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
```
$ hg clone ssh://local@localvault.phacility.com/source/phabx
remote: phabricator-ssh-exec: This repository ("phabx") is not a Mercurial repository. Use "git" to interact with this repository.
abort: no suitable response from remote hg!
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11938
Differential Revision: https://secure.phabricator.com/D16976
Summary: Ref T11801. In some cases, this could lead to us failing to generate the first recurrence in a series.
Test Plan: Imported `weekly.ics` (from D16974) and saw an event correctly occur on Aug 18, with my local timezone set to "America/Los_Angeles".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11801
Differential Revision: https://secure.phabricator.com/D16975
Summary:
Fixes T11936. After editing a repository URI, we were not correctly updating the URI index.
Any other edit to the repository //would// update the index, and this index is only really used by `arc` to figure out which repository a working copy belongs to, so that's how this evaded detection for this long. In particular, creating a repository would usually have an edit after any URI edits, to activate it, which would build the index correctly.
Test Plan:
- Added a new URI to a repository.
- Verified it was immediately reflected in the `repository_uriindex` table.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11936
Differential Revision: https://secure.phabricator.com/D16972
Summary:
Ref T11922. When we deploy on Saturday I need to rebuild all the cluster indexes, but some instances won't have anything indexed so they won't actually trigger the activity.
Add a `--force` flag that just clears an activity even if the activity is not required.
Test Plan: Ran `bin/config done reindex --force` several times.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11922
Differential Revision: https://secure.phabricator.com/D16970
Summary:
Currently, custom Sites must match `.*` or similar to handle 404's, since the fallback is always generic.
This locks them out of the "redirect to canonicalize to `path/` code", so they currently have a choice between a custom 404 page or automatic correction of `/`.
Instead, allow the 404 controller to be constructed explicitly. Sites can now customize 404 by implementing this method and not matching everything.
(Sites can still match everything with a catchall rule if they don't want this behavior for some reason, so this should be strictly more powerful than the old behavior.)
See next diff for CORGI.
Test Plan:
- Visited real 404 (like "/asdfafewfq"), missing-slash-404 (like "/maniphest") and real page (like "/maniphest/") URIs on blog, main, and CORGI sites.
- Got 404 behavior, redirects, and real pages, respectively.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16966
Summary:
Ref T11922. After updating to HEAD of `master`, you need to manually rebuild the index. We don't do this during `bin/storage upgrade` because it can take a very long time (`secure.phabricator.com` took roughly an hour) and can happen while Phabricator is running.
However, if we don't warn users about this they'll just get a broken index unless they go read the changelog (or file an issue, then we tell them to go read the changelog).
This adds a very simple table for notes to administrators so we can write a "you need to go rebuild the index" note, then adds one.
Administrators clear the note by completing the activity and running `bin/config done reindex`. This isn't automatic because there are various strategies you can use to approach the issue, which I'll discuss in greater detail in the linked documentation.
Also, fix an issue where `bin/storage upgrade --apply <patch>` could try to re-mark an already-applied patch as applied.
Test Plan:
- Ran storage ugrades.
- Got instructions to rebuild search index.
- Cleared instructions with `bin/config done reindex`.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T11922
Differential Revision: https://secure.phabricator.com/D16965
Summary: This is still reasonably functional and useful to people, and we don't have better mechanics to offset the change.
Test Plan: New Workboard, set Workboard color, test mobile, desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16964
Summary: Ref T3612. Mobilizes the new lightbox, changes large buttons to circle icons like Conpherence.
Test Plan: Click each new button on desktop, mobile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16961
Summary:
Fixes T11929. When running with a query, we no longer enforce an order on the subquery join to produce results more quickly when searching for common strings.
However, this means that empty queries (like those issued by "Close as Duplicate") don't order subquery results.
Restore a `dateCreated` order if there is no query text.
Test Plan: Artificially set limit to 10, still saw 10 most recent tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11929
Differential Revision: https://secure.phabricator.com/D16960
Summary: Ref T3612. Passes in file size and file icon for non-images.
Test Plan: Review a PDF and PSD in a lightbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16957
Summary:
Fixes T11909. Ref T11816. Instead of offering a dropdown with choices between "Edit/Cancel/Reinstate This Event" and "Edit/Cancel/Reinstate Future Events", make the choice more explicit.
This dialog ends up pretty wordy but this edit is rare, so I think that's alright.
Test Plan: {F2046863}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816, T11909
Differential Revision: https://secure.phabricator.com/D16956
Summary:
Ref T11816. Currently, if someone in California creates an event and then someone in New York edits it, we generate a no-op "<user> changed the start time from 3PM to 3PM." transaction.
This is because the internal timezone of the event is changing, but the actual absolute time is not.
Instead, when an edit wouldn't reschedule an event and would only change the internal timezone, ignore the edit.
Test Plan:
- Edited non-all-day events in PST / EST with out making changes (ignored).
- Edited non-all-day events in PST / EST with changes (changes worked).
- Performed the same edits with all-day events, which also were ignored and worked, respectively.
- Pulled events in and out of all-day mode in different timezones, behavior seemeed reasonable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16955
Summary:
In D16936, I changed logged-out viewers so they use global settings.
This can lead to a `SELECT` from an isolated unit test. Instead, give the test fixtures and use standard `generateNewUser()` stuff.
Test Plan: Ran `arc unit --everything`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16952
Summary:
Ref T11816. I don't really know what happened here, maybe I rewrote and broke this at the last second?
In most cases, we directly respect the `isAllDay` flag on the event, so the internal date state doesn't matter too much.
However, in the case of mail notifications, the raw internal state is relevant. This should fix mail notifications for all-day events.
(I might still turn them off since I'm not sure they're too useful, but it's good to have them working.)
Test Plan:
- Created a new all-day event, verified database values wrote correctly.
- Ran `bin/calendar notify --trace`, verified it picked up an all-day event tomorrow with a large enough `--minutes` value.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16954
Summary: Spruce up the file embeds a little more, hover state, icons, file size.
Test Plan:
Add a psd and pdf, see new icons. Check differential, still see icons there too. Test mobile, desktop.
{F2042539}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16950
Summary: Found these in the `secure` error logs: one bad call, one bad column.
Test Plan: Searched for empty string. Double-checked method name.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16948
Summary:
Ref T6740. When we index a document, also save a copy of the stemmed version.
When querying, search the combined corpus for the terms.
(We may need to tune this a bit later since it's possible for literal, quoted terms to match in the stemmed section, but I think this wil rarely cause issues in practice.)
A downside here is that search sort of breaks if you upgrade into this and don't reindex. I wasn't able to find a way to issue the query that remained compatible with older indexes and didn't have awful performance, so my plan is:
- Put this on `secure`.
- Rebuild the index.
- If things look good after a couple of days, add a way that we can tell people they need to rebuild the search index with a setup warning.
We might get some reports between now and then, but if this is super awful we should know by the end of the weekend.
Test Plan:
WOW AMAZING
{F2021466}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6740
Differential Revision: https://secure.phabricator.com/D16947
Summary: Ref T11741. I'll wait until the release cut to land this; it just adds a test for InnoDB FULLTEXT being available instead of always returning `false`.
Test Plan:
- Ran with InnoDB fulltext locally for a day and a half without issues.
- Ran `bin/storage upgrade`, saw it detect InnoDB fulltext.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D16946
Summary:
Ref T6740. Currently, we issue fulltext queries with an "ORDER BY <score>" on the entire result set.
For very large result sets, this can require MySQL to do a lot of work. However, this work is generally useless: if you search for some common word like "diff" or "internet" or whatever and match 4,000 documents, the chance that we can score whatever thing you were thinking of at the top of the result set is nearly nothing. It's more useful to return quickly, and let the user see that they need to narrow their query to get useful results.
Instead of doing all that work, let MySQL find up to 1,000 results, then pick the best ones out of those.
This actual change is a little flimsy, since our index isn't really big enough to suffer indexing issues. However, searching for common terms on my local install (where I have some large repositories imported and indexed) drops from ~40ms to ~10ms.
My hope is to improve downstream performance for queries like "translatewiki" here, particularly:
<https://phabricator.wikimedia.org/T143863>
That query matches about 300 trillion documents but there's a ~0% chance that the one the user wants is at the top. It takes a couple of seconds to execute, for me. Better to return quickly and let the user refine their results.
I think this will also make some other changes related to stemming easier.
This also removes the "list users first" ordering on the query, which made performance more complicated and seems irrelevant now that we have the typeahead.
Test Plan:
- Searched for some common terms like "code" locally, saw similar results with better performance.
- Searched for useful queries (e.g., small result set), got identical results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6740
Differential Revision: https://secure.phabricator.com/D16944
Summary:
Ref T11741. This makes everything work if we switch to InnoDB, but never actually switches yet.
Since the default minimum word length (3) and stopword list (36 common English words) in InnoDB are generally pretty reasonable, I just didn't add any setup advice for them. I figure we're better off with simpler setup until we identify some real problem that the builtin stopwords create.
Test Plan: Swapped the `false` to `true`, ran `storage adjust`, got InnoDB fulltext indexes, searched for stuff, got default "AND" behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D16942
Summary:
Ref T11741. On recent-enough versions of MySQL, we would prefer to use InnoDB for fulltext indexes instead of MyISAM.
Allow `bin/storage adjust` to read actual and expected table engines, and apply adjustments as necessary.
We have one existing bad table that uses the wrong engine, `metamta_applicationemail`. This change corrects that table.
Test Plan:
- Ran `bin/storage upgrade`.
- Saw the adjustment phase apply this change properly:
```
>>>[463] <query> ALTER TABLE `local_metamta`.`metamta_applicationemail` COLLATE = 'utf8mb4_bin', ENGINE = 'InnoDB'
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D16941
Summary:
Ref T11741. InnoDB uses a stopwords table instead of a stopwords file.
During `storage upgrade`, synchronize the table from the stopwords file on disk.
Test Plan:
- Ran `storage upgrade`.
- Ran `select * from stopwords`, saw stopwords.
- Added some garbage to the table.
- Ran `storage upgrade`, saw it remove it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D16940
Summary:
Ref T11741. Fixes T10642. Parse and compile user queries with a consistent ruleset, then submit queries to the backend using whatever ruleset MySQL is configured with.
This means that `ft_boolean_syntax` no longer needs to be configured (we'll just do the right thing in all cases).
This should improve behavior with RDS immediately (T10642), and allow us to improve behavior with InnoDB in the future (T11741).
Test Plan:
- Ran various queries in the UI, saw the expected results.
- Ran bad queries, got useful errors.
- Searched threads in Conpherence.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10642, T11741
Differential Revision: https://secure.phabricator.com/D16939
Summary:
Fixes T11894. Currently, if you aren't attending any events for a while, we can cache that you are free for the next 72 hours, even if you have an event in a few hours.
Instead, only cache "user is free" until the next event, if one exists.
Test Plan: Dumped cache TTLs, saw 52 minutes instead of ~4300 minutes with a near-upcoming event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11894
Differential Revision: https://secure.phabricator.com/D16937
Summary: Fixes T11917. Give logged-out / omnipotent users the global settings, not the default settings.
Test Plan: Changed applications and language, logged out, saw changes as a public user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11917
Differential Revision: https://secure.phabricator.com/D16936
Summary:
Currently, when a payment method is invalid we still render the full name and let you save the form without making changes. This can be confusing.
Instead:
- Render "<Deleted Payment Method>", literally.
- Render an error immediately.
- Prevent the form from being saved without changing the method.
Test Plan: {F1955487}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16935
Summary:
Ref T11044. This is still catching the older exceptions, which are now more general.
If you loaded the web UI without MySQL running, this meant you got a less-helpful error.
Test Plan: Stopped MySQL, loaded web UI, got a more-helpful error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16930
Summary:
Ref T11044.
- Use shorter lock names. Fixes T11916.
- These granular exceptions now always raise as a more generic "Cluster" exception, even for a single host, because there's less special code around running just one database.
Test Plan:
- Configured bad `mysql.port`, ran `bin/storage upgrade`, got a more helpful error message.
- Ran `bin/storage upgrade --trace`, saw shorter lock names.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044, T11916
Differential Revision: https://secure.phabricator.com/D16924
Summary: Fixes T11845. Users can still embed a text panel on the home page to give it some ambiance.
Test Plan: Wrote an autoplay video as a comment, saw it in feed. Before change: autoplay. After change: no auto play. On task: still autoplay.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11845
Differential Revision: https://secure.phabricator.com/D16920
Summary: Fixes T11910. I spent a couple of minutes looking for the root cause without much luck, but this will all be obsoleted by an eventual upgrade to `EditEngine` anyway.
Test Plan: Set and unset "Wait for Message", which now worked.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11910
Differential Revision: https://secure.phabricator.com/D16919
Summary: Removes the viewable restriction on embedded files. Builds a basic lightbox UI for commenting.
Test Plan:
Add psd, pdf to Maniphest task, clicked on download, comment, left comment. Closed box.
{F1943726}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16917
Summary:
Ref T11044. Few issues here:
- The `PhutilProxyException` is missing an argument (hit this while in read-only mode).
- The `$ref_key` is unused.
- When you add a new master to an existing cluster, we can incorrectly apply `.php` patches which we should not reapply. Instead, mark them as already-applied.
Test Plan:
- Poked this locally, but will initialize `secure004` as an empty master to be sure.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16916
Summary: Ref T11044. Fixes T11672. In T11672, persistent connections seem to work fine, but they can require `max_connections` and other settings to be raised. Since most users don't need them, make them an advanced option.
Test Plan: Configured persistent connections, loaded some pages, observed persistent connections get used.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044, T11672
Differential Revision: https://secure.phabricator.com/D16913
Test Plan: `arc unit`, see test name in list.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16915
Summary:
Ref T11044. Sometimes we have a sequence of patches like this:
- `01.newtable.sql`: Adds a new table to Files.
- `02.movedata.php`: Moves some data that used to live in Tokens to the new table.
This is fairly rare, but not unheard of. More commonly, we can have this sequence:
- `03.newtable.sql`: Add a new column to Phame.
- `04.setvalue.php`: Copy data into that new column.
In both cases, when applying database-by-database, we can get into trouble.
- In the first case, if Files is on a different master, we'll try to move data into a new table before creating the table.
- In the second case, if Phame is on a different master, the PHP patch will connect to it before we add the new column.
In either case, we try to interact with tables or columns which don't exist yet.
Instead, apply each patch in order, to all databases which need it. So we'll apply `01.newtable.sql` EVERYWHERE first, then move on.
In the case of PHP patches, we also now only apply them once, since they never make schema changes. It should normally be OK to apply them more than once safely, but this is a little faster and a little safer if we ever make a mistake.
Test Plan:
- Ran `bin/storage upgrade` on single-host and clustered setups.
- Initialized new storage on single-host and clustered setups.
- Upgraded again after initialization.
- Ran with `--apply`.
- Ran with `--dry-run`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16912
Summary:
Ref T11044. This was old Facebook cruft for reading configuration from SMC (and maybe doing some other questionable things). See D183.
(See also D175 for discussion of this from 2011.)
In modern Phabricator, you can subclass `SiteConfig` to provide dynamic configuration, and we do so in the Phacility cluster. This lets you change any config, and change in response to requests (e.g., for instancing) and is generally more powerful than this mechanism was.
This configuration provider theoretically let you roll your own replication or partitioning, but in practice I believe no one ever did, and no one ever could have anyway without more support in the upstream (for migrations, read-after-write, etc).
Test Plan:
- Grepped for removed option.
- Browsed around with clustering off.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16911
Summary:
Ref T11044. One popular tool in a modern operations environment is Puppet. The primary purpose of this tool is to randomly revert hosts to older or different configurations.
Introducing an element of chaotic unpredictability into operations trains staff to be on high alert at all times, rather than lulled into complacency by predictability or consistency.
When Puppet reverts a Phabricator host's configuration to an older version, we might start writing data to a lot of crazy places where it shouldn't go. This will create a big sticky mess that is virtually impossible to undo, mostly because we'll get two files with ID 123 or two tasks with ID 456 or whatever else and good luck with that.
Instead, after changing the partition layout, require `bin/storage partition` to be run. This writes a copy of the config everywhere.
Then, when we start serving web requests, make sure every database has the exact same config. This will foil Puppet by refusing to run requests on hosts it has reverted.
Test Plan:
- Changed partition configuration.
- Ran Phabricator.
- FOILED!
- Ran `bin/storage partition` to sync config.
- Things worked again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16910
Summary:
Ref T11044. Fixes T10931. This option has essentially never been useful for anything, and we've picked the best implementation for a long time (MySQLi if available, MySQL if not).
I am not aware of any reason to ever set this manually. If someone comes up with some bizarre but legitimate use case that I haven't thought of, we can modularize it.
Test Plan: Browsed around. Grepped for `mysql.implementation`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10931, T11044
Differential Revision: https://secure.phabricator.com/D16909
Summary: Adds a comment box, you can put text into it, hit enter, and see it come back.
Test Plan: Put text into box, see it come back.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16907
Summary:
I frequently run into a situation where I want to kill tasks that have accumulated a lot of failures regardless of what class they are. Or I'll want to kill every worker of a certain class but only if it has failed at least once. This change allows me to run `./bin/worker cancel --class <MYCLASS> --min-failure-count 5` to only kill tasks with at least 5 failed attempts.
The `--min-failure-count N` argument can be used by itself as well as with `--class CLASSNAME`. I don't think it makes sense for it to work with `--id ID`, but I'm not dead set on that or anything.
Test Plan: I ran the worker management workflow with and without the `--min-failure-count` argument and it worked as expected.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley, yelirekim
Differential Revision: https://secure.phabricator.com/D16906
Summary:
Fixes T10759. Fixes T11817. This runs all the general sanity/configuration checks on all the active servers.
None of these warnings are very important, and this doesn't change any logical stuff.
Depends on D16904.
Test Plan: Painstakingly triggered each warning, verified that they rendered correctly and that messages told me which host was affected.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10759, T11817
Differential Revision: https://secure.phabricator.com/D16905
Summary:
Ref T10759. Check master/replica status during startup.
After D16903, this also means that we check this status after a database comes back online after being unreachable.
If a master is replicating, fatal (since this can do a million kinds of bad things).
If a replica is not replicating, warn (this just means the replica is behind so some data is at risk).
Also: if your masters were actually configured properly (mine weren't until this change detected it), we would throw away patches as we applied them, so they would only apply to the //first// master. Instead, properly apply all migration patches to all masters.
Test Plan:
- Started Phabricator with a replicating master, got a fatal.
- Stopped replication on a replica, got a warning.
- With two non-replicating masters, upgraded storage.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10759
Differential Revision: https://secure.phabricator.com/D16904
Summary:
Ref T10759. We may "discover" the presence of a fatal setup error later, after starting Phabricator.
This can happen in a few ways, but most are unlikely. The one I'm immediately concerned about is:
- Phabricator starts up during a disaster with some databases unreachable.
- We start with warnings (unreachable databases are generally not fatal, since it's OK for some subset of hosts to be down in replicated/partitioned setups).
- The unreachable databases later recover and become accessible again.
- When we run checks against them, we discover that they are misconfigured.
Currently, "fatal" setup issues are not truly fatal if we're "in flight" -- we've survived setup checks at least once in the past. This is bad in the scenario above.
Especially with partitioning, it could lead to mangled data in a disaster scenario where operations staff makes a small configuration mistake while trying to get things running again.
Instead, if we "discover" a fatal error while already "in flight", reset the whole setup process as though the webserver had just restarted. Don't serve requests again until we can make it through setup without hitting fatals.
Test Plan:
- Started Phabricator with multiple masters, one of which was down and broken.
- Got a warning about the bad master.
- Revived the master.
- Before: Phabricator detects the fatal, but keeps serving requests.
- After: Phabricator detects the fatal, resets the webserver, and stops serving requests until the fatal is resolved.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10759
Differential Revision: https://secure.phabricator.com/D16903
Summary:
Ref T10759. Currently, these checks run only against configured masters. Instead, check every host.
These checks also sort of cheat through restart during a recovery, when some hosts will be unreachable: they test for "disaster" by seeing if no masters are reachable, and just skip all the checks in that case.
This is bad for at least two reasons:
- After recent changes, it is possible that //some// masters are dead but it's still OK to start. For example, "slowvote" may have no master, but everything else is reachable. We can safely run without slowvote.
- It's possible to start during a disaster and miss important setup checks completely, since we skip them, get a clean bill of health, and never re-test them.
Instead:
- Test each host individually.
- Fundamental problems (lack of InnoDB, bad schema) are fatal on any host.
- If we can't connect, raise it as a //warning// to make sure we check it later. If you start during a disaster, we still want to make sure that schemata are up to date if you later recover a host.
In particular, I'm going to add these checks soon:
- Fatal if a "master" is replicating.
- Fatal if a "replica" is not replicating.
- Fatal if a database partition config differs from web partition config.
- When we let a database off with a warning because it's down, and later upgrade it to a fatal because we discover it is broken after it comes up again, fatal everything. Currently, we keep running if we "discover" the presence of new fatals after surviving setup checks for the first time.
Test Plan:
- Configured with multiple masters, intentionally broke one (simulating a disaster where one master is lost), saw Phabricator still startup.
- Tested individual setup checks by intentionally breaking them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10759
Differential Revision: https://secure.phabricator.com/D16902
Summary:
Fixes T11902.
- Periods now work in short names.
- If you try to name something ".git", no dice.
Test Plan:
- Tried to name something "quack.git", was politely rejected.
- Named something "quack.notgit", and it worked fine.
- Cloned Mercurial and Git repositories over SSH with ".git" and non-".git" variants without hitting any issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11902
Differential Revision: https://secure.phabricator.com/D16908
Summary:
Ref T11044. I'm going to hold this until after the release cut, but I think it's good to go.
This allows installs to configure multiple masters in `cluster.databases` and partition applications across them (for example, put Maniphest on a dedicated database).
When we make a Maniphest connection we go look up which master we should be hitting first, then connect to it.
This has at least approximately been planned for many years, so the actual change is largely just making sure that your config makes sense.
Test Plan:
- Configured `db001.epriestley.com` and `db002.epriestley.com` as master/master.
- Partitioned applications between them.
- Interacted with various applications, saw writes go to the correct host.
- Viewed "Database Servers" and saw partitioning information.
- Ran schema upgrades.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16876
Summary:
Ref T8345. See T8345#201048 for discussion.
This rule (don't show mentions of or from restricted objects) is more consistent with how we render mentions in the timeline and I think generally a better behavior.
Test Plan:
- Mentioned a task on a public task and a private task.
- Privileged user (foreground) sees both.
- Public user (background) sees only the public mention.
{F1929485}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8345
Differential Revision: https://secure.phabricator.com/D16900
Summary: Basic work in progress, but should show timeline comments for files when in lightbox mode. Looks reasonable.
Test Plan: click on images, see comments from timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3612
Differential Revision: https://secure.phabricator.com/D16896
Summary: Ref T11816. We currently color circles green if you're invited, but should color them green if you're attending, too.
Test Plan: Viewed calendar mobile month view, saw attending events in green.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16899
Summary:
Ref T11816. In some cases, Calendar would only export a subset of events because the "export" flag was ignored or the "display" parameter applied an improper date range to the query.
- Make sure the `export` flag gets processed, even though it isn't a "real" field on the search engine.
- Clear the "display" parameter to avoid date range windowing coming from the day/month logic.
Test Plan: Exported a "display=month" view, verified future events came with it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16898
Summary:
See downstream issue here: <https://phabricator.wikimedia.org/T150992>
In at least one case (project milestones) we have a locked, non-lockable field. This means "this is locked, and you can't change the fact that it is locked".
At least for now, preserve this behavior.
Test Plan: Created a new milestone of an existing project. This worked correctly with the patch.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16895
Summary: Fixes T11882. Document using `~/.ssh/config` to mitigate the inconvenience of port 2222.
Test Plan: Read document.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11882
Differential Revision: https://secure.phabricator.com/D16894
Summary: This supports doing a bunch of sales funnel tracking on Phacility.
Test Plan: See next diff.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16890
Summary: Ref T11034. Try to produce a roughly-one-sentence summary instead of a roughly-one-paragraph summary for the browse dialog.
Test Plan:
- Added unit tests, ran unit tests.
- Wrote a longer summary for a project, browsed to it, saw a shorter summary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11034
Differential Revision: https://secure.phabricator.com/D16892
Summary: This code should go inside the field-locking loop. Otherwise, it only applies to the last field, and fatals if there are no fields.
Test Plan: Carefuller inspection.
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4
Differential Revision: https://secure.phabricator.com/D16889
Summary:
Certain unusual queries, like `[-]`, could tokenize into a list which included the empty string.
This would then convert into a query for `... LIKE "%"` which just joins the entire table.
Instead: tokenize smarter; never return the empty token; add some test cases.
Test Plan: Ran unit tests. Queried for `[[blah blah]]`, saw a reasonable query come out the other end.
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4
Differential Revision: https://secure.phabricator.com/D16888
Summary:
Ref T11887. This isn't a great fix but makes the method behave properly until I get around to a real fix.
In the longer term, I want to convert all of this pluggable Harbormaster/Drydock stuff (blueprints, artifacts, build plans) to use EditEngine + EditField instead of the weird mishmash of older/custom stuff it currently uses. However, this is a more involved project to execute and I'd like to be in that area of the codebase first so it gets adequate testing.
Until that happens, just put a reasonble-ish mechanism in place to let artifacts correct inbound types. This is the only artifact type and only parameter which needs casting.
Test Plan:
- Made a `curl` call to `harbormaster.createartifact` to create a URI artifact with `?...&ui.external=1`.
- Before patch: type error on `ui.external` not being a boolean.
- After patch: artifact created successfully.
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11887
Differential Revision: https://secure.phabricator.com/D16887
Summary:
Ref T8510. Use "\n" as a delimiter between name sections. Specifically, project "AAA" with tag "zzz" should be a better match for query "AAA" than project "AAA BBB" is.
Make use of this delimiter slighlty more obvious in the UI.
Test Plan:
- Created projects "Phacility" and "Phacility Core Access".
- Typed "Phacility".
- Before patch: first hit is "Phacility Core Access".
- After patch: first hit is "Phacility".
- Viewed debugging output table, saw visual explanation of behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8510
Differential Revision: https://secure.phabricator.com/D16886
Summary:
Fixes T11785. Lightbox calls `JX.Stratcom.pass()` to let other handlers react, but should not. At least today, we never put, e.g., links inside a lightbox.
This code appears in the original commit so it was probably just copy/pasted from somewhere and I missed it in review.
(Or there's some edge case I'm not thinking of and we'll figure it out soon enough.)
Additionally, blacklist `/file/data/` from Quicksand naviagtion: Quicksand should never fetch these URIs.
Test Plan:
- Disabled `security.alternate-file-domain`.
- Enabled Quicksand ("Persistent Chat").
- Clicked an image thumbnail on a task.
- Repeated that until things flipped out a bit.
- After the patch: no issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11785
Differential Revision: https://secure.phabricator.com/D16884
Summary: Visually, I think these are much cleaner (with colors), and provide more workspace. I also don't really use the sidenav here and if I did, it would be to go back to the project homepage. I think this is overall better. If navigation page to project home is difficult or hard to find, we can maybe make a better header / crumbs bar to reduce that.
Test Plan: New project -> basic new board. Existing project -> color board. Desktop, Mobile, Fullscreen
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16882
Summary: Ref T11816. This could be a little cleaner, but we currently have two copies of the logic. Get them using the same code. Once that's actually working I can go make the code a little prettier.
Test Plan: Viewed Calendar month view tooltips, saw the same values as subheaders.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16880
Summary:
Ref T11816.
- Host gets weird behavior around defaulting to the viewer.
- Invitees get weird behavior around defaulting to the viewer.
- "All Day" is just sort of weird since start / end date aren't customizable.
- Recurring/Frequency are weird here and don't make much sense.
I can't immediately come up with reasons that any of these are particularly useful/valuable to default. More of them can be made editable after T10222 gets sorted out.
Test Plan: Edited edit engine custom forms for Calendar events, saw a more sensible list of customizable fields (e.g., policy stuff).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16878
Summary: Uh, non-all-day-events said 1:30 - 2:29 PM, which is real silly.
Test Plan: Looked at a non-all-day-event.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16874
Summary: Adds a headerimage and lets you set it on posts for added reverence. Is that a word?
Test Plan:
Add an image, see an image.
{F1923010}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16873
Summary:
Fixes T11809. Ref
- Explicitly document the summary icon hints -- I don't think these are too hard to figure out (and maybe this stuff should just go in the tooltips) but we can start here.
- Use color + shape to distinguish between "cancelled" and "declined", not just color (for users with vision accessibility issues).
- Translate a "minute(s)" string into sensible English.
- Use RSVP status on the month view green circle thing.
Test Plan:
- Read docs.
- Looked at month view.
- Read reminder mail.
- Viewed month view mobile view.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16872
Summary:
Ref T11816. Since the dashboard got updated, Differential now interprets "Responsible Users: epriestley" to mean "epriestley, or any project or package epriestley is part of". You can query for just "epriestley" with "exact(epriestley)".
Give Calendar invites the same behavior: "epriestley" means "any event epriestley is invited to, or a project they are a member of is invited to". Individual invites can be queried with "exact(epriestley)".
This is a little bit copy-pastey but I want to wait for a third use case to clean it up since I think I'm going to have to do a bunch of generalization around "how does an individual PHID get turned into a bunch of PHIDs".
Test Plan: Queried for "Invited: dog", "invited: viewer", "invited; exact(dog)", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16870
Summary: No view engine yet (adding header image next), but adds subtitle to display like PhameBlog
Test Plan: Add a subtitle, remove a subtitle.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16871
Summary:
Ref T11816. Projects can be invited to an event, but the UI is currently fairly agnostic about them.
Instead, introduce the idea of "RSVPs", which are basically invites for you as an individual or for any group you're a part of. When we go to check if you're invited, we check for you individually first, then check for any groups you belong to if you haven't already accepted/declined.
On the calendar detail page:
- Show the quick "Join" / "Decline" buttons if any project you're a member of is invited.
- If you're invited, highlight any projects which you're a member of to make that more clear.
On other calendar views:
- If you're invited as part of a project, show the "multiple users" icon.
- If it's just you, continue showing the "add one user" icon.
Test Plan: Viewed month view, day view, detail view. Invited groups and individuals. Invited "Dog Project", accepted invite as user "Dog".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16868
Summary:
Ref T11816. This logic was correct, we just did all the work and then mostly threw away the results. This worked correctly anyway in some timezones.
Instead, actually use `$min_date` and `$max_date`.
Test Plan: In "America/Toronto" with server in a more-western timezone, viewed a "Nov 11 - Nov 12" all-day event, saw those dates.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16867
Summary: Fixes T11867. This should really be on the `CredentialType` itself, but just punt that for now until the API endpoint gets updated. We'll need the actual code here anyway in some form.
Test Plan: {F1922728}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11867
Differential Revision: https://secure.phabricator.com/D16864
Summary: Fixes T11868. This is silly and does not make sense.
Test Plan: Edited a Phurl URL, verified mail only went to me, not to the object itself.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11868
Differential Revision: https://secure.phabricator.com/D16863
Summary:
Ref T9304. This adds a "GuidanceEngine" which can generate "Guidance".
In practice, this lets third-party code (rSERVICES) remove and replace instructions in the UI, which is basically only usefulf or us to tell users to go read the documentation in the Phacility cluster.
The next diff tailors the help on the "Auth Providers" and "Create New User" pages to say "PHACILITY PHACILITY PHACILITY PHACILITY".
Test Plan: Browed to "Auth Providers" and "Create New User" on instanced and non-instanced installs, saw appropriate guidance.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9304
Differential Revision: https://secure.phabricator.com/D16861
Summary: Fixes T11866. This got converted wrong when doing the `/source/` stuff.
Test Plan: Browsed the root directory of a Subversion repository in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11866
Differential Revision: https://secure.phabricator.com/D16860
Summary: See D16851 - there's now a difference in their meaning, so don't unite them in the UI.
Test Plan: Load manage page of repos
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16858
Summary:
Fixes T11818. We don't discard output, so once we read more than 2GB of output we'll exceed the maximum size of a string in an internal buffer.
Instead, configure the future so output is discarded.
Test Plan: Added logging to `libphutil/`, saw internal buffer grow steadily before this change and stay constant at 0 after this change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11818
Differential Revision: https://secure.phabricator.com/D16855
Summary: Fixes T4245. When a repository has a short name, use `/source/shortname/` as its primary URI.
Test Plan:
- Cloned Git repositories from shortnames via HTTP and SSH.
- Cloned Mercurial repositories from shortnames via HTTP and SSH.
- Cloned Subversion repositories from shortnames via SSH.
- Browsed Git, Mercurial and Subversion repositories.
- Added and removed short names to various repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D16851
Summary: Ref T11816. This got dropped somewhere along the way, so the mobile month view no longer showed a green-colored hint if a day has events you're invited to.
Test Plan: Viewed Calendar month view on mobile, saw green circles for days with invited events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16852
Summary: Depends on D16847. Ref T11044. This updates the remaining storage-related workflows from the CLI to accommodate multiple masters.
Test Plan:
- Configured multiple masters.
- Ran all `bin/storage` workflows.
- Ran `arc unit --everything`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16848
Summary:
Depends on D16115. Ref T11044. In the brave new world of multiple masters, we need to check the schemata on each master when looking for missing storage patches, keys, schema changes, etc.
This realigns all the "check out what's up with that schema" calls to work for multiple hosts, and updates the web UI to include a "Server" column and allow you to browse per-server.
This doesn't update `bin/storage`, so it breaks things on its own (and unit tests probably won't pass). I'll update that in the next change.
Test Plan: Configured local environment in cluster mode with multiple masters, saw both hosts' status reported in web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16847
Summary:
Ref T11044. This moves toward partitioned application databases:
- You can define multiple masters.
- Convert all the easily-convertible code to become multi-master aware.
This doesn't convert most of `bin/storage` or "Config > Database (Stuff)" yet, as both are quite involved. They still work for now, but only operate on the first master instead of all masters.
Test Plan: Configured multiple masters, browsed around, ran `bin/storage` commands, ran `bin/storage --host ...`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11044
Differential Revision: https://secure.phabricator.com/D16115
Summary: I moved and then un-moved this incorrectly in D16846.
Test Plan: Looked at the old code, which worked better.
Reviewers: jacksongabbard, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16849
Summary:
Ref T11085. To recreate the issue:
- From the web UI, click "Edit Revision".
- Write something like this as your "Summary" (i.e., put another field marker, like "Test Plan:", into the summary):
> This is a test of the
> Test Plan: field to see
> if it works.
- Save changes.
Later, when the summary is amended into a commit message, the parser will see two "Test Plan:" fields and fail to parse the message.
Instead, prevent users from making this edit.
Test Plan: {F1917640}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11085
Differential Revision: https://secure.phabricator.com/D16846
Summary: Since this was written, `Ennn` became an event monogram and these became real events.
Test Plan: O__O
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16845
Summary:
Ref T11853. My CSS change for the more enormous policy dialog was a little too broad, and affected the "You shall not pass!" dialog too.
Narrow the scope of the CSS rules.
Also add a missing "." that I caught.
Test Plan:
- Looked at policy exception dialogs.
- Looked at policy explanation dialogs.
- Looked at the end of that sentence.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11853
Differential Revision: https://secure.phabricator.com/D16841
Summary:
Fixes T11853. To set this up:
- Create "Project A".
- Join "Project A".
- Create a subproject, "Project A Subproject 1".
- This causes Project A to become a parent project.
- This moves you to be a member of "Project A Subproject 1" instead of "Project A" directly.
- Create another subproject, "Project A Subproject 2".
- Do not join this subproject.
- Set the second subproject's policy to "Visible To: Members of Project A".
- Try to edit the second subproject.
Before this change, this fails:
- When querying projects, we sometime try to skip loading the viewer's membership in ancestor projects as a small optimization.
- Via `PhabricatorExtendedPolicyInterface`, we may then return the parent project to the policy filter for extended checks.
- The PolicyFilter has an optimization: if we're checking an object, and we already have that object, we can just use the object we already have. This is common and useful.
- However, in this case it causes us to reuse an incomplete object (an object without proper membership information). We fail a policy check which we should pass.
Instead, don't skip loading the viewer's membership in ancestor projects.
Test Plan:
- Did all that stuff above.
- Could edit the subproject.
- Ran `arc unit --everything`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11853
Differential Revision: https://secure.phabricator.com/D16840
Summary: Fixes T11839. Both are missing a parameter and one is a copy/paste slop.
Test Plan:
{F1913812}
{F1913813}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11839
Differential Revision: https://secure.phabricator.com/D16837
Summary:
Ref T8510. When users type "platypus" into a typeahead, they want "Platypus Playground" to be a higher-ranked match than "AAA Platypus", even though the latter is alphabetically first.
Specifically, the rule is: results which match the query as a prefix of the result text should rank above results which do not.
I believe we now always get this right on the client side. However, WMF has at least one case (described in T8510) where we do not get it right on the server side, and thus the user sees the wrong result.
The remaining issue is that if "platypus" matches more than 100 results, the result "Platypus Playground" may not appear in the result set at all, beacuse there are 100 copies of "AAA Platypus 1", "AAA Platypus 2", etc., first. So even though the client will apply the correct sort, it doesn't have the result the user wants and can't show it to them.
To fix this, split the server-side query into two phases:
- In the first phase, the "prefix" phase, we find results that **start with** "platypus".
- In the second phase, the "content" phase, we find results that contain "platypus" anywhere.
We skip the "prefix" phase if the user has not typed a query (for example, in the browse view).
Test Plan:
This is a lot of stuff, but the new ranking here puts projects which start with "w" at the top of the list. Lower down the list, you can see some projects which contain "w" but do not appear at the top (like "Serious Work").
{F1913931}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8510
Differential Revision: https://secure.phabricator.com/D16838
Summary:
fixes T11792.
There's no good reason any more to have this option, so just drop it.
Test Plan: Load a file, toggle remaining "blame" button. Load search results page and an image too, which are serviced by the same controller.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T11792
Differential Revision: https://secure.phabricator.com/D16833
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary:
Fixes T11836. See some prior discussion in T8376#120613.
The policy hint in headers in the UI is not exhaustive, and can not reasonably be exhaustive. For example, on a revision, it may say "All Users", but really mean "All users who can see the space this object is in and the repository it belongs to, plus the revision author and reviewers".
These rules are explained if you click (and, often, in the documentation), but "All Users" is still at least somewhat misleading.
I don't think there's any perfect solution here that balances the needs of both new and experienced users perfectly, but this change tries to do a bit better about avoiding cases where we say something very open (like "All Users") when the real policy is not very open.
Specifically, I've made these changes to the header:
- Spaces are now listed in the tag, so it will say `(S3 > All Users)` instead of `(All Users)`. They're already listed in the header, this just makes it more explicit that Spaces are a policy container and part of the view policy.
- Extended policy objects are now listed in the tag, so it will say `(S3 > rARC > All Users)` for a revision in the Arcanist repository which is also in Space 3.
- Objects can now provide a "Policy Codex", which is an object that represents a rulebook of more sophisticated policy descriptions. This codex can replace the tag with something else.
- Imported calendar events now say "Uses Import Policy" instead of, e.g., "All Users".
I've made these changes to the policy dialog:
- Split it into more visually separate sections.
- Added an explicit section for extended policies ("You must also have access to these other objects: ...").
- Broken the object policy rules into a "Special Rules" section (for rules like "you can only see a revision if you can see the repository it is part of") and an "Object Policy" section (for the actual object policy).
- Tried to make it a little more readable?
- The new policy dialogs are great to curl up with in front of a fire with a nice cup of cocoa.
I've made these changes to infrastructure:
- Implementing `PhabricatorPolicyInterface` no longer requires you to implement `describeAutomaticCapability()`.
- Instead, implement `PhabricatorPolicyCodexInterface` and return a `PhabricatorPolicyCodex` object.
- This "codex" is a policy rulebook which can set all the policy icons, labels, colors, rules, etc., to properly explain complex policies.
- Broadly, the old method was usually either not useful (most objects have no special rules) or not powerful enough (objects with special rules often need to do more in order to explain them).
Test Plan:
{F1912860}
{F1912861}
{F1912862}
{F1912863}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T11836
Differential Revision: https://secure.phabricator.com/D16830
Summary: Redesign the action comment box for better use in two column, mobile, nuance.
Test Plan: Test in mobile / desktop / tablet, adding and removing actions. Actionless comment boxes, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: cspeckmim, Korvin
Differential Revision: https://secure.phabricator.com/D16811
Summary: See D16811. I missed this while grepping because the other icon has two aliases (`life-buoy`, `life-ring`) and we were using one of each.
Test Plan: {F1912167}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16829
Summary: Fixes T11834. Actually adding the step wasn't in the `if (...)` block. Also, typo fix.
Test Plan: Saw only one "Explore" on `/guides/`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11834
Differential Revision: https://secure.phabricator.com/D16828
Summary: This isn't spelled as well as it could be.
Test Plan: O_O
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16827
Summary: Ref T5267. I missed these in the variable types conversion.
Test Plan: `arc unit --everything`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267
Differential Revision: https://secure.phabricator.com/D16824
Summary:
Ref T5267. When extrating data from `pht()` calls, also extract the argument types and export them into the map so they can be used by consumers.
We recognize plurals (`phutil_count()`, `new PhutilNumber`) and genders (`phutil_person()`). We'll need to annotate the codebase for those, since they're currently runtime-only.
Test Plan:
Rebuilt extraction maps, got data like this (note "number" type annotation).
```
"Scaling pool \"%s\" up to %s daemon(s).": {
"uses": [
{
"file": "/daemon/PhutilDaemonOverseer.php",
"line": 378
}
],
"types": [
null,
"number"
]
},
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267
Differential Revision: https://secure.phabricator.com/D16823
Summary:
Ref T7643. When you do something like this:
- Edit a task description.
- Click "Show Details" on the resulting transaction.
- Get a prose diff dialog showing the change.
...now add some "Old" and "New" tabs. These are useful for:
- reverting to the old text by copy/pasting;
- reading just the new/old text if the diff is noisy;
- sometimes just nice to have?
(This looks a little rough but I didn't want to put a negative margin on tab groups inside dialogs? Not sure what the best fix here is.)
Test Plan: {F1909390}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16817
Summary:
Ref T11809. I missed this when adding a "Busy" status.
Also the other dot is orange? Just make them all orange for consistency.
Test Plan: Viewed `@username` of busy users (orange), away users (red).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16819
Summary: Ref T7643. When we send mail about a change to a package description, allow it to say "CHANGES TO PACKAGE DESCRIPTION" instead of "EDIT DETAILS". Smooth!
Test Plan: {F1909417}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16818
Summary:
Ref T4788. I thought I implemented this, but actualy didn't.
When we're in the "mid-sized" fallback mode (graph has more than 100 nodes, but not more than than 100 parents/children), don't actually draw the graph. It's almost always uninteresting and huge.
Instead, this just renders a list of direct parents, then the task, then the direct children, which is pretty straightforward.
Test Plan: Set limit to 5, saw mid-sized fallback graph with no actual graph drawing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16816
Summary:
Ref T11801. These are pretty fiddly because users expect to see the end time for timed events ("10 AM - 11 AM" is ONE hour long) but not for all-day events ("Nov 2 - Nov 3" is TWO days long!)
We also want to store the thing the user actually entered so we don't lose data if they un-all-day the event later.
This may take a little more fiddling since it feels a little shaky, but I couldn't break this version immediately.
Test Plan: Imported a French holiday, got proper display in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11801
Differential Revision: https://secure.phabricator.com/D16815
Summary:
Ref T11801. This makes testing/debugging a little easier.
Also fix some inconsistencies with `importAuthorPHID` handling -- it should be the import's author PHID in all cases, so we update imported events properly.
Test Plan: Imported a French holiday with `bin/calendar reload ...`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11801
Differential Revision: https://secure.phabricator.com/D16814
Summary:
Ref T5267. Although translations with very few strings are already put into a "Limited Translations" group, this isn't necessarily clear and was empirically confusing to at least one user, who was surprised that selecting "Spanish" had no UI effect.
Instead, hide limited and test translations entirely unless the install is in developer mode.
Test Plan: In a non-developer-mode install, viewed translations menu. No longer saw translations with very few strings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267
Differential Revision: https://secure.phabricator.com/D16807
Summary: Ref T5267. Fix one minor bug (paths were not being resolved properly) and one minor string issue (missing `%d` in a string).
Test Plan: Extracted strings, got a cleaner result.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267
Differential Revision: https://secure.phabricator.com/D16808
Summary:
Fixes T4788. This change:
- converts the "Task Graph" into a "Related Objects" tabgroup.
- makes "Task Graph" the first tab in the group.
- moves "Mocks" to become a tab.
- adds a new "Mentions" tab, which shows inbound and outbound mentions.
Primary goal of "mocks" is to give us room for a pinboard/thumbnail view after the next Pholio iteration. Might make sense to make it the default tab (if present) at that point, too, since mocks are probably more important than related tasks when they're present.
Primary goal of "mentions" is to provide a bit of general support for various freeform relationships between tasks: if you want to treat tasks as "siblings" or "related" or "following" or whatever, you can at least find them all in one place. I don't plan to formalize any of these weird one-off relationships in the upstream, although it's vaguely possible that some far-future update might just let you define arbitrary custom relationships and then you can do whatever you want.
Test Plan: {F1906974}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4788
Differential Revision: https://secure.phabricator.com/D16806
Summary: Ref T11801. When a file is larger than 512KB, queue it for background import instead of trying to do it in the foreground, sinc we risk hitting `max_execution_time`.
Test Plan: {F1906943}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11801
Differential Revision: https://secure.phabricator.com/D16805
Summary:
Ref T11801. This issue led to the stack trace in T11801#199042.
It wasn't obvious that this was wrong because the recover-on-duplicate-key code made it work correctly.
Test Plan: Imported an event with external attendees with no warnings in the log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11801
Differential Revision: https://secure.phabricator.com/D16804
Summary: Ref T11816. We're running this code on empty events which haven't been initialized and don't have a source attached -- just use a more explanatory check which doesn't need anything attached.
Test Plan: Edited default Calendar policies.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16803
Summary:
Ref T11816.
- Now that we can do something meaningful with them, bring back the yellow dots for "busy".
- Default to "busy" when attending events (we could make this "busy" for short events and "away" for long events or something).
- Let users pick how to display their attending status on the event page.
- Also show which event the user is attending since I had to mess with the cache code anyway. We can get rid of this again if it doesn't feel good.
Test Plan:
{F1904179}
{F1904180}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16802
Summary: Ref T11816. Depends on D16800. Show warnings generated by ICS import in the UI.
Test Plan: {F1904122}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16801
Summary:
When Phortune merchant accounts are created via mechanisms other than the web UI (for example, by Phacility unit tests) this validation check may fail.
Transactions are validated even if no transactions of the given type are being applied, to allow the editor to raise errors like "Name is required!".
If there's no TYPE_INVOICEEMAIL transaction, we'll get called with empty `$xactions` and fail on `strlen($new_email)` because the variable is never defined.
As a secondary issue, if contactInfo, invoiceEmail or invoiceFooter are not provided the record will fail to insert (none of these are nullable).
Test Plan: Ran Phacility unit tests, got a clean result for new instance creation.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16798
Summary:
Fixes T11812.
- Pull the logic for building the "Create Whatever" dropdown out.
- Use it to generate NUX buttons, too.
- Use the new logic in Paste and Maniphest.
Test Plan:
- Viewed Paste NUX, button worked.
- Viewed Maniphest NUX with multiple create forms, button worked.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T11812
Differential Revision: https://secure.phabricator.com/D16797
Summary:
Ref T11809.
- Allow users to remove the "Until" date from recurring events.
- When removing "Until", show a sensible string ("...set this event to repeat forever.")
- When users go through the "Make Recurring" workflow, don't require them to explicitly select "Recurring: Recurring" from the dropdown. This intent is clear from clicking "Make Recurring".
- When editing "All Future Events", don't literally apply date changes to them, since that doesn't make sense. We update the template, then reschedule any events which haven't been edited already. I think this is what users probably mean if they make this edit.
- When creating an event with a non-default icon, don't show "alice changed the icon from Default to Party.".
- Hide the "recurring mode" transaction, which had no string ("alice edited this Event.") and was redundant anyway.
- Also, add a little piece of developer text to make hunting these things down easier.
Test Plan: Edited various events, parents, children, made events recur, set until, unset until, viewed transactions, rescheduled parents, rescheduled children.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16796
Summary:
Ref T11809. Roughly documents most of the tricky/unintuitive stuff.
Also fixes a bug with "Make Recurring" with no "Until" date.
Test Plan: Read document.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16792
Summary:
Ref T11809. Currently, commenting on a recurring event hits the same "one or all?" dialog that other edits do.
For comments and edits submitted via the comment widget, we can safely assume that you mean "just this one", since it doesn't really make sense to try to bulk-edit an event from that UI.
Test Plan: Commented on a recurring event parent and an event in the series.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16795
Summary: Ref T11809. This makes the mail more consistent with Differential and Maniphest, which only include additional details in the first mail in the thread.
Test Plan:
- Created an event with a description.
- First mail included it.
- Followups did not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16794
Summary:
Ref T11809. We show a red dot next to a username to indicate that the user is away (on vacation, in a meeting, etc).
It's not very obvious what this means unless you know that's what it is: when you click the username or view a hovercard, there's no visual hint about what the red dot means. It does say "Away", but there is a lot of information and it doesn't visually connect the two.
Connect the two visually by putting a red dot next to the "Away" bit, too.
Test Plan:
Here's my version of it, this feels OK to me but could maybe be more designed:
{F1893916}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16791
Summary:
Ref T11809. As we move toward unprototyping, this panel is probably more relevant/dynamic/interesting more often than the badges panel, I think?
Particularly, I want to make the red dots a little easier to understand, and I think putting this above the fold will help aid discovery (red dot -> click -> see red dot -> see "away until ..." -> see calendar -> "oh they're at a meeting"?).
This is entirely a product/subjective thing so I'm fine with not doing it or using a different order.
I think there's maybe even an argument for putting this above "Projects", but "Projects" feels more core to me, at least for now.
Test Plan: Viewed a user profile, saw "Calendar" above "Badges".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16790
Summary: Ref T11808. This variable is wrong, and would sometimes cause events to set themsevles as their own parents. They would then fail to load, and disrupt cursor paging.
Test Plan:
- Reproduced T11808 locally by reloading test data 2+ times, creating events with themselves as their own parents.
- Appplied fix.
- Nuked data, reloaded, no more self-parents.
- Test datafile: {F1894017}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11808
Differential Revision: https://secure.phabricator.com/D16793
Summary: Ref T11809. These have been replaced with more flexible storage that accommodates a wider range of behaviors, including those in the ICS format and RRULEs.
Test Plan:
- Ran migration.
- Viewed, created, edited events.
- Grepped for all removed names/symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16789
Summary:
Ref T11809. This came out of Facebook many years ago for computing the number of business days that revisions had been stale.
We removed the little staleness marker a few months ago and haven't seen complaints about it.
If we did holidays now it would make sense to integrate them more directly with Calendar as real events, but I have no plans to pursue this anytime soon. It's easy enough to add the federal holidays manually (~5 minutes of work per year?) if you want them, and they're commentable/editable and you can add local holidays if you're not in the US.
Test Plan:
- Ran `bin/storage upgrade -f`.
- Grepped for `CalendarHoliday`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16788
Summary:
Fixes T11808. I couldn't reproduce the issue there locally so I'm just cheating a little bit until a better reproduction case shows up.
We don't need to do a full load here anyway, and testing for any row is more efficient.
Test Plan: Poked around imports without issues, but couldn't reproduce this problem locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11808
Differential Revision: https://secure.phabricator.com/D16787
Summary:
Fixes T11805. Depends on D16785. This generally tries to smooth out transactions:
- All-day stuff now says "Nov 3" instead of "Nov 3 12:00:00 AM".
- Fewer weird bugs / extra transactions.
- No more silly extra "yeah, you definitely set that event time" transaction on create.
Test Plan: Edited events; changed from all-day to not-all-day and back again, viewed transaction log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11805
Differential Revision: https://secure.phabricator.com/D16786
Summary: Ref T7931. This is still quite rough, but should technically send vaguely-useful email as part of the standard trigger infrastructure.
Test Plan: Ran `bin/phd start`, created an event shortly, saw reminder email send in `bin/mail list-outbound`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7931
Differential Revision: https://secure.phabricator.com/D16784
Summary:
Ref T7931. I'm going to do this separate from existing infrastructure because:
- events start at different times for different users;
- I like the idea of being able to batch stuff (send one email about several upcoming events);
- triggering on ghost/recurring events is a real complicated mess.
This puts a skeleton in place that finds all the events we need to notify about and writes some silly example bodies to stdout, marking that we notified users so they don't get notified again.
Test Plan:
Ran `bin/calendar notify`, got a "great" notification in the command output.
{F1891625}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7931
Differential Revision: https://secure.phabricator.com/D16783
Summary:
Fixes T11804. This probably isn't perfect but seems to work fairly reasonably and not be as much of a weird nonsense mess like the old behavior was.
When a user edits a recurring event, we ask them what they're trying to do. Then we more or less do that.
Test Plan:
- Edited an event in the middle of a series.
- Edited the first event in a series.
- Edited "just this" and "all future" events in various places in a series.
- Edited normal events.
- Cancelled various events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11804
Differential Revision: https://secure.phabricator.com/D16782
Summary:
Ref T11804. This one is messy because we have to fork the //next// event, possibly creating it first.
Then we can edit the parent normally.
Test Plan: Cancelled the first event in a series, only that one cancelled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11804
Differential Revision: https://secure.phabricator.com/D16781
Summary:
When you edit "X and all future events", X becomes the new parent of an event series.
Currently, it loses its relationship to its original parent. Instead, retain that relationship -- it's separate from the normal "parent", but we can use it to make the UI more clear or tweak behaviors later.
This mostly just keeps us from losing/destroying data that we might need/want later.
Test Plan:
- Ran migrations.
- Cancelled "X and all future events", saw sensible-appearing beahvior in the database for "seriesParentPHID".
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16780
Summary: Ref T11804. The field now reads the correct value directly and we don't need this wrapper.
Test Plan: Poked around Calendar without explosions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11804
Differential Revision: https://secure.phabricator.com/D16779
Summary:
Ref T11804. This puts us on a path toward some kind of reasonable behavior here.
Currently, cancelling recurring events makes approximately zero sense ever in any situation.
Instead, give users the choice to cancel just the instance, or all future events. This is similar to Calendar.app. (Google Calendar has a third option, "All Events", which I may implement).
When the user picks something, basically do that.
The particulars of "do that" are messy. We have to split the series into two different series, stop the first series early, then edit the second series. Then we need to update any concrete events that are now part of the second series.
This code will get less junk in the next couple of diffs (I hope?) since I need to make it apply to edits, too, but this was a little easier to get started with.
Test Plan:
Cancelled an instance of an event; cancelled "All future events".
Both of them more or less worked in a reasonble way.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11804
Differential Revision: https://secure.phabricator.com/D16778
Summary:
In ICS, an event on "Nov 1" starts on "2016-11-01" and ends on "2016-11-02".
This is convenient for computers, but this isn't what users expect to enter in date controls. They expect to enter "nov 1" to "Nov 1" for a one-day, all-day event. This is consistent with other applications.
Store the value the user entered, but treat it as the first second of the next day when actually using it if the event is an all day event.
Test Plan:
Mucked around with multi-day all-day events, recurring all-day events, imports, etc. Couldn't catch any weird/unintuitive stuff anymore offhand.
(Previously, entering "Nov 1" to "Nov 2" created a one-day event, which was unclear.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16777
Summary:
This feels a little cleaner:
- Clean up transaction log a bit.
- Use a checkbox instead of a two-option dropdown.
This is a little messy because the browser doesn't send anything if the user submits a form with an un-clicked checkbox.
We now send a dummy value ("Hey, there's definitely a checkbox in this form!") so the server can figure out what to do.
Test Plan:
- Edited all-dayness of an event.
- Viewed transaction log.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16776
Summary:
Ref T11326. If you scheudle a monthly event on the 31st, the default behavior of RRULE means that it only occurs in months with 31 days.
This is actually how Google Calendar and Calendar.app both work: if you schedule a monthly event on the 31st, you get about six events per year.
This seems real confusing and bad to me?
Instead, if the user schedules a monthly event on the 29th, 30th or 31st, pretend they scheduled it on the "last day of the month" or "second-to-last day of the month" or similar, so they always get 12 events per year.
This could be slightly confusing too, but seems way less weird than not getting an event every month.
Test Plan: Scheduled events on the 31st of October, saw them occur in November too after the patch.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16775
Summary:
Ref T11326. Currently, the "Create Event" form is pretty wordy. One particular culprit is the "recurring" controls, which are (presumably) rarely used and visually complex.
- Reflow the default form to hopefully feel a little better.
- Move recurrence stuff to a separate workflow.
Test Plan:
{F1891355}
{F1891356}
{F1891357}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16774
Summary:
This adds the ability for Phabricator's OAuth server implementation to use HTTP basic auth for the client ID and secret and brings it in line with the OAuth 2.0 specification in this respect.
Fixes T11794
Test Plan: Fixes my use case. Shouldn't impact other use-cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: 0, Korvin
Maniphest Tasks: T11794
Differential Revision: https://secure.phabricator.com/D16763
Summary: Ref T11326. Since we were missing an `(int)` cast here, the code ended up thinking that changing `12345` to `"12345"` was an edit. It isn't.
Test Plan: Created/edited events, no more extra "changed start time from X to the same X" transaction clutter.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16773
Summary:
Ref T10747. In the in-email ICS event card that Gmail shows, it has a "Who" field which reads "Unknown Organizer*" if the URI for the organizer isn't email-address-like.
Previously, we used a URI like `https://phabricator.install.com/p/username`, which I think is OK as far as RFC 5545 is concerned, but Gmail doesn't like it.
Instead, use `PHID-USER-asdfa@phabricator.install.com`, which doesn't go anywhere, but makes Gmail happy. Users don't normally see this URI anyway.
Test Plan:
Got a readable "Who" in Gmail when importing an event exported from Calendar:
{F1890571}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16772
Summary: Ref T10747. This turns on the newer EditEngine behavior so we get a nice "X created this event." transaction, instead of an "X renamed this from <nothing> to Event Name."
Test Plan: Imported an event, saw a nice timeline.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16771
Summary: Ref T10747. The transaction version of this copies the "all day" flag over properly, but this non-transaction version needs to copy it explicitly.
Test Plan: Imported an all-day event, saw it come in as all-day.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16770
Summary: Makes a more complete PDF looking invoice form for printing in Phortune.
Test Plan: Make an invoice, click print view, print.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16762
Summary: Fixes T11799. This string is varying on the first parameter, but should vary on the second parameter.
Test Plan: Ran `bin/garbage set-policy ...`, saw proper translation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11799
Differential Revision: https://secure.phabricator.com/D16769
Summary: Ref T10747. If stuff has been deleted on the other calendar, delete it on ours.
Test Plan:
Imported with deletion, saw deletions:
{F1889689}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16768
Summary:
Ref T10747.
- Adds import documentation.
- Adds import/export docs to the help menu.
- Removes some weird/old/out-of-date information from the general user guide, which I'll rewrite later.
Test Plan: Read documentation somewhat thoroughly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16766
Summary:
This fixes the permissions issue with D16750, which is actually not really a permissions issue, exactly.
This is the only place anywhere that we use a tokenizer field //and// give it a default value which is not the same as the object value (when creating a merchant, we default it to the viewer).
In other cases (like Maniphest) we avoid this because you can edit the form to have defaults, which would collide with whatever default we provide. Some disucssion in T10222.
Since we aren't going to let you edit these forms for the forseeable future, this behavior is reasonable here though.
However, it triggered a sort-of-bug related to conflict detection for these fields (see T4768). These fields actually have two values: a hidden "initial" value, and a visible edited value.
When you submit the form, we compute your edit by comparing the edited value to the initial value, then applying adds/removes, instead of just saying "set value equal to new value". This prevents issues when two people edit at the same time and both make changes to the field.
In this case, the initial value was being set to the display value, so the field would say "Value: [(alincoln x)]" but internally have that as the intitial value, too. When you submitted, it would see "you didn't change anything", and thus not add any members.
So the viewer wouldn't actually be added as a member, then the policy check would correctly fail.
Note that there are still some policy issues here (you can remove yourself from a Merchant and lock yourself out) but they fall into the realm of stuff discussed in D16677.
Test Plan: Created a merchant account with D16750 applied.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16764
Summary: This is more consistent with the icon we use for documentation elsewhere.
Test Plan: Looked at the icon, had an easier time guessing it meant "documentation".
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16765
Summary: Converts PhortuneMerchant to EditEngine.
Test Plan: Edits existing merchants fine, same issue as Conpherence when making new ones with permissions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16750
Summary: Is a logo. For merchants.
Test Plan: Set a new logo, remove it. See on list.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D16751
Summary:
Ref T10747. When we import a ".ics" file, represent any attendees as simple external references.
For consistency with other areas of the product, I've avoided disclosing email addresses. We'll try to get a real name if we can.
(We store addresses and could expose or use them later, or do some kind of masking junk like "epr...ley@g...l.com" which is utterly impossible to figure out.)
Test Plan: {F1888367}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16759
Summary:
Ref T11326. Currently:
- The month view and day view (ghosts) don't show that you're invited to a child event.
- The detail view copies the invite list, including attending status, but only //after// it shows the page for the first time.
Instead, for now, just do this:
- Ghosts/stubs use the parent invite list, but treat everyone as "invited".
- Materializing a stub just saves the list as-is (i.e., invited, not a copy of attending/declined/etc).
This behavior may need some refining eventually but is at least reasonable (not obviously bad/buggy).
Test Plan:
- Viewed month/day views, now shown as "invited".
- Viewed detail view, now invitee list shows up properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16758
Summary:
A live instance hit the scenario described in the comment, where an out-of-date user was being selected as the actor.
Since they were no longer an account member, they could not see the payment method and autopay was failing.
Instead, select a relatively arbitrary user who is a current, valid, non-disabled member.
Test Plan: Ran subscriptions with `bin/worker execute ...`, saw it select a valid actor.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16757
Summary:
Depends on D16755. Right now, we build a setup check map (to run preflight checks), then later load libraries.
This means any checks included in third-party libraries don't get added to the map, and no longer run.
(These are rare, but Phacility has a couple).
Instead, delete the caches after loading extra libraries.
Test Plan: With this and D16755, re-ran setup checks and saw Phacility setup checks run.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16756
Summary:
Fixes T11638.
- Fix a regression: I broke this "round to the nearest hour" code a while ago while fiddling with datetimes.
- Improve a beahvior: from the day view, make the menu-bar "Create Event" button default to creating an event on the day you were viewing.
Test Plan: Created events from month and day views, got nice round numbers and proper day suggestions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11638
Differential Revision: https://secure.phabricator.com/D16754
Summary: Fixes T11733. This fixes the issue by working around it, but it isn't useful to set these fields to a default value anyway.
Test Plan: Created a default Calendar form, set some other defaults, created an event, stuff no longer exploded.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11733
Differential Revision: https://secure.phabricator.com/D16753
Summary:
Ref T10747. For URI-based (and, in the future, Google-based) imports, we can automatically refresh them periodically.
(In the general case there's no way to get a push notification for an ICS file, so we just have to do this every-so-often.)
Test Plan:
- Set an ICS file to update hourly.
- Used `bin/trigger fire --id ...` to fire it artificially.
- Saw Calendar update.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16752
Summary: Part of making this look/feel/be more professional is having decent receipts for billing, including contact information (whatever we want to put in there). I'm not using this anywhere at the moment, but will.
Test Plan: Add Contact Info, see Contact Info. Also, why is Remarkup not rendering with line breaks? Seems to be a OneOff thing... anywho... bears!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D14125
Summary:
Ref T10747. RRULE events can repeat "UNTIL" a certain time, or a certain "COUNT" of times.
In the UI, we only support "UNTIL". Also support "COUNT".
Test Plan: Imported an event which repeats every other day, 5 times. Got 5 instances.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16749
Summary: Ref T10747. This makes development/debugging/testing easier and moves us closer to triggered imports (e.g., keep in sync with Google once per day).
Test Plan:
- Reloaded an event import.
- Edited an event in Google Calendar, reloaded, got updated event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16747
Summary:
Ref T11469. This isn't directly related, but has been on my radar for a while: building SSH keyfiles (particular for installs with a lot of keys, like ours) can be fairly slow.
At least one cluster instance is making multiple clone requests per second. While that should probably be rate limited separately, caching this should mitigate the impact of these requests.
This is pretty straightforward to cache since it's exactly the same every time, and only changes when users modify SSH keys (which is rare).
Test Plan:
- Ran `bin/auth-ssh`, saw authfile generate.
- Ran it again, saw it read from cache.
- Changed an SSH key.
- Ran it again, saw it regenerate.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11469
Differential Revision: https://secure.phabricator.com/D16744
Summary: I moved this to setContent with the new search result layout, but failed to update NUX here.
Test Plan: Leave all rooms, get Joinable Rooms with list of 10 rooms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16743
Summary:
Because most threads are private, this query can overheat the policy filter (today, probably only on this install).
Improve the common case by skipping "Visible To: Room Participants" threads if the viewer isn't a participant. This means they don't hit the application and don't count toward overheating the filter.
Test Plan: Viewed Conpherence threads.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16740
Summary:
Fixes T11771. Adds a lock around each GC process so we don't try to, e.g., delete old files on two machines at once just because they're both running trigger daemons.
The other aspects of this daemon (actual triggers; nuance importers) already have separate locks.
Test Plan: Ran `bin/phd debug trigger --trace`, saw daemon acquire locks and collect garbage.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11771
Differential Revision: https://secure.phabricator.com/D16739
Summary: Ref T11766. When users run `git pull` or similar, log the operation in the pull log.
Test Plan: Performed SSH pulls, got a log in the database. Today, this event log is purely diagnostic and has no UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11766
Differential Revision: https://secure.phabricator.com/D16738
Summary: Ref T11773. Not committed to this implementation, but adds some "Developer" query actions to jump to the nux/overheated states without needing to know secret magic URL variables.
Test Plan: {F1878984}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11773
Differential Revision: https://secure.phabricator.com/D16736
Summary: I think maybe these should be more separate from JX.Title, but seems to work ok. May build new favicons just for messages though. Proof of concept UI.
Test Plan: Send message on one browser, see red icon in other browser. Click on menu, count and favicon switch back to normal.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16734
Summary:
See D16734.
- Add ".ico" files to the Celerity map.
- Add a formal route for "/favicon.ico".
- Remove instructions to configure `/rsrc/` and `/favicon.ico` rewrite rules.
Long ago, we served resources directly via `/rsrc/` in at least some cases. As we added more features, this stopped working more and more often (for example, Apache can never serve CSS this way, because it doesn't know how to post-process `{$variables}`).
In modern code (until this change), only `/favicon.ico` is still expected to be served this way.
Instead, serve it with an explicit route via controller (this allows different Sites to have different favicons, for example).
Remove the instructions suggesting the old rewrite rules be configured. It's OK if they're still in place -- they won't break anything, so we don't need to rush to get users to delete them.
We should keep "webroot/favicon.ico" in place for now, since it needs to be there for users with the old rewrite rule.
Test Plan:
- Ran celerity map.
- Loaded `/favicon.ico`, got resource via route.
- Used `celerity_generate_resource_uri()` to get paths to other icons, loaded them, got icons.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16737
Summary: Ref T11773. This is an initial first step toward a more complete solution, but should make the worst case much less bad: prior to this change, the worst case was "30 second exeuction timeout". After this patch, the worst case is "no results + explanatory message", which is strictly better.
Test Plan:
Made all feed stories fail policy checks, loaded home page.
- Before adding overheating: 9,600 queries / 20 seconds
- After adding overheating: 376 queries / 800ms
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11773
Differential Revision: https://secure.phabricator.com/D16735
Summary: Ref T10747. This doesn't have a "keep up to date" option yet, but can, e.g., fetch a Google Calendar URI
Test Plan: Fetched a Google Calendar URI, got some events imported.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16730
Summary:
Ref T10747. Previously, importing a recurring event failed to mark the instnaces of the event as imported.
Now, we copy the source/UID/importer over.
Test Plan: Imported a recurring event, viewed event series, saw all of them marked imported.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16728
Summary:
Ref T10747. When viewing an imported event:
- Make it more clear that it is imported and where it is from.
- Add some explicit "this is imported" help.
Test Plan: Viewed imported and normal events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16727
Summary: Ref T10747. When viewing an import detail page, show a little more information about what you're looking at.
Test Plan: {F1876957}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16726
Summary: Fixes T11764. Moves rendering of the column to client-side, which can skip if it detects we're on mobile.
Test Plan: Open column on desktop, switch to mobile, don't see column. Toggle column on mobile on and off. Switch back to desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11764
Differential Revision: https://secure.phabricator.com/D16725
Summary: Ref T10747. Although I could possibly imagine some very selective cases where we do this eventually, these are read-only for now and not interesting to publish/mail about. The presumption is that the original/authoritative system has already notified relevant parties or they're subscribing passively.
Test Plan: Imported some name changes for events, saw no more mail/feed stuff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16723
Summary:
Ref T10747. When a user drops a ".ics" file or a bunch of ".ics" files into a calendar view, import the events.
(Possibly we should just do this if you drop ".ics" files into any application, but we can look at that later.)
Test Plan: Dropped some .ics files into calendar views, got imports.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16722
Summary:
Ref T10747. When we hit an ICS parser error, render it into a log instead of fataling.
(This will be more important in the future with subscription-based URL ICS import.)
Test Plan: {F1875292}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16721
Summary:
Ref T10747. If you accidentally import the wrong thing, you can clean up the big mess you made.
These imported events are read-only so it's OK to destroy them completely (vs disable/hide/archive).
Test Plan: Destroyed some imported events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16720
Summary:
Ref T10747.
- Look at more than 25 logs!
- Review your favorite logs. Heartwarming! :)
Test Plan: Looked at logs. Wow! Logs!
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16719
Summary: Ref T10747. Don't let users import SECONDLY events, or events outside of the range of a signed 32-bit integer (these are likely not too hard to support, but they're more headaches than we need right now).
Test Plan: Tried to import these no-good problem events, got helpful import errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16716
Summary: There isn't any link back to all your joined rooms when on mobile, add it here.
Test Plan: Pull up mobile, click on menu, see list of threads, click on other room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16717
Summary:
Looks like the logic was there already but some minor parts were missing.
Fixes T8082.
Test Plan:
- Create document `/w/foo`
- Delete document `/w/foo`
- Create document `/w/bar`
- Move document `/w/bar` for `/w/foo`
No error was displayed and document `/w/bar` was moved to `/w/foo`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8082
Differential Revision: https://secure.phabricator.com/D16713
Summary:
Fixes T11748. This option currently implies a line limit (e.g., inline patches that are less than 100 lines long). This breaks down if a diff has a 10MB line, like a huge blob of JSON all on one line.
For now, imply a reasonable byte limit (256 bytes per line).
See T11767 for future work to make this and related options more cohesive.
Test Plan:
- With option at `1000`: sent Differential email, saw patches inlined.
- With option at `10`: sent Differential email, saw patches dropped because of the byte limit.
- `var_dump()`'d the actual limits and used `bin/worker execute --id ...` to sanity check that things were working properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11748
Differential Revision: https://secure.phabricator.com/D16714
Summary: This removes 'full-display', 'minimal-display' from Conpherence, which I recall was because we had 2 UIs for column and regular chat. I'm also tossing in slightly nicer search results, with a link to the actual message and the full date shown for context.
Test Plan: Post a message in mobile, tablet, full conpherence, and in durable column. Clean up UI in durable column. Do a search in Full UI, click on result date, get taken to the message... usually. My test data is a little wonky, but I think this works most of the time.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16710
Summary: Ref T11730. Removes the unused column, seen no issues during past week migrations.
Test Plan: Run migration, check database no longer contains column.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11730
Differential Revision: https://secure.phabricator.com/D16711
Summary: Basically all here, but still probably needs some polish (links to jump? full dates?). Looks much better, still duplicates messages though sometimes. Needs to debug that more.
Test Plan:
Revisit search UI inside Conpherence, outside Conpherence, and normal room searches in Conpherence.
{F1870748}
{F1870749}
{F1870750}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16708
Summary: I passed this in as a config, but need to parse it live when threads change, otherwise the wrong room could be searched.
Test Plan: Search in one room, click a second, search again, see correct results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16707
Summary: Ref T10747. When stuff goes wrong (or right) let the user know what happened.
Test Plan: {F1870139}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16704
Summary: Accept Conduit parameter values as strings (e.g. from `curl`) and convert to required type.
Test Plan:
Call conduit method with int/bool parameter iusing `curl` and make sure it does not result in validation error, e.g.
```
$ curl http://$PHABRICATOR_HOST/api/maniphest.search -d api.token=$CONDUIT_TOKEN -d constraints[modifiedEnd]=$(date +%s) -d constraints[hasParents]=true -d limit=1
```
Fixes T10456.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10456
Differential Revision: https://secure.phabricator.com/D16694
Summary: Converts Owners package transactions to modular transactions.
Test Plan:
- created a new package
- edited all simple properties from the web ui
- checked that project and user owners were added as reviewers appropriately to new diffs
- inspected the change details for various types of path add / remove / update / reorder changes
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16651
Summary:
This search engine ports cleanly to Conduit out of the box.
Ref T11694
Test Plan: called the API method from the console, browsed blueprints in the ui
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T11694
Differential Revision: https://secure.phabricator.com/D16593
Summary: Adds a search bar toggle and results for searching inside a Conpherence Room. The UI of the results itself are not styled yet, and will follow up with another diff.
Test Plan: Go to Conpherence, search for "asdf", get lots of results. Search for nothing, get no change, search for something fictitious, get no threads found (will follow up with search result UI).
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16697
Summary: Ref T10747. Show which events a source imported, and link to the full list as a query result.
Test Plan: {F1870049}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16703
Summary: Ref T10747. This doesn't do much for ICS file imports (you can't disable them since it doesn't do anything meaningful) but will matter more for ICS-subscription imports later.
Test Plan: Clicked "Disable" on an ICS file import, got explanatory dialog.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16702
Summary:
Ref T10747.
- Apply what changes we can with transactions, so you can see how an event has changed and import actions are more explicit.
- I'll hide these from email/feed soon: I want them to appear on the event, but not generate notifications, since that could be especially annoying for automated events.
- When importing, try to update existing events if we can.
Test Plan:
Imported a ".ics" file several times with minor changes, saw them reflected in the UI with transactions.
{F1870027}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16701
Summary: Ref T11730. Removes unused code since this is now it's own page.
Test Plan: rebuild maps, grep for javelin code, classnames
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11730
Differential Revision: https://secure.phabricator.com/D16700
Summary: Ref T10747. This barely works, but can technically import some event data.
Test Plan: Used import flow to import a ".ics" document.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16699
Summary:
Ref T10747. Adds a bunch of stuff so we can keep track of which events we've imported from external sources.
This doesn't do anything yet: you can't actually import anything.
Test Plan:
- Ran `bin/storage upgrade`.
- Clicked "Imports", saw an empty wasteland.
- Created/edited events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16696
Summary: We currently fetch 15 transactions for 5 rooms, which leads to some room subtitles in the notification panel to being blank since nothing was fetched. I don't think this is a great fix, but moves the bar much further. Maybe there is a more accurate fix that isn't 5 SQL queries?
Test Plan: Review notification panel in sandbox, ensure all threads have some additional information.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16695
Summary: Ref T3165. Builds an ngram table for Conpherence Room titles, allowing a tokenizer for searching a subset of rooms.
Test Plan: Say `Gabbert` in two different rooms, search all, see two rooms returned. Search specific room, see specific result.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D16692
Summary: Background is now always white, spacing in header is more consistent
Test Plan: test mobile, table, desktop application search apps.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16691
Summary:
Ref T11706. Add some casts so we don't return `"0"` for `false`.
Also I forgot to document one of the things.
Test Plan: Called `calendar.event.search`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11706
Differential Revision: https://secure.phabricator.com/D16690
Summary:
`DrydockAuthorizationSearchEngine` was being used solely to display authorizations for a specific blueprint from the web UI and consequently expected that callers set a specific blueprint before performing a query. Here we check to see if a blueprint has been set in cases where the engine could be operating from either Conduit or the web.
Ref T11694
Test Plan:
- called the API method from the console
- approved an authorization
- followed the "view all" link from a blueprint page
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T11694
Differential Revision: https://secure.phabricator.com/D16592
Summary: We have more space here for last 8.
Test Plan: Reload, see 8.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16689
Summary: Fixes T11706. I think this approach (roughly: provide the information in a few different formats) is generally reasonable, and should let clients choose how much date/time magic they want to do.
Test Plan: Called `calenadar.event.search`, viewed results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11706
Differential Revision: https://secure.phabricator.com/D16688
Summary:
Fixes T11746. The opcache docs are on a different page, so point there if we're raising opcache issues.
(It's possible for a setup issue to say "configure X, or configure Y", where X is opcache and Y is non-opcache, so we may want to render both links.)
Test Plan: {F1867109}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11746
Differential Revision: https://secure.phabricator.com/D16685
Summary: Fixes T11745. I just missed this while juggling some of the internal storage.
Test Plan: Created a new event with recurrence behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11745
Differential Revision: https://secure.phabricator.com/D16684
Summary:
Ref T10747. This adds disable/enable to exports.
Mostly useful if you leak a URI by accident.
Test Plan:
- Disabled and enabled exports.
- Verified that disabled exports don't actually export any data.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16681
Summary:
Ref T10747. This explains how exports work.
Also make mail exports use the same logic as other stuff.
Test Plan: Read documentation. Did some exports.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16680
Summary:
Ref T10747. This:
- Exports recurring events properly, with RRULE + RECURRENCE-ID.
- When exporting a part of an event series, export the whole series to ICS so it is represented faithfully.
- Make the subscribable URL for "Export" objects work.
Test Plan:
- Downloaded the ".ics" for a normal event, imported it into Calendar.app and Google Calendar.
- Downloaded the ".ics" for a recurring event, imported it into Calendar.app and Google Calendar.
- Defined an ".ics" Export of my events, subscribed to them in Calendar.app.
- Edited an event in Phabricator.
- Hit {key Command R} in Calendar.app, saw changes. (MAGIC!)
- This export included recurring events, which appeared the same way in Calendar.app and Phabricator.
- Can't import into Google Calendar from my local install easily since Google's servers can't hit my laptop, but I'll test once we deploy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16679
Summary:
Ref T10747.
- Adds a "Use Results..." dropdown to query result pages, with actions you can take with search results (today: create export; in future: bulk edit, export as excel, make dashboard panel, etc).
- Allows you to create an export against a query key.
- I'm just using a text edit field for this for now.
- Fleshes out export modes. I plan to support: public (as though you were logged out), privileged (as though you were logged in) and availability (event times, but not details).
This does not actually export stuff yet.
Test Plan: Created some exports. Viewed and listed exports.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16676
Summary:
Ref T10747. Rough flow is:
- Run a query.
- Select a new "Export Events..." action.
- This lets you define an "Export", which has a unique URL you can paste into Google Calendar or Calendar.app or whatever.
Most of this does nothing yet but here's the boilerplate.
Test Plan: Doesn't do anything yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16675
Summary:
Ref T11326. This reorders sections:
- Description (if present)
- Recurring event series info (if recurring)
- Invitees (this also has custom stuff, if it exists)
Test Plan: Viewed some events, saw more sensible order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11326
Differential Revision: https://secure.phabricator.com/D16671
Summary:
Ref T10747.
- Store recurrence as RRULEs internally.
- Use RRULE constants.
- Migrate existing rules to RRULEs.
Test Plan: Ran migration, nothing seemed broken?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16670
Summary: Ref T10747. This drives event queries through RRULE, too.
Test Plan: Created recurring events, saw them appear correctly on the calendar.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16668
Summary:
Ref T10737. Today, we evalute recurrence twice: once when querying, and once in all other cases. This converts the second case to use the RRULE engine.
Next up is making the query use the RRULE engine, too.
Test Plan: Created a new recurring event, iterated through it by clicking "next instance", viewed it on Calendar view.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10737
Differential Revision: https://secure.phabricator.com/D16667
Summary:
Ref T10747. This deprecates "dateFrom", "dateTo", "allDayDateFrom", "allDayDateTo", and "recurrenceEndDate".
They are replaced with "utc*Epoch" fields (for querying) and CalendarDateTime objects (for start, end, until). These objects can represent the full range of dates and times expressible in ICS format, allowing us to import a wider range of ICS events.
Test Plan:
Ran migrations, viewed/edited Calendar, didn't catch anything catastrophcially broken.
This likely needs some followups, I'll keep it local for a bit until I'm confident I didn't break anything too catastrophically. I'm retaining the old data for now so we can likely fix things if it turns out there is some sort of issue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16664
Summary: Ref T10747. Moves away from getDateFrom() / getDateTo() and makes a few more date/time methods more consistent.
Test Plan: Created, edited, viewed events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16663
Summary: Ref T10747. The CalendarDateTime object now carries the viewer timezone as part of its state, so we don't need to have separate accessors.
Test Plan:
- Viewed events, checked that crumbs render properly.
- Edited events.
- Created new events.
- Viewed calendar.
- Viewed event detail pages.
- Viewed profile mini-calendar.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16662
Summary:
Ref T10747. This does double-writes and starts generating/writing CalendarDateTimes.
This greater flexibility is necessary to support the full range of ICS-specifiable events, including "floating" events.
This doesn't do anything yet.
Test Plan: Created and edited events, verified sensible representations of corresponding datetimes appeared in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16661
Summary:
Ref T10747. Currently, Calendar events are mostly epoch-based and cheat a little bit for all-day events.
This already felt a little flimsy, and can't reasonably accommodate the full range of `.ics` events, which include "floating" events (e.g., occurs at 3PM regardless of timezone, like "Tea Time").
As a secondary issue, we identify instances of a recurring event by instance number (1, 2, 3, etc.). This can't accommodate the full range of `.ics` events, which include arbitrary additional "RDATE" events (e.g., recurrs every week, and also on these specific extra days).
However, we do need to store some epoch information so we can do query windowing: when the user looks at "October 2016", we want to select the smallest number of events that we can from the database initially, before refining them down to generate instances. We can't reasonably query the actual dates no matter how we store them because this depends on computing things like UNTIL, COUNT, initial dates, whether events are recurring or not, timezones, etc.
Instead, when we save an event compute the earliest second it occurs on in UTC and the latest second it occurs on in UTC. We can then query for a small superset of possible events in "October 2016" for any viewer pretty easily.
Also, start laying the groundwork for using fewer epochs in the rest of the code, and for reducing the role of sequence indexes (I plan to keep some sequences indexes around, probably, since they're nice in the UI, but not all child events will have indexes since there's no index for an RDATE event).
This doesn't migrate existing events yet or actually read these new columns -- that will come later once the new code is a little more solid.
Test Plan:
- Ran `bin/storage upgrade`.
- Created a new event.
- Saved an existing event.
- Viewed database, saw sensible-looking "UTC Epoch" values.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16652
Summary: This moves room pictures out of the dialog and into it's own PictureController. Also adds a standard image (and removes the "last person to chat" picture (though we could add that back. My plan is though that direct messages use auto use the other person's photo, after we have editengine and room pictures will have a plain, replaceable image.
Test Plan: Set a new room picture, remove a picture. Run migration, see old images properly set with new image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11730
Differential Revision: https://secure.phabricator.com/D16669
Summary: Ref T11730. Removes the front end crop feature. Will follow up with proper removal, but this seems broken outright.
Test Plan:
Edit a room, don't seen "Crop" feature. Upload new photo, works fine.
- grep for `ConpherencePicCropControl`
- grep for `aphront-crop`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11730
Differential Revision: https://secure.phabricator.com/D16665
Summary: This adds the room image to the main header in full Conpherence. It's nice, plus I plan to move the image edit workflow to it to simplify the move to EditEngine. I plan to build some default images for Conpherence which should be better about denoting the room, not just the last person writing.
Test Plan: Click on lots of rooms with and without topics.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16666
Summary: Not sure this ever worked correctly, but now once we have a supported action, skip the rest of the transactions. Currently you'll see a random old post.
Test Plan: Test multiple rooms in various states with new messages, edits, new room titles, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16660
Summary: Unclear these are worth sending, but mostly seems useful. Returns `getTitle` for the transaction if it's not a message. Fixes T10683
Test Plan: Leave rooms, change names, add pictures.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10683
Differential Revision: https://secure.phabricator.com/D16658
Summary: Provide higher resolution for Conpherence room images. Fixes T11728
Test Plan:
Upload a new photo, see it pulls in 200px image as background.
{F1858660}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11728
Differential Revision: https://secure.phabricator.com/D16659
Summary: Depending on when packages loaded, this CSS sometimes gets overwritten. Make it more specific and always present.
Test Plan: Reload a lot
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16657
Summary: Remove policy icons from durable column, create a basic nux layout and style.
Test Plan: leave all rooms, pop open chat, see helpful text and button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16655
Summary:
Fix typo 'Branches' in the panel header for the Diffusion Actions
management panel.
Test Plan: Saw 'Actions' in the panel heading
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16654
Summary: More work to do here on the JS side, but this at least makes sure users with a small chat window have some notification marked that new replies have not been seen.
Test Plan: Open two windows. Window 1 has durable minimized, Window 2 is full conpherence. Send a message from Window 2, see header count in Window 1 increase. Repeat with durable open, see no change in window.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16650
Summary: A bit better styling, this adds an indication icon for if you're connected or not (and later, away, etc).
Test Plan: Test in Notifications menu, Conpherence full, Durable Column.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16647
Summary: Sends and stores additional body classes at the page level. Removes old ones, sets new ones.
Test Plan: home -> application search -> colored workboard -> config -> home with persistent chat open and minimized.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16646
Summary: This exposes the chat window to a larger audience beside people who accidentaly hit `\`.
Test Plan:
Lots of clicks and reloads.
{F1856043}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16643
Summary: Creates a background that renders inside the Quicksand frame, through sorcery.
Test Plan: Turn on Quicksand, visit lots of pages. See correct background colors. This probably blows something up I'm not testing.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16642
Summary: I missed removing this during the file purge of '16. Fixes T11717
Test Plan: Will test live
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11717
Differential Revision: https://secure.phabricator.com/D16639
Summary:
Since I plan to add collapsing, this widens the chat window and moves the switcher to the side, for more visual space for conversation.
TODO: make a magical minimizer so I can always have it open.
Test Plan:
Tested on my large display and little Macbook.
{F1854092}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16635
Summary: Fixes T11712. This is somewhat misleading with encryption enabled.
Test Plan: Viewed chunked and unchunked files.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11712
Differential Revision: https://secure.phabricator.com/D16636
Summary: Adds a CSS class if comments come in from the same user in the past 2 minutes for cleaner UI. Note will have to find some better display UI when comment editing comes.
Test Plan: Test lots of random Conpherence messages with different transactions, different people, and quick commenting.
Reviewers: scp, epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16632
Summary: This feels pretty reasonable with little effort, and I think I'd use it more than the full column.
Test Plan:
Chat a lot on various pages.... still some quicksand quirks around various pages.
{F1853487}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: scp, Korvin
Differential Revision: https://secure.phabricator.com/D16627
Summary: Fixes T11622. Moves the remarkup upload button into the text area on mobile/tablet.
Test Plan: Mobile/Tablet/Desktop Conpherence
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11622
Differential Revision: https://secure.phabricator.com/D16626
Summary: Adds a connection status message in Conpherence
Test Plan: Check status
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16625
Summary:
Fixes T11705. I did not realize that `ON DUPLICATE KEY UPDATE` was order-dependent, so the "reset" clause of this `IF(...)` never actually worked.
Reorder it so we check if we're changing the message type //first//, then actually change the message type.
This makes the count reset properly when a failing repository succeeds, or a working repository fails.
Test Plan:
- On `master`, forced a working repository to fail a `bin/repository update`, saw the message change types (expected) but keep the old count (wrong!).
- With this patch, repeated the process and saw the count reset properly.
- Ran the patch, verified counts reset to 0.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11705
Differential Revision: https://secure.phabricator.com/D16623
Summary:
Looking at IPs who recently registered more than one account in
Phabricator and trying to figure out whether they are spam bots
or just all on the same university network, I often want to check
recent user activity of these accounts. Hence linking the entries
in the User column to their user page comes in handy.
Test Plan: Tested on local instance and works as expected.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16620
Summary: Ref T11217. This just adds the table that we'll store tokens in. It doesn't make use of the table at all yet. This is mostly pulled from this diff (D16178). Specifically I mostly followed Evan's instructions related to the token table here: D16178#189120.
Test Plan: I ran `./bin/storage upgrade` successfully and there were no schema errors.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T11217
Differential Revision: https://secure.phabricator.com/D16621
Summary: Fixes T11623. Enables send-on-enter and shift-enter for linebreaks, per durable column. Also cleaned up UI for Joining Room or Logging In.
Test Plan: See room I can join, click Join Room. Leave Room, Log out, visit room with login prompt. Login, Join Room again.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11623
Differential Revision: https://secure.phabricator.com/D16595
Summary: Fixes T10715. Badges on the profile view now link to the badge view
Test Plan: Went to the profile view and clicked the link.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T10715
Differential Revision: https://secure.phabricator.com/D16604
Summary: Fixes T9063. Removes the "Application" field from the search because it was largely redundant with the 'Name Contains' field.
Test Plan: Went to `/conduit/query/modern/`, clicked on `Edit Query` and noted that there is no "Application" field anymore. The 'Name Contains' field still works however.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T9063
Differential Revision: https://secure.phabricator.com/D16602
Summary: Fixes T10681. Adds a search API endpoint and an edit API endpoint for Phurl URLs. I still need to add the ability to search by name, alias, URL, and maybe description.
Test Plan: Test the methods through `/conduit/method/phurls.search/` and `/conduit/method/phurls.edit/`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T10681
Differential Revision: https://secure.phabricator.com/D16600
Summary:
Ref T4190. Added the remarkup rule to embed images:
Syntax is as follows:
`{image <IMAGE_URL>}`
Parameters are also supported, like:
`{image uri=<IMAGE_URI>, width=500px, height=200px, alt=picture of a moose, href=google.com}`
URLs without a protocol are not supported.
Test Plan: Tested with many of the syntax variations. If the provided URL doesn't point to an image, then a broken image icon will be shown.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T4190
Differential Revision: https://secure.phabricator.com/D16597
Summary: Somehow this got through last week :( It's a bug that causes the controller to... *ahem*... just not work. Luckily nothing uses this yet so nothing was really affected.
Test Plan: Hit `/file/imageproxy/?uri=http://i.imgur.com/nTvVrYN.jpg` and are served a nice picture of a bird
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Differential Revision: https://secure.phabricator.com/D16598
Summary:
Ref T11672. At low loads, this causes us to use more connections, which is pushing some installs over the default limits.
Rather than trying to walk users through changing `max_connections`, `open_files_limit`, `fs.file-max`, `ulimit`, etc., just put things back for now. After T11044 we should have headroom to use persistent connections within the default limits on all reasonable systems..
Test Plan: Loaded Phabricator, poked around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11672
Differential Revision: https://secure.phabricator.com/D16591
Summary:
Ref T4190. Currently only have the endpoint and controller working. I added caching so subsequent attempts to proxy the same image should result in the same redirect URL. Still need to:
- Write a remarkup rule that uses the endpoint
Test Plan: Hit /file/imageproxy/?uri=http://i.imgur.com/nTvVrYN.jpg and are served the picture
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T4190
Differential Revision: https://secure.phabricator.com/D16581
Summary: Ref T11687. Subscription to Blogs comes with many additional features, don't lock people in.
Test Plan: Saw I was no longer subscribed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11687
Differential Revision: https://secure.phabricator.com/D16589
Summary:
Fixes T11679. This application is probably vanishing into the aether eventually, but stop it from fataling for now.
Here's the glyph: ▛
It's like a fragment of a block of file data! Right? Obviously.
Test Plan: Visited `/phragment/` with glpyhs on, saw the glyph.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey, hach-que
Maniphest Tasks: T11679
Differential Revision: https://secure.phabricator.com/D16588
Summary: Fixes T11685. We missed this one straggler the recent conversion of Phurl to EditEngine, in T10673.
Test Plan: Visited `/phurl/?nux=1`, clicked "Shorten a URL".
Reviewers: chad, jcox
Reviewed By: jcox
Maniphest Tasks: T11685
Differential Revision: https://secure.phabricator.com/D16587
Summary:
Fixes T11683. Likely as a result of the persitent connections change, more users are seeing MySQL connection limit errors.
The persistent connections change means we use //fewer// connections at the high end, but I'm guessing PHP is keeping some more connections around in the pool, so while high-traffic hosts use fewer connections, low-traffic hosts now use more.
Raise an explicit setup warning about this. Users should be adjusting it anyway, there's no value to leaving it at extremely low default and connections are baiscally free until you run out of outbound ports.
Test Plan: {F1844630}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11683
Differential Revision: https://secure.phabricator.com/D16586
Summary: Fixes T11676. Instead of trying to fit task titles to the display, truncate them and let the table scroll.
Test Plan:
Table now scrolls when cramped:
{F1843396}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11676
Differential Revision: https://secure.phabricator.com/D16583
Summary:
Fixes T11677. This makes two minor adjustments to the repository import daemons:
- The first step ("Message") now queues at a slightly-lower-than-default (for already-imported repositories) or very-low (for newly importing repositories) priority level.
- The other steps now queue at "default" priority level. This is actually what they already did, but without this change their behavior would be to inherit the priority level of their parents.
This has two effects:
- When adding new repositories to an existing install, they shouldn't block other things from happening anymore.
- The daemons will tend to start one commit and run through all of its steps before starting another commit. This makes progress through the queue more even and predictable.
- Before, they did ALL the message tasks, then ALL the change tasks, etc. This works fine but is confusing/uneven/less-predictable because each type of task takes a different amount of time.
Test Plan:
- Added a new repository.
- Saw all of its "message" steps queue at priority 4000.
- Saw followups queue at priority 2000.
- Saw progress generally "finish what you started" -- go through the queue one commit at a time, instead of one type of task at a time.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11677
Differential Revision: https://secure.phabricator.com/D16585
Summary:
Fixes T11675. This capability was erroneously (probably?) removed in D14766.
This search implementation (which uses exact match) probably isn't perfect for all cases of "text" fields, but empirically it seems to be what a significant number of users are after.
Test Plan:
Searched for a custom text field value.
{F1843383}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11675
Differential Revision: https://secure.phabricator.com/D16582
Summary:
Ref T11672. Depends on D16577. When establishing a connection from a webserver context, try to use persistent connections.
The hope is that this will fix outbound port exhaustion issues experienced on repository hosts handling large queue volumes.
Test Plan:
Added this to a page:
```lang=php
$tables = array(
new PhabricatorUser(),
new ManiphestTask(),
new DifferentialRevision(),
new PhabricatorRepository(),
new PhabricatorPaste(),
);
$ids = array();
foreach ($tables as $table) {
$conn = $table->establishConnection('r');
$cid = queryfx_one(
$conn,
'SELECT CONNECTION_ID() cid');
$ids[get_class($table)] = $cid['cid'];
}
var_dump($ids);
```
Reloaded the page a bunch of times and saw no reissued connections (the pool seems to keep a particular connection bound to a particular database), but did see connection reuse across requests.
That is, across reloads the same connection IDs appeared, but the same connection ID never appeared twice in the same request. This is what we want.
Also googled for issues with persistent connections, but everything I found was unconcerning and obscure (local variables and other very complex state that we don't use), and a bunch of the docs are reassuring (transactions, etc., get reset properly).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11672
Differential Revision: https://secure.phabricator.com/D16578
Summary: Fixes T10673. Set up Phurl to use Edit Engine. There's no way this is all I needed to do to get it working, so I'll be making another pass at it and testing more thoroughly...
Test Plan: Ran through the Phurl URL creation/edit/deletion process.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T10673
Differential Revision: https://secure.phabricator.com/D16573
Ref T11665.
Without `-n 1`, this logs the ENTIRE history of the repository. We
actually get the right result, but this is egregiously slow. Add `-n 1`
to return only one result.
It appears that I wrote this wrong way back in 2011, in D953. This
query is rarely used (until recently) which is likely why it has
escaped notice for so long.
Test Plan: Used Conduit console to execute `diffusion.rawdiffquery`.
Got the same results but spent 8ms instead of 200ms executing this
command, in a very small repository.
Summary:
The commit which added checks for the old homepage options (now in
Dashboard) in rP9d9a47e9cf, added them to the auth section, where they
would present:
This option has been migrated to the "Auth" application. Your old
configuration is still in effect, but now stored in "Auth" instead of
configuration. Going forward, you can manage authentication from the
web UI.
Remove them from the moved-to-Auth list, and coalesce the multiple
definitions of the help text into one.
Test Plan:
- set maniphest.priorities.unbreak-now to something
- observe the setup issue reported
- hope it tells you the right thing
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, chad
Differential Revision: https://secure.phabricator.com/D16576
Summary:
Ref T11665. Currently, when a repository hits an error, we retry it after 15s. This is correct if the error was temporary/transient/config-related (e.g., bad network or administrator setting up credentials) but not so great if the error is long-lasting (completely bad authentication, invalid URI, etc), as it can pile up to a meaningful amount of unnecessary load over time.
Instead, record how many times in a row we've hit an error and adjust backoff behavior: first error is 15s, then 30s, 45s, etc.
Additionally, when computing the backoff for an empty repository, use the repository creation time as though it was the most recent commit. This is a good proxy which gives us reasonable backoff behavior.
This required removing the `CODE_WORKING` messages, since they would have reset the error count. We could restore them (as a different type of message), but I think they aren't particularly useful since cloning usually doesn't take too long and there's more status information avilable now than there was when this stuff was written.
Test Plan:
- Ran `bin/phd debug pull`.
- Saw sensible, increasing backoffs selected for repositories with errors.
- Saw sensible backoffs selected for empty repositories.
Reviewers: chad
Maniphest Tasks: T11665
Differential Revision: https://secure.phabricator.com/D16575
Summary:
Ref T11665. Fixes T7865. When we restart the daemons, the repository pull daemon currently resets the cooldowns on all of its pulls. This can generate a burst of initial load when restarting a lot of instance daemons (as in the Phacility cluster), described in T7865. This smooths things out so that recent pulls are considered, and any repositories which were waiting keep waiting.
Somewhat counterintuitively, hosted repositories write `TYPE_FETCH` status messages, so this should work equally well for hosted and observed repositories.
This also paves the way for better backoff behavior on repository errors, described in T11665. The error backoff now uses the same logic that the standard backoff does. The next change will make backoff computation consider recent errors.
(This is technically too large for repositories which have encountered one error and have a low commit rate, but I'll fix that in the following change; this is just a checkpoint on the way there.)
Test Plan: Ran `bin/phd debug pull`, saw the daemon compute reasonable windows based on previous pull activity.
Reviewers: chad
Maniphest Tasks: T7865, T11665
Differential Revision: https://secure.phabricator.com/D16574
Summary: Ref T8628.
Test Plan: Performed an action that uses the redirect controller (trying to visit a repo page while not logged in). Logged in and was redirected as expected
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D16571
Summary: Ref T8628
Test Plan: Updated DarkConsoleDataController and observed that the darkconsole still works as expected
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D16570
Summary: Fixes T11642. Added a 'name' field to the results from harbormaster.build.search.
Test Plan: Went to `/conduit/method/harbormaster.build.search/` and ran a search that would yield results (because otherwise there will be nothing there). Noted that there was, in fact, a name in the results.
Reviewers: yelirekim, #blessed_reviewers, epriestley
Reviewed By: yelirekim, #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T11642
Differential Revision: https://secure.phabricator.com/D16569
Summary: I believe these are left over from widgets, when we added a "Files" widget that kept track of everything added to the window.
Test Plan: Added files to a Conpherece, Set an image when editing. Anything else?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16567
Summary: Roughly, if user isn't in any rooms, search for joinable ones. If no results, show big NUX banner.
Test Plan: Left all rooms, got fallback, joined room, left room. Create new instance, see new NUX. Set instance to public, visit Conpherence with and without public rooms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16563
Summary: [Draft] Posting this up because feed is pulling `getTitle` and not `getTitleForFeed` and I'm super confused. Restarted phd and apache.
Test Plan: Create a new room, see link in feed. Change topic, see story, add people, don't see story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11645
Differential Revision: https://secure.phabricator.com/D16561
Summary:
Ref T11650. Currently, we load packages and then discard the archived ones.
However, this gets "dominion" rules (where a more-general package gives up ownership if a more-specific package exists) wrong if the more-specific package is archived: we incorrectly give up ownership.
Instead, just ignore these packages completely when loading affected packages. This is slightly simpler.
(There are technically two pieces of code we have to do this for, which should be a single piece of code but which haven't yet been unified.)
Test Plan:
- Created packages:
- Package A, on "/" (strong dominion, autoreview).
- Package B, on "/x/" (weak dominion, autoreview).
- Package C, on "/x/y" (archived, autoreview).
- Create a revision affecting "/x/y".
- Saw correct path ownership in table of contents ("B", strongest package only).
- Saw correct autoreview behavior (A + B).
- (Prior to patch, in `master`, reproduced the problem behaviors described in T11650, with bad dominion rules and failure to autoreview B.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11650
Differential Revision: https://secure.phabricator.com/D16564
Summary: We no longer have widgets, don't need these calls for simplicity.
Test Plan: Add a user to a room, remove a user from a room. Grep for `needWidgetData`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16562
Summary: Mostly quality of life in renames and moving everything to the view class. Minor CSS tweaks. Fix room handles getting added when adding a new user.
Test Plan: Add to room, remove from room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16560
Summary:
Ref T10747.
- Remove the warning dialog since these files don't seem to do anything confusing/problematic in Calendar.app or Google Calendar. Those importers generally need to be defensive about how they handle random ".ics" files from arbitrary third parties anyway, and this makes testing imports easier since we have a GET-table ".ics" URI for public events.
- Attach ".ics" files to email.
Test Plan:
- Clicked "Export as .ics", got an ICS file.
- Used "bin/mail show-outbound" to review an ICS attachment, although I don't actually have real mail set up locally so this may still be a little funky.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16559
Summary: Ref T10747. This exports these sections when generating an ".ics" file.
Test Plan: {F1832214}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16558
Summary: Darkens the topic, adds fun colors to action icons.
Test Plan: Hover over new action icons. See darker topic text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16556
Summary: Ref T10747. Allows you to grab an event as a (basic) ICS file.
Test Plan:
- Exported a normal event.
- Exported an all-day event.
{F1830577}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10747
Differential Revision: https://secure.phabricator.com/D16553
Summary:
Minor rebuild / redesign of Conpherence. Most of this is new UX and tossing out things like widgets, device fallbacks. I expect some of the UI to get more polished after next pass, but most everything here is in place.
- Removed "Widgets", now just a single Participants pane
- Added "Topic"
- New header
- Settings, Edit are action icons
- Removed a lot of JS
- Simplified CSS as much as I could
Test Plan:
Desktop, Tablet, Mobile. Adding and removing people. Setting new topics, new rooms.
{F1828662}
{F1828669}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16550
Summary: These floats were wonky in FF, adding some hardening to make sure they clear.
Test Plan:
Firefox, Chrome, IE.
{F1815981}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16547
Summary: We currently try to build crumbs out even if the user has no membership in any. This removes these useless controls.
Test Plan: Leave all Rooms, see NUX state with no "Edit Room", blank title, or "Collapse Column" controls.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16546
Summary:
Fixes T11586. First pass at a class for displaying invisible characters. Still need to:
- Write a couple unit tests
- Add some styling to the .invisible-special spans
- Actually start using the class when displaying form errors to users
Currently this makes the string `"\nab\x00c\x01d\te\nf"` look like:
{F1812711}
Test Plan:
Unit tests all pass and run in <1ms:
{F1812998}
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T11586
Differential Revision: https://secure.phabricator.com/D16541
Summary: This adds a "column" icon into crumbs, like in workboards, for expanding or hiding the "Widget Pane". This is per user sticky and defaults to off.
Test Plan: View a Conpherence Room, see no widgets by default. Toggle it on, see widget. Reload page, see widget stick. Verify mobile, tablets ignore hiding.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10364
Differential Revision: https://secure.phabricator.com/D16533
Summary:
Moves search and new room out of crumb bar and into the thread column. This opens up space to add "Edit Room" into the crumbs area as an action link. Also removed 'widget-edit' pathways and javascript.
Fixes T8972
Test Plan:
Search, New, Edit rooms. Click different rooms, see correct URL each time.
{F1813226}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8972
Differential Revision: https://secure.phabricator.com/D16543
Summary:
Fixes T11627.
Beyond being complex, I have no real reason to believe these checks even work (and they don't test repositories, file storage, logfiles, etc).
Test Plan:
Faked the error:
{F1813433}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11627
Differential Revision: https://secure.phabricator.com/D16544
Summary:
Ref T11613. In D16503/T11598 I refined the setup flow to improve messaging for early-stage setup issues, but failed to fully untangle things.
We sometimes still try to access a cache which uses configuration before we build configuration, which causes an error.
Instead, store "are we in flight / has setup ever worked?" in a separate cache which doesn't use the cache namespace. This stops us from trying to read config before building config.
Test Plan:
Hit bad extension error with a fake extension, got a proper setup help page:
{F1812803}
Solved the error, reloaded, broke things again, got a "friendly" page:
{F1812805}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11613
Differential Revision: https://secure.phabricator.com/D16542
Summary:
* All Applications are useing the description "Tags", Phriction actually uses "Projects"
* Changed "Projects" to "Tags" for phriction
Ref T11614
Test Plan:
* Edited a document, took a look at the description for that input line
* Added a project to a page and saved it
Reviewers: chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: avivey, chad, epriestley
Tags: #phriction
Maniphest Tasks: T11614
Differential Revision: https://secure.phabricator.com/D16537
Summary: Fixes T10155
Test Plan: View an empty repository in diffusion, check for the exception.
See T10155 for steps to reproduce
Reviewers: epriestley
Subscribers:
Summary: Cleans up a number of inconsistencies, makes Conpherence lighter, easier to read, make names larger, darker.
Test Plan: Review Conpherence chat in Main App, Durable Column, Mobile, Tablet
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16531
Summary: Just changes the default here. Main reasoning is if I'm searching for a room, show me all of them first. "My Rooms" is where I just came from.
Test Plan: Click Search in Conpherence, see All Rooms as default.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16527
Summary: Fixes T10781. Re-uses sidenav styles from global space, though a little hacky. Fix crumb border, fix nux experience. Fiddle spacing.
Test Plan:
Use Conpherence as new user, make a room, post some messages.
{F1805658}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10781
Differential Revision: https://secure.phabricator.com/D16528
Summary: Fixes a typo in the Almanac docs.
Test Plan: Didn't test this one.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16529
Summary: Fixes T10131. Adds new CSS to better present document pages for printing. Added a print link to Phriction.
Test Plan: Tested Phriction, Phame, and Legalpad.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10131
Differential Revision: https://secure.phabricator.com/D16524
Summary:
Fixes T11610. Clean up some sketchy old code from long ago.
If you had rules that use conditions like "Accepted revision exists" and ran them in the test console, we'd never load the "CommitData" and fatal.
Instead, load CommitData in `newTestAdapter()` and generally make these pathways a little more modern.
Test Plan:
- Wrote an "Accepted Revision Exists" rule.
- Ran a commit in the test console.
- Before patch, got fatal from T11610.
- After patch, got clean test result.
- Also pushed a commit and reviewed the transcript to make sure the rule ran properly.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T11610
Differential Revision: https://secure.phabricator.com/D16522
Summary: Fixes T11607.
Test Plan:
- Made a comment using `{key ...}`.
- Used `bin/mail show-outbound --id X --dump-html > test.html` to review HTML:
{F1805304}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11607
Differential Revision: https://secure.phabricator.com/D16523
Summary: Fixes T11567. This way people can use things like `sans-serif` and `-webkit-small-control` for their "monospaced" font
Test Plan:
I added the hyphen to the regex then was able to set my Monospaced Font to be anything with a hyphen in it.
I also tried to break it pretty extensively, but couldn't find anything that would let me write malicious CSS or JS.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T11567
Differential Revision: https://secure.phabricator.com/D16519
Summary: Fixes T11595. Previously if a user didn't have permissions to view an application it would still appear in the application typeahead in various menus. This change will prevent that by checking if the app is installed for the viewer before displaying it as an option
Test Plan: I went to the "Pin Applications" menu and typed in "Conpherence" and saw it appear as an option. I then went to the "Edit Policies" menu as an admin and removed permissions for my user to use Conpherence. Once I did that, it no longer showed up in the typeahead menu
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T11595
Differential Revision: https://secure.phabricator.com/D16518
Summary:
Fixes T11396. Currently, you can keep clicking "Next >" forever to generate infinite instances of an event, even if it has a set end date.
Likewise, you can visit `/E123/999999` or whatever to stub out the 999999th instance of an event.
Instead:
- Before creating a new stub, make sure it happens before any end date.
- 404 stubs if we can't create them.
- Disable the "Next >" button if it isn't valid.
Test Plan:
- Visited `/E123/9999` for an event with a recurrence end date, got 404.
- Clicked "Next >" on an event with an end date, got new events until I hit the end date.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11396
Differential Revision: https://secure.phabricator.com/D16517
Summary: If you don't have any rooms, we currently show no menu and users have no logical means of closing the column. This lets "Hide Column" at least still appear. Fixes T9195
Test Plan: Fresh install, hit {key \}, see menu. Close Column.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9195
Differential Revision: https://secure.phabricator.com/D16516
Summary:
Fixes T11604. If we send two requests to render a brand new tag at about the same time (say, 50ms apart) but JIRA takes more than 50ms to return from its API call, the two processes will race one another and try to save the same external object.
If they do, have whichever one lost the race just load the object the other one created.
Apply this to other bridges, too.
Test Plan:
- Created a new task in JIRA.
- Referenced it for the first time in Differential, in a comment.
- This causes two tag renders to fire. This //might// be a bug but I spend 30 seconds on it without figuring out what was up. Regardless, we should fix the race even if the reason it's triggering so easily legitimately is a bug.
- Before patch: big error dialog (as in T11604).
- After patch: smooth sailing.
{F1804008}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11604
Differential Revision: https://secure.phabricator.com/D16514
Summary: In new PHUITwoColumnLayout, some new colors were used that aren't in the High Contrast PostProcessor. Adding them and proper fallbacks.
Test Plan: High Contrast on/off on a timeline page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16515
Summary: It's super
Test Plan: Set lots of project icons
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16513
Summary: This fixes a bug where the icon won't display since it doesn't include `fa-`.
Test Plan: Change icon on a project, revisit timeline and see icon.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16512
Summary:
Fixes T11583.
- When users run `bin/storage upgrade` for the first time on a new install, we currently give them a prompt which feels rough and which they can only reasonably ever answer "yes" to.
- We generally use cautionary language ("found issues with schema") in this workflow. Adjustments are now routine, so use more neutral and progress-oriented language ("found adjustments to apply").
Test Plan:
- Ran `bin/storage upgrade --namesapce kappa123`, got an adjustment using neutral language without prompting.
- Dropped a key, ran `bin/storage upgrade`, got normal workflow (but with more neutral language).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11583
Differential Revision: https://secure.phabricator.com/D16509
Summary: Fixes T11593. We ask for a list of values when searching for custom "link" fields, but don't handle it correctly when actually construcitng a query.
Test Plan:
Added this custom field:
```
{
"mycompany.target-version": {
"name": "Target Version",
"type": "link",
"search": true
}
}
```
Set a task to "beta". Let daemons index it. Queried for:
```
constraints: {
"custom.mycompany.target-version": [
"beta"
]
}
```
Got just one result back.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11593
Differential Revision: https://secure.phabricator.com/D16508
Summary:
Ref T11593. When you call a `*.search` method like `maniphest.search`, we don't currently validate that all the constraints you pass are recognized.
I think there were two very weak arguments for not doing this:
- It makes compatibility in `arc` across versions slightly easier: if we add a new constraint, we could add it to `arc` but also do client-side filtering for a while.
- Conduit parameter types //could//, in theory, accept multiple inputs or optional/alias inputs.
These reasons are pretty fluff and T11593 is a concrete issue caused by not validating. Just validate instead.
Test Plan:
- Made a `maniphest.search` call with a bogus constraint, got an explicit error about the bad constraint.
- Made a `maniphest.search` call with a valid constraint (`"ids"`).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11593
Differential Revision: https://secure.phabricator.com/D16507
Summary:
Ref T11589. When we hit a fatal setup issue (essentially always a connection failure) //after// we've already survived them on at least one request, we can be pretty sure a server went down and that the problem is not a setup/configuration issue.
In this case, show a friendlier error page instead of the fairly detailed technical one.
Test Plan:
- Broke MySQL config.
- Restarted Apache.
- Got the "admin/setup" error page:
{F1803268}
- Fixed the MySQL config.
- Loaded any page, to put us "in flight".
- Broke MySQL config.
- Loaded any page.
- Got the friendly "in flight" error page:
{F1803271}
If you want to design this better, easiest way to get to it is:
- Set `mysql.port` to `9999` in `conf/local/local.json`.
- Reload any page while already running (don't restart).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11589
Differential Revision: https://secure.phabricator.com/D16503
Summary: Ref T11589. Provide a way for scripts to say "just continue if database config fails", and use it in `bin/config` and `bin/storage`.
Test Plan:
- Broke database config.
- Ran `bin/config`, worked fine.
- Ran `bin/storage`, got helpful "set up the database" message.
- Ran `bin/repository`, got fatal.
- Ran normal site with valid/invalid config, got proper feedback.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11589
Differential Revision: https://secure.phabricator.com/D16502
Summary:
Ref T11589. Previously, when we failed to load database configuration we just continued anyway, in order to get to setup checks so we could raise a better error.
There was a small chance that this could lead to pages running in a broken state, where ONLY that connection failed and everything else worked. This was accidentally fixed by narrowing the exceptions we continue on in D16489.
However, this "fix" meant that users no longer got helpful setup instructions. Instead:
- Keep throwing these exceptions: it's bad to continue if we've failed to connect to the database.
- However, catch them and turn them into setup errors.
- Share all the setup code so these errors and setup check errors work the same way.
Test Plan:
- Intentionally broke `mysql.host` and `mysql.pass`.
- Loaded pages.
- Got good setup errors.
- Hit normal setup errors too.
- Put everything back.
- Swapped into cluster mode.
- Intentionally broke cluster mode, saw failover to readonly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11589
Differential Revision: https://secure.phabricator.com/D16501
Summary:
Ref T11589. This runs:
- preflight checks (critical checks: PHP version stuff, extensions);
- configuration;
- normal checks.
The PHP checks are split into critical ("bad version") and noncritical ("sub-optimal config").
I tidied up the extension checks slightly, we realistically depend on `cURL` nowadays.
Test Plan:
- Faked a preflight failure.
- Hit preflight check.
- Got expected error screen.
- Loaded normal pages.
- Hit a normal setup check.
- Used DarkConsole "Startup" tab to verify that preflight checks take <1ms to run (we run them on every page without caching, at least for now, but they only do trivial checks like PHP versions).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11589
Differential Revision: https://secure.phabricator.com/D16500
Summary:
Ref T11596. When exporting data from the Phacility cluster, we `bin/files migrate` data from S3 into a database dump on the `aux` tier.
With current semantics, this //moves// the data and destroys it in S3.
Add a `--copy` flag to //copy// the data instead. This leaves the old copy around, which is what we want for exports.
Test Plan:
- Ran `bin/files migrate` to go from `blob` to `disk` with `--copy`. Verified a copy was left in the database.
- Copied it back, verified a copy was left on disk (total: 2 database copies, 1 disk copy).
- Moved it back without copy, verified database was destroyed and disk was created (total: 1 database copy, 2 disk copies).
- Moved it back without copy, verified local disk was destroyed and blob was created (total: 2 datbabase copies, 1 disk copy).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11596
Differential Revision: https://secure.phabricator.com/D16497
Summary:
Ref T11589. Currently, initialization order is a bit tangled: we load configuration from the database, then later test if we can connect to the database.
Instead, I'm going to do: preflight checks ("PHP Version OK?", "Extensions installed?"), then configuration, then normal setup checks.
To prepare for this, flag core checks as "preflight" and add a setup panel to visually confirm that I didn't miss anything.
Test Plan: {F1803210}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11589
Differential Revision: https://secure.phabricator.com/D16499
Summary: Caught one of these while reviewing docs, grepped for the other one.
Test Plan: `grep`, reading
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16498
Summary: For phabricator. Adds a Slack auth adapater and icon.
Test Plan:
Create a new Slack Application for login, generate id and secret. Activate login and registration for Slack. Create a new account with Slack credentials. Log out. Log in with Slack credentials. Set my avatar with Slack. Slack. Slack.
{F1802649}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16496
If the namespace is something like "test_example" we currently fail to
renamespace the dump.
(Cowboy committing this since this is currently blocking a data export.)
Test Plan:
- Renamespaced a local dump, examined the output, saw 60 create / 60 use, reimported it.
- Will export in production.
Auditors: chad
Summary:
Two minor issues that I caught in the log while fixing Phame permissions:
- We had a JS bug which would cause us to immediately generate two comment previews at the exact same time -- one for loading the page, and one for "switching to desktop". Instead, only generate the "switch to desktop" preview if we really switched to desktop from a different device layout.
- These two requests could end up reading/writing the VersionedDraft table at exactly the same time fairly often (e.g., after a comment submission, the page would load, send two preview requests at exactly the same time, and they'd race fairly reliably for me locally). If we do race, recover from the race.
Test Plan:
Submitted some Phame comments.
- No more error log errors about VersionedDraft keys.
- Saw only one preview request when loading the page instead of two.
Here's the specific stack trace I caught:
```
[Mon Sep 05 12:15:33.639930 2016] [:error] [pid 50608] [client 127.0.0.1:55278] [2016-09-05 14:15:33] EXCEPTION: (AphrontDuplicateKeyQueryException) #1062: Duplicate entry 'PHID-POST-fknnpzjnsdgc3rqobhst-PHID-USER-pr5rjpuilpfserepsd2k-13' for key 'key_object' at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:314]
[Mon Sep 05 12:15:33.640801 2016] [:error] [pid 50608] [client 127.0.0.1:55278] arcanist(head=master, ref.master=9e82ef979e81), corgi(head=master, ref.master=5b9171222bc9), instances(head=stable, ref.master=485bc8128198, ref.stable=2983bc917601), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=phame2, ref.master=4b6da9735ba7, ref.phame2=4b6da9735ba7), phutil(head=stable, ref.master=97f05269fdb1, ref.stable=c14343ee620e), services(head=stable, ref.master=1fcb5cdb7582, ref.stable=2d8088a5b4b3)
[Mon Sep 05 12:15:33.640815 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #0 <#2> AphrontBaseMySQLDatabaseConnection::throwCommonException(integer, string) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:348]
[Mon Sep 05 12:15:33.640830 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #1 <#2> AphrontBaseMySQLDatabaseConnection::throwQueryCodeException(integer, string) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:289]
[Mon Sep 05 12:15:33.640833 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #2 <#2> AphrontBaseMySQLDatabaseConnection::throwQueryException(mysqli) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:185]
[Mon Sep 05 12:15:33.640836 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #3 <#2> AphrontBaseMySQLDatabaseConnection::executeRawQuery(string) called at [<phutil>/src/xsprintf/queryfx.php:8]
[Mon Sep 05 12:15:33.640839 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #4 <#2> queryfx(AphrontMySQLiDatabaseConnection, string, string, string, array, string)
[Mon Sep 05 12:15:33.640841 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #5 <#2> call_user_func_array(string, array) called at [<phutil>/src/aphront/storage/connection/AphrontDatabaseConnection.php:42]
[Mon Sep 05 12:15:33.640844 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #6 <#2> AphrontDatabaseConnection::query(string, string, string, array, string) called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1261]
[Mon Sep 05 12:15:33.640846 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #7 <#2> LiskDAO::insertRecordIntoDatabase(string) called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1106]
[Mon Sep 05 12:15:33.640849 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #8 <#2> LiskDAO::insert() called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1075]
[Mon Sep 05 12:15:33.640851 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #9 <#2> LiskDAO::save() called at [<phabricator>/src/applications/draft/storage/PhabricatorVersionedDraft.php:65]
[Mon Sep 05 12:15:33.640854 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #10 <#2> PhabricatorVersionedDraft::loadOrCreateDraft(string, string, integer) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1669]
[Mon Sep 05 12:15:33.640857 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #11 <#2> PhabricatorEditEngine::buildCommentResponse(PhamePost) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:894]
[Mon Sep 05 12:15:33.640859 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #12 <#2> PhabricatorEditEngine::buildResponse() called at [<phabricator>/src/applications/phame/controller/post/PhamePostEditController.php:60]
[Mon Sep 05 12:15:33.640862 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #13 <#2> PhamePostEditController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
[Mon Sep 05 12:15:33.640865 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #14 phlog(AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php:27]
[Mon Sep 05 12:15:33.640868 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #15 PhabricatorAjaxRequestExceptionHandler::handleRequestException(AphrontRequest, AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:644]
[Mon Sep 05 12:15:33.640870 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #16 AphrontApplicationConfiguration::handleException(AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:242]
[Mon Sep 05 12:15:33.640873 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #17 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]
[Mon Sep 05 12:15:33.640879 2016] [:error] [pid 50608] [client 127.0.0.1:55278] #18 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]
```
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16494
Summary:
Fixes T11584. This controller does unnecessary CAN_EDIT policy checks.
These checks are enforced by `EditEngine`, and you can make certain types of edits (including comments) even without full-blown edit permission.
Test Plan:
- Commented as a user without edit permission.
- Tried to edit as a user without edit permission, was rebuffed with a policy dialog.
- Edited as a user with edit permission.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11584
Differential Revision: https://secure.phabricator.com/D16493
Summary:
Fixes T11590. Currently, we incorrectly consider cluster repository versions that are (or were) on devices which are no longer part of the active cluster service when building this status screen.
Instead, ignore them. This is just a display bug; the actual `ClusterEngine` already had similar logic.
Test Plan:
- Added a bad leader record to `repository_workingcopyversion`.
- Before patch, got a bad "Partial (1w)" sync:
{F1802292}
- After patch, got a good "Sycnchronized":
{F1802293}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11590
Differential Revision: https://secure.phabricator.com/D16492
Summary: Ref T11132, swaps in new UI for welcome page using guide modules
Test Plan: Test instance and non instance guides. Test each setting. Unclear on how to test people / Phacility. Just change the URL link?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16482
Summary:
This is hacky, and I'm not sure I'm happy with it; Until T9365 is done, this will show up
broken tests with an appropriate star in the Revision History.
Test Plan: Created 1M messages in a couple of old diffs in a revision. The query took ~80us (On SSD drive).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16483
Summary:
Fixes T11577. When we connect to a host and try to select a database which does not exist, we currently treat it as though the host wasn't reachable.
This isn't correct, and prevents storage from being initialized while already in cluster mode, since the "config" database won't exist yet the first time we connect.
Instead, distinguish between `AphrontSchemaQueryException` (thrown on connection if the requested database is not present) and other errors.
Test Plan:
- Put Phabricator into cluster database mode (`cluster.databases = ...`).
- Swapped `storage.default-namespace` to force initialization of a new install.
- Ran `bin/storage upgrade`.
- Before patch: Immediate fatal about unreachablility.
- After patch: Database initialized.
- Also ran initialization steps in tranditional single-host mode (`cluster.databases` empty, `mysql.host` configured).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11577
Differential Revision: https://secure.phabricator.com/D16489
Summary:
Ref T10867 for original use case. This workflow provides a plausible way for administrators to stop the daemons when performing upgrades or maintenance, then bring those daemons back up without resulting in the failure of builds that were running at the time.
On our organization's phab install, builds are running 24/7. The majority of these builds last for at least several minutes, and contain build steps which fail if interrupted and then resumed, as happens when turning daemons on and off.
Instead of allowing these build steps to resume execution as normal, this workflow will instruct active builds to restart their entire build process instead of just resuming whichever step they were on.
Test Plan:
contrived a build plan which would fail if resumed partway through:
- lease a working copy
- command `touch restart_{build.id}`
- command `test -e restart_{build.id} && rm restart_{build.id} && sleep 60`
followed old procedure:
- run a few of these builds manually
- `./bin/phd stop`
- `./bin/phd start`
- saw the builds fail
followed new procedure:
- run a few of these builds manually
- `./bin/phd stop`
- `./bin/harbormaster restart --active`
- `./bin/phd start`
- saw the builds pass
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10867
Differential Revision: https://secure.phabricator.com/D16485
Summary:
* Fixed conveted => converted
Ref T11576
Test Plan: * Looked at a page, where somebody converted an AllDay Event to a normal one
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Tags: #calendar
Maniphest Tasks: T11576
Differential Revision: https://secure.phabricator.com/D16488
Summary: Ref T11575. After D16431, the parser may return arrays.
Test Plan: Ran `bin/diviner generate --clean` in `phabricator/` without errors. Previously, this raised some parsing errors related to getting arrays where strings were expected.
Reviewers: chad, yelirekim, joshuaspence
Reviewed By: joshuaspence
Maniphest Tasks: T11575
Differential Revision: https://secure.phabricator.com/D16487
Summary:
Ref T7924. This:
- Adds support for remarkup block changes to Modular Transactions.
- Exposes remarkup changes from the Calendar event "Description" transaction.
This makes stuff like mentions and file embeds work properly.
Test Plan:
Mentioned a task in an event description, saw a mention appear on the task.
Uploaded a file to an event description, saw the file become "Attached" to the event.
(Neither of these worked properly before.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7924
Differential Revision: https://secure.phabricator.com/D16481
Summary: See T10746.
Test Plan: Fail one of several builds, run `./bin/harbormaster update`, see that Build Status is Failed.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, O14 ATC Monitoring, yelirekim
Maniphest Tasks: T10746
Differential Revision: https://secure.phabricator.com/D16480
