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:
Ref T11893. Previously, we excuted all `bin/storage` queries through `StorageManagementAPI` objects.
After D16848, we execute some queries through `PhabricatorDatabaseRef`. However, the refs we use weren't getting passed the `-u` / `-p` flags correctly, for specifying alternate administrative credentials.
Test Plan:
- Created a second account (`trunk`).
- Ran `bin/storage -u trunk`.
- Made libphutil throw when not connecting as that user.
- Before patch: some connections incorrectly used the default user, "root".
- After patch: all connections correctly used the configured user, "trunk".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11893
Differential Revision: https://secure.phabricator.com/D16901
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:
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: 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:
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 T11886. D16882 prevented workboard cards from being dragged between columns because it reduced the effective document height to almost nothing (~78px, just the menu bars).
Instead, use the larger of the document height and viewport height as "infinity".
Test Plan: Dragged columns between workboards successfully.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11886
Differential Revision: https://secure.phabricator.com/D16885
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: Fixes some outlying issues with new comment box and Phame Posts.
Test Plan: Review CSS in sandbox, post new reply, see proper spacing.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16869
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:
Ref T11816. We currently try to expand the picker control from the little calendar icon. This works alright on desktop, but not great on tablet/mobile.
On tablet/mobile, center the control on screen instead. Also, mask the background since it can get a bit busy because we can't really control what's under the control anymore.
Finally, move the little popup thing around if the user resizes the window.
Test Plan: {F1922773}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16866
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
