Summary:
Fixes T12807. Some shells may apparently mangle/strip UTF8 characters? Just dodge this whole problem by sending the pattern over stdin rather than actually figuring out the particulars.
Related tasks, like T7339 and T5554, discuss finding broader fixes for this class of issue, and this definitely isn't exactly a fully legitimate fix, but in many cases (as here) we can reasonably just avoid the problem rather than actually fixing it, at least for a long time.
Test Plan: Searched for emoji and non-emoji locally, but this worked fine (on OSX) for me before the patch too.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12807
Differential Revision: https://secure.phabricator.com/D18105
Summary:
Fixes T12811. The issue here //appears// to be that both the "alice committed rXYZabc" and "Herald added projects..." actions have the same (default) strength and end up applying in arbitrary order, and probably got shuffled around as this transitioned to Modular transactions.
Give "alice committed rXYZabc" an explicitly higher action strength.
Test Plan:
- Wrote an "Always, add project X" Herald rule for commits.
- Ran `bin/repository reparse --herald ...`.
- Saw an "alice committed rXYZabc" story instead of a "Herald added projects: X" story.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12811
Differential Revision: https://secure.phabricator.com/D18104
Summary:
Fixes T12806. Ref T12733.
- Don't count synthetic (lint) comments as anything.
- When you begin writing an inline then cancel it, don't count it as anything.
- When we would show "0 / X", just show "X".
Test Plan:
- Viewed a diff with synthetic comments, no button.
- Wrote, then cancelled an inline. No "X comments".
- Clicked / unlicked "Done", saw "X" -> "1 / X".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12806, T12733
Differential Revision: https://secure.phabricator.com/D18103
Summary: Does the UI work that's part of T12234 and adds migrations for both of the old-style duplicate transactions.
Test Plan:
- Started with a clean DB.
- Checked out really old code that marks tasks as dupes using comments.
- Made a bunch of tasks and closed some as dupes. Made a bunch of additional comments.
- Checked out D10427 and did a `storage upgrade`.
- Made a bunch more new tasks and dupes.
- Snapshotted DB.
- Ran migration repeatedly until all expected edges showed up in the `phabricator_maniphest.edge`table.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T12234
Differential Revision: https://secure.phabricator.com/D18037
Summary: Ran across a few straglers. Convert to the correct color.
Test Plan: grep for profile-image-button, check profile image selection page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18096
Summary: These are now unused.
Test Plan: grep, remove uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18090
Summary: Porting over a fix that we could miss the tail end of commits. Also use the new tag borderless option.
Test Plan: Review various commit pages in profile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18086
Summary: Little nits and spacing changes to viewing diffusion commit history on phones.
Test Plan:
Review in Chrome, iOS Simulator.
{F4990749}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18085
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.
Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18077
Summary: Formally support borderless tags in PHUITagView.
Test Plan: Used in Diffusion History List
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18074
Summary:
Ref T12793. I'd like to understand exactly when we broke this, but this seems to be a minimal fix that shouldn't do anything surprising.
When you move document `/a/` to `/a/b/` and that path doesn't exist yet, the Content transaction currently fails because there's "no content". The content gets added later by the "move" transaction but this is implicit.
To make this work, just ignore the "missing field" error. This is a little roundabout but unlikely to break anything in weird ways.
Test Plan:
- Moved document `/a/b/` to `/a/b/c/`.
- Before patch: error about missing content.
- After patch: move worked properly.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12793
Differential Revision: https://secure.phabricator.com/D18069
Summary: Saturate the color a little more, add yellow
Test Plan: uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18068
Summary:
Commits in the list are grouped by the date they occurred in server time. This may not be the date they occurred in client time.
Use client time, not server time, to group commits.
Test Plan:
- Set server timezone to "Asia/Famagusta".
- Set client timezone to "America/Los_Angeles".
- Viewed Phabricator repository history.
Here's what it looks like before the change:
{F4987094}
Note that the headers of the first two groups both say "Yesterday".
This is because the first commits in each group occurred on June 1 and June 2, respectively, in Famagusta, but both occurred on June 1 in Los Angeles.
Here's what it looks like after the change:
{F4987095}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18067
Summary:
Currently, the last group of commits is not shown in the list view because the final `$list` is never added to `$view`.
For example, if the first page would contain commits from "April 7", "April 6", and "April 5", commits from "April 5" are not shown.
(If a repository has 100 commits in a single day, nothing is shown.)
On this server, here's the bottom of page 1:
{F4987087}
Here's the top of page 2:
{F4987088}
However, here's `git log` between those commits:
```
$ git log --oneline 7e46^..5f49f
5f49f9c793 Add sound to logged out Conpherence
1644b45050 Disperse task subpriorities in blocks
c6a7bcfe89 Make Pholio description behave as a remarkup field (e.g., subscribe mentioned users)
bbc5f79227 Make membership lock/unlock feed stories read more naturally
789d57522b Make editing project images redirect to "Manage" more consistently
10b3879232 Make Project slug/hashtag transactions render a little more nicely
abd791889c Update Maniphest title transaction again
5a34b299e4 Update Maniphest title language
601622013d Clarify milestone/subproject creation language
c9889e3d55 Fix an issue in Phriction where moving a document just copied it instead
fdf00f6df4 Clean up some minor UI behaviors in Differential
6c46f27d98 Add quest objectives to the minimap
d783299a19 Fix Phriction status not set property on new document
93e28da76e Add more "disabled" UI to PHUIObjectItemView
7e46d7ab6a Migrate Project color to modular transactions
```
This group of commits does not currently appear anywhere in the list.
Test Plan: Viewed a page of commits, saw 100 commits.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18066
Summary:
Ref T12733. Some minor issues:
- The `strlen(...)` test against `$this->text` fails if a caller does something like `setText(array(...))`. This is rare, but used in `DiffusionBrowseController`, from D15487.
- Add PHUIX examples for icon-only buttons.
- Remove unused `SIMPLE` constant now that no callsites remain.
Test Plan:
- Viewed a directory in Diffusion's "Browse" view in a Git repository, no longer saw a warning / error log.
- Viewed PHUIX Components UI examples.
- Grepped for `::SIMPLE`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18065
Summary:
- Add a simple green button... maybe don't need
- Fix tokenizer search icon
- Splite simple and button-bar into own files
Test Plan: uiexamples, various pages with buttons, diffusion
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18063
Summary:
Fixes T12789. See that task for discussion. Currently, when multiple packages own the same path but have different dominion rules we get some weird/aribtrary/inconsistent results.
Instead, implement these rules:
- If zero or more weak and one or more strong packages claim a path, the strong packages (exactly) all own it.
- If one or more weak packages and zero strong packages claim a path, the weak packages all own it.
The major change here is that instead of keeping the //first// weak package we run into, we keep all the weak packages with the longest claim that we run into.
This needs to be implemented twice because Owners has two different near-copies of this logic, only one of which has test coverage. Some day maybe this will get fixed.
Test Plan:
- Added failing unit tests, made them pass.
- Viewed all A/B strong/weak combinations in Diffusion, saw sensible ownership results.
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Maniphest Tasks: T12789
Differential Revision: https://secure.phabricator.com/D18064
Summary:
Fixes T12790. I don't think this was actually a regression, Settings just wasn't launchable before global settings (since it had no real landing page, and the profile menu always had a link) and didn't get marked launchable once we added them.
I also double-checked other un-launchable apps; Nuance is probably close enough to make launchable now while I'm in here.
Test Plan: Typed "settings" into global typeahead, got settings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12790
Differential Revision: https://secure.phabricator.com/D18062
Summary: Ref rPf2fcafb40dde94ddf4ee22716fea74fca0334a64#38208, I think this is a more usable layout. Gets rid of clippy, audit. Adds back Differential link as tag, Build Status as button.
Test Plan: Faked data on this for Differential, Builds, should all work though. Test on real and fake repositories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18061
Summary:
See D18018. Ref T12787. This doesn't actually work; we started publishing these stories as a side effect of converting to ModularTransactions, then I fixed the rendering.
This mechanism has very few callsites and I suspect we may want to get rid of it (see T12787) so just keep publishing these stories for now.
Test Plan: Changed the point value of a task, saw a feed story both before and after the patch.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12787
Differential Revision: https://secure.phabricator.com/D18059
Summary: Fixes T12787. Modular Transactions don't actually support `shouldHideForFeed()`. I'll add some discussion to the task.
Test Plan: Created a subtask, saw no more "X reopened Y, a subtask of P" feed story.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12787
Differential Revision: https://secure.phabricator.com/D18058
Summary: Let's buttons just be an icon, no pressure to also have text.
Test Plan: UIExamples, Search, Home, Policy Controls... Probably 99% of them.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18056
Summary: Ref T12780. I'd like 18,000 GitHub stars now please thank you
Test Plan: this feature is awful
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12780
Differential Revision: https://secure.phabricator.com/D18053
Summary: Ref T12780. Makes the button do something useful, like link to the history at the right spot in the graph.
Test Plan: Click on various browse buttons, get correct url.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12780
Differential Revision: https://secure.phabricator.com/D18054
Summary:
Ref T12733. Ref M1476. This adds `PHUIXButtonView`, for client-side button rendering.
It also adds a PHUIX example which renders the server and client versions of each component side-by-side so it's easier to see if they're messed up.
Test Plan: {F4984128}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18051
Summary:
Ref M1476. I'm planning to add some PHUIX examples, but sort out the existing examples a little first. I added some categories:
- Catalogs: these are what I look at most often (emoji, icons, colors).
- Single use: elements with only one use (badges, feed stories, hovercards, setup issues).
- Technical: examples which are really just "test this thing in the browser" (avatars, gestures, notifications, remarkup).
- Other: evrything else, mostly general-purpose multi-use components.
Test Plan:
(See left nav.)
{F4984042}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18050
Summary: Ref M1476. I'm going to see if I can set up side-by-side "PHUI" vs "PHUIX" to make maintaining them a touch easier. Before doing that, nuke some really old UI examples that don't seem very useful.
Test Plan: Viewed UIExamples, saw fewer bad ones.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18049
Summary:
Ref M1476. Currently, `setColor('simple')` is meaningful. Instead, `setButtonType('simple')`.
Depends on D18047.
Test Plan: Looked at UI examples, Phame, Auth. Notifications mooted by D18047.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18048
Summary: Ref T12733. Completely removes the objectives UI.
Test Plan:
- Grepped for `objective`, etc.
- Browsed revisions, no JS errors / broken stuff.
- (If I missed anything, it's likely to turn up in followup changes.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18043
Summary: This moves Diffusion History to use an easier to parse list view for commits and their (diff, audit, build) status. I left TableView around, which is used on a repositories home, and we can maybe add a "graph view" history back as another controller. Not sure what the real use is for that kind of feature though. I don't have Harbormaster set up locally so I could use another install to give this a run. I also expect to maybe not live with this UI as final, I like the UX, but the icons for indicating status don't really feel great to me, just OK.
Test Plan:
pull various repositories, check various history displays.
{F4980356}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18039
Summary: Ref T12423. Set the grouping by priority. Note this doesn't render headers but I don't want to spend a lot of time on this.
Test Plan: Review tasks in my sandbox, see them ordered by priority.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12423
Differential Revision: https://secure.phabricator.com/D18046
Summary: Ref T12776. This extraction of file PHIDs extracted "PholioImage" object PHIDs (`PHID-PIMG-...`), not "File" PHIDs (`PHID-FILE-...`). Instead, dig into the Pholio images and actually extract the file PHIDs. This method is now similar to the `PholioImageFileTransaction` method, which works already.
Test Plan:
- Create a mock.
- Update one of the images.
- In Files, view the "Attached" tab of the updated image.
- Before patch: not attached to mock.
- After patch: properly attached to mock.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12776
Differential Revision: https://secure.phabricator.com/D18042
Summary: Fixes T12775. Currently, we do not validate this option and it's possible to configure it in an invalid way.
Test Plan: Tried to misconfigure things, was helpfully pointed toward errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12775
Differential Revision: https://secure.phabricator.com/D18041
Summary: These are unfortunatly manually built so I missed them in testing circle view changes.
Test Plan: Test lightbox, conpherence, uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18036
Summary: Start on plan outlined in T12124. Adds a new Conduit method for querying information about task priorities.
Test Plan: Ran locally; observed expected output: {F4979109}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D18035
Summary:
Ref T12314. Open to counterdiffs / iterating / suggestions / skipping most or all of this, mostly just throwing this out there as a maybe-reasonable first pass.
When a task has a subtype (like "Plant" or "Animal"), provide some hints on the task list, workboards, and task detail.
To make these hints more useful, allow subtypes to have icons and colors.
Also use these icons and colors in the typeahead tokens.
The current rule is that we show the subtype if it's not the default subtype. Another rule we could use is "show the subtype if there's more than one subtype defined", but my guess is that most installs will mostly have something like "normal task" as the default subtype.
Test Plan:
The interfaces this affects are: task detail view, task list view, workboard cards, subtype typeahead.
{F3539128}
{F3539144}
{F3539167}
{F3539185}
Reviewers: chad
Reviewed By: chad
Subscribers: johnny-bit, bbrdaric, benwick, fooishbar
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17451
Summary: I think this is reasonable for my current use case, but stacking icons overally is pretty clunky.
Test Plan:
UIExamples
{F4978899}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18032
Summary: I think this name is more accurate, also add proper links to author image.
Test Plan: Review commits in sandbox, see new URL on image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18026
Summary: Fixes T12505. `PhabricatorProjectsMembershipIndexEngineExtension->materializeProject()` was incorrectly bailing early for milestone objects, which prevented milestone members from being calculated correctly. This was causing problems where (for example) an Owners package owned by a milestone wasn't being satisfied when a member of the milestone approved a revision.
Test Plan: Invoked migration, observed that a user's milestones correctly showed up when searched for. Also observed that accepting a revision on behalf of a milestone now satisfies Owners rules.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12505
Differential Revision: https://secure.phabricator.com/D18033
Summary:
Fixes T12762. Currently, there's no way to get from these boxes into generaly history in Feed, and it isn't clear that the operation is possible.
For now, add some simple links. See T12762 for future work.
Test Plan:
- Viewed user profles, saw "View All".
- Viewed project profiles, saw "View All".
{F4978858}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18030
Summary: Ref T12762.
Test Plan:
- Ran queries with start date, end date, both, neither.
- Used EXPLAIN to try to make sure doing the bitshift isn't going to be a performance issue.
{F4978842}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18029
Summary:
Ref T12762. This updates FeedSeachEngine to user modern construction.
I've tried to retain behavior exactly, although the "Include stories about projects I'm a member of" checkbox is now nonstandard/obsolete. We could likely fold that into "Include Projects" in a future change which does a backward compatibility break.
Test Plan:
- Queried feed without constraints.
- Queried feed by user, project, "stuff I'm a member of", prebuilt "Tags" query.
- Viewed user profile / project profile feeds.
- Used function tokens (`viewer()`, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18028
Summary: Ref T12762. Updates some conventions and methods. This has no (meaningful) behavioral changes.
Test Plan:
- Grepped for `setFilterPHIDs()`.
- Viewed main feed, user feed, project feed.
- Called `feed.query`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12762
Differential Revision: https://secure.phabricator.com/D18027
Summary:
Fixes T12720. Currently, old daemon records are collected based on creation date. By default, the GC collects them after 7 days.
After T12298, this can incorrectly collect hibernating daemons which are in state "wait".
In all cases, this could fail to collect daemons which are stuck in "running" for a long time for some reason. This doesn't seem to be causing any problems right now, but it makes me hesitant to do "dateCreated + not running or waiting" since that might make this become a problem, or make an existing problem with this that we just haven't bumped into worse.
Daemons always heartbeat periodically and update their rows, so `dateModified` is always fresh, so collect rows based only on modification date.
Test Plan:
- Ran daemons (`bin/phd start`).
- Waited a few minutes.
- Verified that hibernating daemons in the "wait" state had fresh timestamps.
- Verified that very old daemons still got GC'd properly.
```
mysql> select id, daemon, status, FROM_UNIXTIME(dateCreated), FROM_UNIXTIME(dateModified) from daemon_log;
+-------+--------------------------------------+--------+----------------------------+-----------------------------+
| id | daemon | status | FROM_UNIXTIME(dateCreated) | FROM_UNIXTIME(dateModified) |
+-------+--------------------------------------+--------+----------------------------+-----------------------------+
| 73377 | PhabricatorTaskmasterDaemon | exit | 2017-05-19 10:53:03 | 2017-05-19 12:38:54 |
...
| 73388 | PhabricatorRepositoryPullLocalDaemon | run | 2017-05-26 08:43:29 | 2017-05-26 08:45:30 |
| 73389 | PhabricatorTriggerDaemon | run | 2017-05-26 08:43:29 | 2017-05-26 08:46:35 |
| 73390 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:29 | 2017-05-26 08:46:35 |
| 73391 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:33 | 2017-05-26 08:46:33 |
| 73392 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:37 | 2017-05-26 08:46:31 |
| 73393 | PhabricatorTaskmasterDaemon | wait | 2017-05-26 08:43:40 | 2017-05-26 08:46:33 |
+-------+--------------------------------------+--------+----------------------------+-----------------------------+
17 rows in set (0.00 sec)
```
Note that:
- The oldest daemon is <7 days old -- I had some other older rows but they got GC'd properly.
- The hibernating taskmasters (at the bottom, in state "wait") have recent/more-current `dateModified` dates than their `dateCreated` dates.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12720
Differential Revision: https://secure.phabricator.com/D18024
Summary:
Ref T12738. By default, we process Nuance commands in the background. The intent is to let the user continue working at full speed if Twitter or GitHub (or whatever) is being a little slow.
Some commands don't do anything heavy and can be processed in the foreground. Let commands choose to try foreground execution.
Test Plan: Threw complaints in the trash, saw them immediately go into the trash.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18015
Summary: Makes this a bit more flexible and allow UI to take over `col-2` completely. Also cleaned up application search a little with tags
Test Plan: Review various pages, grep for callsites.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18021
Summary: Some of these are unused, defaults to a lighter color naturally.
Test Plan: uiexamples, grep, phriction
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18020
Summary:
Fixes T12757. Here's a simple repro for this:
- Add a package you own as a reviewer to a revision you're reviewing.
- Open two windows, select "Accept", don't submit the form.
- Submit the form in window A.
- Submit the fomr in window B.
Previously, window B would show an error, because we considered accepting on behalf of the package invalid, as the package had already accepted.
Instead, let repeat-accepts through without complaint.
Some product stuff:
- We could roadblock users with a more narrow validation error message here instead, like "Package X has already been accepted.", but I think this would be more annoying than helpful.
- If your accept has no effect (i.e., everything you're accepting for has already accepted) we currently just let it through. I think this is fine -- and a bit tricky to tailor -- but the ideal/consistent beavior is to do a "no effect" warning like "All the reviewers you're accepting for have already accepted.". This is sufficiently finnicky/rare (and probably not terribly useful/desiable in this specific case)that I'm just punting.
Test Plan: Did the flow above, got an "Accept" instead of a validation error.
Reviewers: chad, lvital
Reviewed By: chad, lvital
Subscribers: lvital
Maniphest Tasks: T12757
Differential Revision: https://secure.phabricator.com/D18019
Summary: In some cases we may want a different URI for the image on an item than the header/title of the item (like user / title). This prioritizes ImageHref over Href.
Test Plan: uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18016
Summary:
Fixes T12753. See that task for reproduction instructions.
We add a `GROUP BY` clause to queries with an "ANCESTOR" edge constraint only if the constaint has more than one PHID, but this is incorrect: the same row can be found twice by an ANCESTOR query if task T is tagged with both "B" and "C", children of "A", and the user queries for "tasks in A".
Instead, always add GROUP BY for ANCESTOR queries.
Test Plan:
- Followed test plan in T12753.
- Saw proper paging controls after change.
- Saw `GROUP BY` in DarkConsole.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12753
Differential Revision: https://secure.phabricator.com/D18012
Test Plan: Unit tests pass, manually changed the default sort and filter on a workboard and observed expected transactions in the DB.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D18013
Summary: Ref T12738. Implements some modular behavior for Nuance commands.
Test Plan: {F4975322}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18011
Summary:
Ref T12738. This makes clicking "Throw In Trash" technically do something, sort of.
In Nuance, the default mode of operation for actions is asynchronous -- so you don't have to wait for a response from Twitter or GitHub after you mash the "send default reply tweet" / "close this pull request with a nice response" button and can move directly to the next item instead.
In the future, some operations will attempt to apply synchronously (e.g., local actions like "ignore this item forever"). This fakes our way through that for now.
There's also no connection to the action actually doing anything yet, but I'll probably rig that up next.
Test Plan: {F4975227}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18010
Summary:
Ref T12738. This doesn't actually do anything yet, but allows items to define commands that show up in the UI.
Adds a "Throw in Trash" item for complaints.
This construction will allow future changes to add an `EngineExtension` which can provide generic/default commands across item types.
Test Plan: {F4975086}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18009
Summary:
Ref T12738. This is mostly just laying in groundwork and prerequisites, like the ability to query items by queue.
Eventually, this will become the main UI which staff use to process a queue of items. For now, it does nothing and renders nonsense.
This and probably the next big chunk of changes are all going to be made-up, nonfinal things that just make basic operations work until we have fundamental flows -- like "assign", "comment", "close" -- working at a basic level and can think more about UI/workflow.
Test Plan:
Visited the page, it loaded a mostly-reasonable item and then rendered nonsense:
{F4975050}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18008
Summary: Ref T12738. Some of the Nuance "form" workflows currently fatal after work on the GitHub stuff. Try to make everything stop fataling, at least.
Test Plan: Using "Complaints Form" no longer fatals, and now lodges a complaint instead.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18007
Summary: Minor, just shows the slugs on the manage project page, also normalized language to "details"
Test Plan: review a project with slugs, description.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D17985
Summary: Gives the ability to hide a big long block of text in an ObjectListItem without cluttering the UI.
Test Plan:
Added a test case to UIExamples. Click on icon, see content. Click again, content go away.
{F4974153}
{F4974311}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18006
Summary: Going to play a bit with this layout (diffusion sans audit) and see how it feels on profile. Uses a user image, moves the commit hash (easily selectible) and separates commits by date.
Test Plan:
Review profiles with and without commits.
{F4973987}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18005
Summary: This was interesting, because there were a mix of callsites using transactions and others that just set the property on the `Project` object. I made everything consistent in using transactions to change this property. I also found an implementation of `getTitle()` that I don't think is ever being invoked since `shouldHide()` is returning `true`, but I migrated it anyway.
Test Plan: Unit tests pass + enabling/disabling workboards (and importing).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D18004
Summary: This moves the navigation to a standard sidebar, and moves all actions to the curtain. Also pulled out info view when available for cleaner UI.
Test Plan:
Create a git, svn, hg test repository and verify each page in the sidebar renders as expected.
{F4973792}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18003
Test Plan: Unit tests pass. Went through the UI for creating new subprojects and milestones, but didn't setup some API calls to check that all the validation errors were still caught.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17999
Summary: Fixes T12744. Unclear why `null` doesn't work here but does for the title, but `!strlen` seems to work fine in both cases.
Test Plan: Create a new task, check mail folder, see [Created]
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12744
Differential Revision: https://secure.phabricator.com/D18002
Summary: The tag/shade stuff changed, so purge older markup (like Diviner documents).
Test Plan: {F4972666}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17998
Summary: Ref T12625
Test Plan: Move a document to a new location, verify the old and new document. Edit both. Grep for MOVE_AWAY
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12625
Differential Revision: https://secure.phabricator.com/D17988
Summary: GREEN
Test Plan: View a dashboard page, see green button
Reviewers: amckinley, epriestley
Reviewed By: epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17997
Summary: Ref T12738. Update sources to modular transactions.
Test Plan: Created and edited a source.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D17994
Summary:
Ref T12738. Moves existing non-modular transactions to modular transactions.
Some of these are pretty flimsy, but a lot of them don't actually work or do anything in Nuance yet anyway.
Test Plan: Gently poked Nuance, nothing fell over.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D17990
Summary: Grep for phui-tag-shade and verify we're no longer calling shade-color directly.
Test Plan: Search, workboard, story points, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17993
Summary: Adds a new tag type, starts to try to clean up the mess that are PHUITags
Test Plan:
Review UIExamples.
{F4972323}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17991
Summary: See T12673
Test Plan: Unit tests pass. Locked and unlocked a project and saw timeline changes.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17986
Summary: Ref T12423. Adds back revisions as a user profile page. I don't want to think about custom profiles for a while.
Test Plan: Make some diffs, visit my profile, see diffs.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12423
Differential Revision: https://secure.phabricator.com/D17987
Summary: Adds a divider and better grouping
Test Plan: Click on dropdown menu on a workboard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17984
Summary: Ref T12733. Shows a comment snippet when hovering inlines in the objective list.
Test Plan: {F4968490}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17980
Summary:
Ref T12733.
- While editing a comment, show a pink star ({icon star, color=pink}) with a tooltip.
- Slight UI tweaks, including draft comments getting an indigo pencil ({icon pencil, color=indigo}).
Test Plan: {F4968470}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17977
Summary: See D17955.
Test Plan: Loaded a revision, no longer saw annotations with prototypes off. Still saw annotations with prototypes on.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17983
Summary: Fixes T12735. Adds a sound if the user is logged out, skips checking a setting.
Test Plan: set participants to null and verify sound plays, no exceptions1111
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12735
Differential Revision: https://secure.phabricator.com/D17973
Summary:
Ref T7664. The current algorithm for moving task subpriorities can end up stuck in a real sticky swamp in some unusual situations.
Instead, use an algorithm which works like this:
- When we notice two tasks are too close together, look at the area around those tasks (just a few paces).
- If things look pretty empty, we can just spread the tasks out a little bit.
- But, if things are still real crowded, take another look further.
- Keep doing that until we're looking at a real nice big spot which doesn't have too many tasks in it in total, even if they're all in one place right now.
- Then, move 'em out!
Also:
- Just swallow our pride and do the gross `INSERT INTO ... "", "", "", "", "", "", ... ON DUPLICATE KEY UPDATE` to bulk update.
- Fix an issue where a single move could cause two different subpriority recalculations.
Test Plan:
- Changed `ManiphesTaskTestCase->testTaskAdjacentBlocks()` to insert 1,000 tasks with identical subpriorities, saw them spread out in 11 queries instead of >1,000.
- Dragged tons of tasks around on workboards.
- Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7664
Differential Revision: https://secure.phabricator.com/D17959
Summary: Ref T12732. This is pre-existing but fix it since I caught it while banging around.
Test Plan: {F4967442}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17970
Summary:
Ref T12732. Currently, different ways of setting a profile image can leave you in different places.
Instead, always send the user back to the "Manage" page.
Test Plan: Used "Current Picture", "use picture", "Build picture" and "upload picture", always ended up in the same spot.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17967
Summary: Ref T12732. Use `renderValue()` to build `renderValueList()` so we get nice fancy text for these.
Test Plan: {F4967410}
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17966
Summary: Ref T12732. See D17918. With modular transactions, `getCustomTransactionNewValue()` isn't actually called.
Test Plan: Moved document `/x/` to `/y/`, saw document gone at `/x/` instead of copied.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17963
Summary:
Minor UI tweaks:
- Use the dynamic icon for each file (e.g., image, text), not a hard-coded icon.
- Render the path (less important) in grey and the filename (more important) in black.
Test Plan: {F4966176}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17957
Summary:
Add important objectives (like waygates and quest markers) to the minimap.
This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.
Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)
{F4966037}
Reviewers: chad, amckinley
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17955
Summary: I deleted too many lines of code here and TYPE_MOVE was always being applied when CONTENT was set. This should fix on next document save, but should I write some migration tool anyways?
Test Plan: Create a new document, see document with correct status.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17960
Summary: Brings more UI tweaks to disabled objects, like projects/people. Also fixes a missing icon in projects.
Test Plan: Application search with people and projects that have disabled results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17962
Summary: I'm not sure you can actually remove a project's image (maybe via the API?), but I kept the code for rendering the relevant title/feed anyway.
Test Plan: Unit tests + adding/changing project pictures.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17954
Test Plan: Unit tests all pass. Added/removed/altered some project hashtags and observed expected transactions in timeline.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17952
Summary: This allows adding of pinboard items to a timeline. I'm hoping we can get this in for Maniphest (Pholio, Cover Image) and Macro (because, Macro), but unsure how to scalably do this. Anyways, here's the front end.
Test Plan:
Make some fake timeline items in UIExamples, test mobile, tablet, and desktop breakpoints.
{F4965798}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17950
Summary:
Fixes T1591. This was removed long ago because it was a mess to implement and caused a bunch of weird issues, and also my tolerance for dealing with weird JS issues was much, much lower.
I have now survived the fires of JX.Scrollbar and would love to address 200 small nitpicks about obscure browser behaviors on Linux, so open the floodgates again.
A secondary goal here is to create room to add a global view state menu on the right, with 300 options like "hide all inlines", "hide done inlines", "hide collapsed inlines", "hide ghosts", "show ghosts", "enable filetree", "disable filetree", etc, etc. Not sure how much of this I'll actually do. I have one more experiment I want to try first.
Test Plan: {F4963294}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1591
Differential Revision: https://secure.phabricator.com/D17945
Summary:
Ref T12616. This puts "h" back to collapse or expand the current file.
This removes some very complicated/messy code around following links in the table of contents and getting files auto-expanded. I suspect no one will miss this, but we can restore it if ayone notices.
Test Plan: Pressed "h" to collapse/expand a file. Also used the menu items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17940
Summary: Also changes access modifiers on `PhabricatorProjectTransactionEditor` and sets up `storage` for `applyExternalEffects`.
Test Plan: Created new projects, attempted to create without name, with too long of a name, and with a name that conflicts with other projects and observed expected errors.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T12673
Differential Revision: https://secure.phabricator.com/D17947
Summary: Used by `PholioImageFileTransaction::mergeTransactions()`. I forgot to test adding multiple images to a Mock at the same time after migrating `mergeTransactions` over to the modular framework.
Test Plan: Added multiple images in a single transaction and didn't get an exception about accessing a protected function.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17946
Summary: This says "Edit Subscription" but the page only lets you update the payment method for autopay. I think this is confusing users. I tried to find the best language here, but suggest something else if you prefer.
Test Plan: Review language on sample subscription page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17944
Summary: Skips rendering of partial elements if no actions are present.
Test Plan: Tested on profile menu item page, maniphest curtain, phriction dropdown, and instance backups page (no actions at all).
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17931
Summary:
Fixes T8323. See that task for a description.
We were using `nonempty()`, but that rule doesn't cover synthetic deletions (file present in an earlier diff, but no longer present in the later diff).
Test Plan: Followed the steps in T8323, got a clean comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8323
Differential Revision: https://secure.phabricator.com/D17929
Summary: Ref T12616. This makes line range selection use the new code, and removes the remainder of the old "hover a line number" / "select a line range" code.
Test Plan: Hovered line numbers; selected line ranges.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17927
Summary: We currently override button color for headers, since the default is blue, but if a developer sets a specific color, we should respect that.
Test Plan: Set a button in the header to green and see green. See grey everywhere else.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17922
Summary:
Fixes T7682. The left-hand-side "<th />" row did not generate with the correct ID.
(I couldn't reproduce the exact issue described in T7682, but hovering comments on either side now works properly for me.)
Test Plan: {F4962479}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7682
Differential Revision: https://secure.phabricator.com/D17926
Summary:
Ref T11401. Fixes T5232. Ref T12616.
Partly, this moves more code over to the new stuff.
This also allows "r" to work if you have code selected (not just comments). If you "reply" to code, you start a new comment.
You can "R" a comment to quote it. This just starts a new comment normally if you "R" a block of code. This is sort of a power-user version of "quote" since it seems like it probably doesn't really make sense to put it in the UI ever (maybe).
With the new click-to-select, you can click + "R" to reply-with-quote.
Test Plan: Used "r" and "R" to reply to comments and code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11401, T5232
Differential Revision: https://secure.phabricator.com/D17920
Summary: Moves this transaction over to modular transactions.
Test Plan: Move a document, re-title a document, try to move over an existing document.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17918
Summary: Our `local.json` configuration file contains various secrets, including database usernames and passwords. As such, we recently changed the permissions on this file from `0644` to `0640`. After doing so, however, I constantly forget to run commands with `sudo`. This is made worse by the fact that `PhabricatorConfigLocalSource` seems to simply ignore `local.json` is it isn't readable, whereas throwing an `Exception` would have saved me a lot of debugging.
Test Plan:
```name=Before
> /usr/local/src/phabricator/bin/config get mysql.pass
{
"config": [
{
"key": "mysql.pass",
"source": "local",
"value": null,
"status": "unset",
"errorInfo": null
},
{
"key": "mysql.pass",
"source": "database",
"value": null,
"status": "error",
"errorInfo": "Database source is not configured properly"
}
]
}
```
```name=After
> /usr/local/src/phabricator/bin/config get mysql.pass
[2017-05-16 21:49:26] EXCEPTION: (FilesystemException) Path '/usr/local/src/phabricator/conf/local/local.json' is not readable. at [<phutil>/src/filesystem/Filesystem.php:1124]
arcanist(head=stable, ref.master=3c4735795a29, ref.stable=20ad47f27331), phabricator(head=stable, ref.master=3dae9701298f, ref.stable=fcebaa5097f3), phutil(head=stable, ref.master=a900d7b63e95, ref.stable=d02cc05931b0)
#0 Filesystem::assertReadable(string) called at [<phutil>/src/filesystem/Filesystem.php:39]
#1 Filesystem::readFile(string) called at [<phabricator>/src/infrastructure/env/PhabricatorConfigLocalSource.php:25]
#2 PhabricatorConfigLocalSource::loadConfig() called at [<phabricator>/src/infrastructure/env/PhabricatorConfigLocalSource.php:6]
#3 PhabricatorConfigLocalSource::__construct() called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:195]
#4 PhabricatorEnv::buildConfigurationSourceStack(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:95]
#5 PhabricatorEnv::initializeCommonEnvironment(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:75]
#6 PhabricatorEnv::initializeScriptEnvironment(boolean) called at [<phabricator>/scripts/init/lib.php:22]
#7 init_phabricator_script(array) called at [<phabricator>/scripts/init/init-setup.php:11]
#8 require_once(string) called at [<phabricator>/scripts/setup/manage_config.php:5]
```
Reviewers: #blessed_reviewers, joshuaspence
Reviewed By: joshuaspence
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17917
Summary: It's an icon. For story points.
Test Plan: Set some points, see icon.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17915
Summary:
Ref T12616. Fixes T11648. Currently, we snug up replies with a negative margin (from T10563) but this throws off the anchor highlighting.
Instead:
- Remove padding from these dolumns.
- Use margins on the stuff inside them instead.
- Less margins for replies.
- Less margins for collapsed comments.
- Show some text for collapsed comments.
Test Plan:
{F4960890}
{F4960891}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11648
Differential Revision: https://secure.phabricator.com/D17913
Summary:
Fixes T8420. Now that hidden inlines no longer fold into a big clump, anchors can just jump to them in a normal way.
Move the anchors up a smidge so thing work.
Test Plan: Clicked an anchor pointed at a hidden inline, ended up in the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8420
Differential Revision: https://secure.phabricator.com/D17910
Summary: Run through all the pages in projects and make sure they all feel similar. Adds back curtain on board manage page, even though it is sad for only having a single action.
Test Plan: Test all pages on a project for consistency in UI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17909
Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)
Now that the code is in a better place:
- Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
- Click again to deselect it.
- You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
- This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.
Also, make "Reply" render more consistently.
Test Plan:
- Did all that stuff, things seemed to work OK.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12715, T12616
Differential Revision: https://secure.phabricator.com/D17908
Summary: Cleans up the UI, moves details over to curtain, adds some fallback no data strings.
Test Plan: Review with and without subprojects, milestones.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17907
Summary: We seem to already support this, just takes it fully there. We don't need to see things like "Flag", etc, on certain subpages of projects/people/etc.
Test Plan: Review Members, Subproject pages, no longer see "Flag for Later" which only is for the Project itself. Check manage, still there.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17897
Summary:
Fixes T8130. Allows selected comments to be shown/hidden (with "q") or marked done/not-done (with "w").
(These key selections are because "qwer" are right next to each other on QWERTY keyboards, and now mean "hide, done, edit, reply".)
Also, allow "N" and "P" to do next/previous inline, including hidden inlines. This makes "q" to hide/show a little more powerful and a little easier to undo.
Test Plan: Used "q", "w", "N" and "P" to navigate and interact with comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8130
Differential Revision: https://secure.phabricator.com/D17906
Summary: Restricts the view of the membership privileges to just the Members page itself, and not other pages like Home/Details.
Test Plan: Test Home, Test Members, see correct layouts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17896
Summary: This UI can use the setDrag call to reduce clutter on the reodering dialog.
Test Plan:
Reorder some columns, save.
{F4959906}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17898
Summary: Ref T12616. This makes "edit" and "reply" work again.
Test Plan:
Used "e" and "r" to edit and reply.
Also used them in bogus ways and got useful UI feedback.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17895
Summary: Ref T12616. This moves most keyboard shortcuts into DiffChangesetList. It breaks some shortcuts that I plan to restore later, noted in T12616 (toggle file, edit inline, reply to inline), since I think ripping them out now and rebuilding them in a little bit will make things much simpler.
Test Plan:
- Used j, k, n, p, J, K shortcuts to navigate a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17859
Summary:
Ref T12616. This moves "reply" to the new stuff and deletes DifferentialInlineEditor, which no longer does anything.
(This breaks some keyboard shortcuts, but I'll rebase D17859 shortly.)
Test Plan: Replied to inlines; things seemed to work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17894
Summary:
Ref T12616. This updates clicking the "Done" checkbox for the new stuff.
This one is pretty clean since the "Done" checkbox doesn't do too much weird magic.
Test Plan: Clicked the box a few times.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17888
Summary:
Ref T12616. Fixes T12153. Currently, when you hide inlines, they hide completely and turn into a little bubble on the previous line.
Instead, collapse them to a single line one-by-one. Narrowly, this fixes T12153.
In the future, I plan to make these changes so this feature makes more sense:
- Introduce global "hide everything" states (T8909) so you can completely hide stuff if you want, and this represents more of a halfway state between "nuke it" and "view it".
- Make the actual rendering better, so it says "epriestley: blah blah..." instead of just "..." -- and looks less dumb.
The real goal here is to introduce `DiffInline` and continue moving stuff from the tangled jungle of a million top-level behaviors to sensible smooth statefulness.
Test Plan:
- Hid and revealed inlines in unified and two-up modes.
- These look pretty junk for now:
{F4948659}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T12153
Differential Revision: https://secure.phabricator.com/D17861
Summary: Ref T12616. This cements the relationship between ChangesetList (parent container) and Changeset (child) and passes translations down so Changeset can use them to translate the text "Loading..."
Test Plan: Viewed loading changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17846
Summary: Ref T12616. This ends up being a little messy ("one giant function") and maybe I'll clean it up a bit later, but continue consolidating the wild jungle of behaviors into a smaller set of responsible objects.
Test Plan: Clicked all the menu options, saw them work properly. Grepped for removed methods.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17845
Summary:
Ref T12616. Diffusion, only, has a "Show All Context" button which expands the full context on all changes.
I don't remember the exact history on this, but it hasn't existed in Differential for some time and no one has complained. I suspect that the "View Options > Show All Context" on each file may replace it. I can't really come up with good reasons to use it, offhand. If we want to restore it, I think global options after T1591 is promising.
{F4945561}
Test Plan:
- Loaded a commit in Diffusion, no longer saw a button.
- Grepped for relevant sigils.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17843
Summary: Also cleans up now-dead code relating to old transactions
Test Plan: Created lots of mocks, replaced their images, added/removed images, changed the sequence, verified expected DB xaction rows, Mock updates, and correct rendering of timeline.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17892
Summary: Various little fixes, mostly moves information from the "Details" section either into the curtain or into the specific watchers or members list based on user viewership. I think this page is both cleaner and more informative.
Test Plan:
Lock, Unlock, Watch, Join, various projects with multiple users.
{F4959101}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17891
Summary: Removal of `PholioMockEditor::applyCustomInternalTransaction()` in D17868 broke creation of new mocks in Pholio. Puts the empty method back until we finish migrating Pholio to modular transactions.
Test Plan: Created some mocks, observed lack of unhandled exception.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17889
Summary: I think this is the correct fix, sets a consistent value for transactions, old and new, for Maniphest point values.
Test Plan:
Edit title, see no point feed story, set points, see point story, set points to same value, see no story, remove points, see remove point story.
{F4958233}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17885
Summary: Fixes T12713. We don't need to show watching and member info on other views other than ApplicationSearch (for now) so add a few methods to restrict the calls.
Test Plan: Visit project search, profile, project home, project home with subprojects
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12713
Differential Revision: https://secure.phabricator.com/D17883
Summary: Slightly nicer, more consistent UI. Also removed "Column History" from dropdowns as this is available on the general board manage page.
Test Plan: Review Board and Column management pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17881
Summary:
Fixes T12710. See that task for discussion. This is pretty ugly/redundant but not broken.
(Feel free to reject this and pursue something else.)
Test Plan:
- For a project with active subprojects/milestones, viewed the project profile and subprojects tabs.
- After patch: they're ugly, but no longer fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12710
Differential Revision: https://secure.phabricator.com/D17882
Summary: Moves "reorder columns" and "change background" up a level, redesigns "manage" page to be a little cleaner.
Test Plan: Change colors, reorder columns, manage page, disable board, re-enable board.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17879
Summary: Fixes T12707 Adds additional information to search results for if user is a member or a watcher of a project. Also removed the icon colors, which I'll find a better way to denote in future.
Test Plan: Join a project, watch a project, view results list in /projects/
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12707
Differential Revision: https://secure.phabricator.com/D17880
Summary:
Ref T12685.
- Better icon/color/label consistency.
- Make "0" a valid response.
- Fix a bug where creating a poll with Quicksand enabled led to bad times (form submitted back to the search screen).
Test Plan: {F4956412}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17878
Summary:
Ref T12685. When you edit an application's policies but don't make any changes, you currently get stuck on the same page. This isn't how other edit screens work, and I think it's a holdover from eras long ago.
Make it consistent with other applications.
Also, fix a missing `pht()`.
Test Plan:
- Edited an application configuation.
- Clicked "Save" without making changes.
- After patch, was redirected back to detail page like in other applications.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17877
Summary:
Ref T12685. I provided this incorrect (`return new` rather than `throw`) implementation earlier; it can now be replaced with a proper implementation.
This caused application policy edits to spew this into the daemon log:
```
[2017-05-14 15:35:27] EXCEPTION: (Error) Call to undefined method PhutilMethodNotImplementedException::setActor() at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:69]
```
Test Plan:
- Used `bin/worker execute --id <id>` to execute a previously-failing task.
- Saw a feed story publish.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17876
Summary: This brings up "Edit Column" as an action item under the main column dropdown as well as a "Column History" for completeness. Unsure column history is actually useful, but leaving it in anyways. It might be nice to have some sort of dialog version of a history page.
Test Plan: Make a workboard, add a column, edit column name, stay on workboard.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17874
Summary:
Fixes T12679. Reproduction steps appear to be:
- As a logged-out user, view revision list or commit list.
- Enable bucketing by action required.
- Before patch: `foreach (null as ...)` causes error spew.
- After patch: `foreach (array() as ...)` works great.
Test Plan:
- Reproduced issue by following steps above in Differential (revisions) and Diffusion (audits/commits).
- After patches, no more errors in the log.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12679
Differential Revision: https://secure.phabricator.com/D17872
Summary: Ref T12707. Adds a simple filter for the viewer if logged in.
Test Plan: Watch a project, click on watching list, see project I'm watching.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley
Maniphest Tasks: T12707
Differential Revision: https://secure.phabricator.com/D17873
Summary: Also removes more now-dead code from `PholioTransaction`.
Test Plan: opened and closed a bunch of mocks, edited a bunch of image descriptions
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17868
Summary: Caught this in the logs, calling an old transaction, update it.
Test Plan: Answer a question, tail log, see no error.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17870
Summary: Trailing logs for errors here, picked up some unset constants in Phame.
Test Plan: New Blog, New Post, tail daemon log for errors. See feed stories.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17871
Summary: The ports over a similar "profile image" menu item to Projects. It gives us some room to use the project icon in the sidenav along with a larger photo. It also will open up some room in the sub-page headers for us to focus on that page, and not the identity of the project at hand. Expect a few more project related touch up diffs.
Test Plan:
Review new projects menu on a few projects, update the image, see new image. Great for team photos.
{F4951264}
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17869
Summary: Another Franken-transaction. Adds transactions for ImageName and MockName. Adds transaction-level validation for presence of mock name; removed same from edit controller. Removes `PholioTransaction::getRemarkupBodyForFeed()`, which appears to be dead code since that method isn't defined on any other types.
Test Plan: made a bunch of changes to pholio mocks and images and observed expected results
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, chad, epriestley
Differential Revision: https://secure.phabricator.com/D17865
Summary: Updates Legalpad to use EditEngine, paving the way for //transaction comments//. Spooky.
Test Plan:
- New Document
- Require signing, Corp - see fail
- Require signing, Noone - see fail
- Require signing, Ind - get asked to sign
- Edit Document
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17862
Summary: Begins the process of migrating Pholio to Modular Transactions by starting with the mock's description and changing the base class of PholioTransaction. Expect several more of these diffs to quickly follow. Also changes the icon for description changes to `fa-pencil`; previously it was check or ban, depending on the open/closed state. Looks like an accidental switch fallthrough.
Test Plan: made new mocks, edited their descriptions, observed same UI as previously
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17864
Summary: This function `renderHandleLink` doesn't exist. Update call.
Test Plan: I'm still lost on how to refund a transaction? Existing?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17858
Summary: Ref T12685. Moves Fund to EditEngine commenting.
Test Plan: View an old Fund, leave a comment, add a subscriber.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17857
Summary: Ref T12685, updates fund for edit engine.
Test Plan: Create a Fund, Edit a Fund, wipe out Merchants, check errors for name and missing merchants, back Fund.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17855
Summary: Ref T12685, cleans up various macro issues, remove subscribers, fix feed stories, etc.
Test Plan: Create a new macro, see no subscribers, edit various macros.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17848
Summary:
See D17848. This improves things a little bit in two cases:
Case 1:
- Create a macro.
- Pick a valid file.
- Pick an invalid name.
- Submit form.
- Before patch: your file is lost and you have to pick it again.
- After patch: your file is "held" in the form, you just can't see it in the UI. If you submit again, it keeps the same file. If you pick a new file, it uses that one instead.
Case 2:
- Apply D17848.
- Delete the `if ($value) {` thing that I'm weirded out about (see inline).
- Edit a macro.
- Don't pick a new file.
- Before patch: error, can't null the image PHID.
- Afer patch: not picking a new file means "keep the same file", but you can't tell from the UI.
Basically, the behaviors are good now, they just aren't very clear from the UI since "the field has an existing/just-submitted value" and "the field is empty" look the same. I think this is still a net win and we can fix up the UI later.
Test Plan: See workflows above.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17853
Summary: Ref T12685. Moves to `xaction` folder and sets description changes in transaction stories.
Test Plan: Make a poll, edit the description.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17849
Summary: Ref T12685. Checks merchant capabilities at the edit engine level
Test Plan: Test with and without admin level permissions. Get restricted access if I cheat. Still able to create with admin.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17852
Summary: Ref T12685. Makes the description field full remarkup and fixes setting a credential secret after destruction.
Test Plan: Change description a lot, set and destroy credentials.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17851
Summary: Ref T12685. Sets required on required fields, cleans up mailtags on PhamePost
Test Plan: Create a new blog, post.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17850
Summary: Fixes T12682.
Test Plan: Ran `bin/storage upgrade --dryrun` repeatedly with un-applied patches, saw it not apply them and not mark them applied.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12682
Differential Revision: https://secure.phabricator.com/D17837
Summary: This was mis-tested by only using one account, which could always see the image. External transaction moved file attachment to the modular transaction for file and audio instead.
Test Plan: Test adding audio and a macro on a pleb account, visit with normal account and see macro fine.
Reviewers: epriestley, amckinley
Reviewed By: amckinley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17836
Summary: Not sure when these stopped, also fixed mailtag contants.
Test Plan: Close an initiative, see story, fund initiative, see story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17835
Summary: Fixes T12627. Updates FundInitiative and FundBacker with modular transactions.
Test Plan: Create an Initiative, back it with fake monies, close initiative, reopen, edit various fields.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12627
Differential Revision: https://secure.phabricator.com/D17782
Summary:
Fixes T12623. Adds new modular transactions to Slowvote. Also converts
the `shuffle` column to `bool` for consistency with other boolean-ish columns.
Test Plan:
Create a new vote, modified everything that could be modified from the web UI,
observed expected timeline.
Example timeline: {F4938843}
Example transaction values in DB: {F4938850}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T12623
Differential Revision: https://secure.phabricator.com/D17830
Summary:
We are submitting `epriestley (Evan Priestley) <noreply@meta.phacility.com>`, but should be submitting `"epriestley (Evan Priestley)" <noreply@meta.phacility.com>`.
Add the missing quotes.
Test Plan: Locally, this makes the API calls work against the Mailgun sandbox domain.
Reviewers: chad, amckinley
Reviewed By: chad, amckinley
Differential Revision: https://secure.phabricator.com/D17831
Summary: Updates the Spaces application for modular transactions, seemed easy to bang out.
Test Plan: Create a space, edit a space, archive a space. Verify default space works as intended.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17829
Summary: Update Legalpad for modular transactions
Test Plan:
- New Document (no sign)
- New Document (individual)
- New Document (corp)
- Require Signature - get prompted to sign before I can do anything.
- Edit Documents
- Sign Documents
- Comment on Documents
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17826
Summary: Updates Passphrase for modular transactions.
Test Plan: Create, edit, lock, view, lots of different types of Passphrases. Enable Conduit, Lock Passphrases, Destroy Secrets from the interface and verify from the DB it was eradicated.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17824
Summary: Still needs some cleanup, but ready for review in broad outline form.
Test Plan:
Made lots of policy changes to the Badges application and confirmed expected rows in `application_xactions`, confirmed expected changes to `phabricator.application-settings`.
See example output (not quite working for custom policy objects) here:
{F4922240}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, chad, epriestley
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17757
Summary: Moves over to transaction commenting.
Test Plan: Leave a comment (tested with TYPE_COMMENT still present).
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17823
Summary: See D17812, etc. We can figure this out by looking at the object carefully. We don't need to go delete all the old TYPE_COMMENT (it doesn't hurt anything) but can nuke it when we see it.
Test Plan:
- Made a comment in Slowvote (supports commenting).
- Viewed an Almanac device (does not support commenting).
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17822
Summary: Just a small touch up to move this to edit engine.
Test Plan:
- Create a question
- Edit a question
- Close question
- Test NUX state
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17812
Summary: Updates PhamePost for modular transactions.
Test Plan:
- Create a post
- Edit a post
- Add a header image
- Delete header image
- Award Token
- Leave comment
- Unpublish post
- Check History page
- Move post
- Archive post
{F4936456}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17818
Summary: Updates Macro to use EditEngine. Also removes "URL" field for adding a Macro, which I think it's worth pursuing.
Test Plan:
- Create a Macro
- Forget to name it
- Try a PDF
- Use a Macro
- Edit a macro (not working)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17821
Summary: Moves PhameBlog over to the wonderful world of modular transactions and the riches that lay beyond...
Test Plan:
- Create Blog
- Edit Blog
- Set Header
- Delete Header
- Add picture
- Archive blog
- Set incorrect domain values
- Be irresponsible with subtitle length
- Activate blog
- Change description
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17815
Summary:
Fixes T12661.
When changing the start date of an event from some time in the past to some time significantly in the future (more than 24 hours), we'd invalidate only future caches and leave users in an "away" state. Instead, just invalidate all past and future caches (this is simpler than trying to figure out a narrower window, and should not make us do too much extra work).
When uninviting users from events, their caches also didn't get cleared correctly. Instead, clear them.
Test Plan:
- Changed an event from "Apr 1 - June 1" to "May 15 - June 1", saw availablity clear properly.
- Uninvited user `@dog` from an ongoing event, saw availability clear properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12661
Differential Revision: https://secure.phabricator.com/D17809
Summary: T12656, mark these methods as frozen and use conpherence.edit instead.
Test Plan: Visit conduit, check status is displayed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17808
Summary: Fixes T12659. Previously this would lead to an error when trying to run `arc diff` on a revision that had a milestone as a reviewer (or any non-octothorpe'd Object Name)
Test Plan: Followed repro steps in T12659 and didn't get the error described. Also clicked around and didn't notice any obvious regressions in projects or differential
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, yelirekim
Maniphest Tasks: T12659
Differential Revision: https://secure.phabricator.com/D17807
Summary:
Fixes T12642. Currently, writing "Fixes T..." in a comment gets picked up as a formal "fixes".
This is a bit confusing, and can also give you a "no effect" error if you "fixes ..." a task which is already "fixes"'d.
We could make the duplicate action a non-error, but just prevent the text from having an effect instead, which seems cleaner.
Test Plan:
- Wrote "Fixes ..." in a summary, saw a "fixes" relationship established.
- Wrote "Fixes ..." in a comment, got a "mention" instead.
- `var_dump()`'d some stuff as a sanity check, looked reasonable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12642
Differential Revision: https://secure.phabricator.com/D17805
Summary: Builds a basic room generator for Conpherence, picks a random name, adds 10 random users to it, sets view and edit policy to all users.
Test Plan:
`bin/lipsum generate conpherence`
{F4928815}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17699
Summary: Moves Conpherence to use EditEngine. This removes the "First Message" field, but I think that's ok until we have direct messaging of some sort, then maybe have built-ins cover that case.
Test Plan:
- Visit /new/ and /edit/ for creating new rooms.
- Edit a room in full conpherence
- Edit a room in durable column
- grep for METADATA calls
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11729
Differential Revision: https://secure.phabricator.com/D16677
Summary:
- Change column type from `sort128` to `sort`.
- Remove `originalName`. This column is unused. Long ago, we used it to generate a `Thread-Topic` header for mail, but just use PHIDs now (the value just needs to be stable for a given object, users normally don't see it).
Test Plan:
- Created a package with a beautifully long name. Magnificent!
- Grepped for `originalName` / `getOriginalName()`, found no Owners hits.
- Verified that there isn't any name-length validation code to remove.
{F4925637}
Reviewers: chad, amckinley
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17798
Summary:
Ref T12646.
- Use "wb1" instead of "wb" to use level 1 gzip compression (faster, less compressy). Locally, this went about 2x faster and the output only grew 4% larger.
- LinesOfALargeExecFuture does a lot of unnecessary string operations, and can boil down to a busy wait. The process is pretty saturated by I/O so this isn't the end of the world, but just use raw ExecFuture with FutureIterator so that we wait in `select()`.
- Also, nice the process to +19 so we try to give other things CPU.
Test Plan:
- Ran `bin/storage dump --compress --output ...`.
- Saw CPU time for my local database drop from ~240s to ~90s, with a 4% larger output. Most of this was adding the `1`, but the ExecFuture thing helped a little, too.
- I'm not sure what a great way to test `nice` in a local environment is and it's system dependent anyway, but nothing got worse / blew up.
- Used `gzcat | head` and `gzcat | tail` on the result to sanity-check that everything was preserved.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12646
Differential Revision: https://secure.phabricator.com/D17795
Summary: Ref T12600. Basically all the property (not path) information on a hovercard for owner packages.
Test Plan:
Create a package with LOTS OF RULES. Test it as open and archived states.
{F4923441}
{F4923444}
Reviewers: epriestley, jmeador
Reviewed By: jmeador
Subscribers: jmeador, Korvin
Maniphest Tasks: T12600
Differential Revision: https://secure.phabricator.com/D17793
Summary: When a notice is in a table view in a two column layout, reset the margins.
Test Plan: Visit OwnerDetails with no paths set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17792
Summary: I can't believe I've been staring at this page for so long without noticing this typo
Test Plan: doitlive
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17791
Summary:
Ref T12613. Currently, the SVNTEST and HGTEST repositories are improperly configured on `secure`. These repositories use VCS systems which do not support synchronization, so they can not be served from cluster services with multiple hosts.
However, I've incorrectly configured them the same way as all the Git repositories, which support synchronization. This causes about 50% of requests to randomly fail (when they reach the wrong host).
Detect this issue and warn the user that the configuration is not valid.
It should be exceptionally difficult for normal installs to run into this.
Test Plan:
- Mostly faked these conditions locally, verified that `secure` really has this configuration.
- I'll push this, verify that the issue is detected correctly in production, then fix the config which should resolve the intermittent issues with SVNTEST.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12613
Differential Revision: https://secure.phabricator.com/D17774
Summary:
Ref T12635. See that task for discussion.
You can currently end up with a verified primary address but no "verified" flag on your account through an unusual sequence of address mutations.
Test Plan:
- Registered without verifying, using address "A".
- Added a second email address, address "B".
- Verified B (most easily with `bin/auth verify`).
- Changed my primary email to B.
- Before patch: account not verified.
- After patch: account verified.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12635
Differential Revision: https://secure.phabricator.com/D17785
Summary: Swaps out hovercard boring view for super cool workboard card view. Will have more diffs to add additional information down the road.
Test Plan: {F4921092}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17769
Summary:
Ref T12611. Currently, the HTTP/SSH logs don't have an option to include the instance name.
Add such an option.
Leave it out of the default logs because most installs don't use this.
Test Plan: See next changes.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12611
Differential Revision: https://secure.phabricator.com/D17776
Summary: Fixes T12614.
Test Plan:
- Ran tests, saw them pass.
- Changed a thumbnail to 800x800, saw tests detect that the default image was missing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12614
Differential Revision: https://secure.phabricator.com/D17773
Summary: Ref T12622.
Test Plan: As a logged-out and logged-in user, loaded Conpherence threads.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12622
Differential Revision: https://secure.phabricator.com/D17768
Summary: Fixes T12622. This variable is mis-named.
Test Plan: Visit a room I have not joined.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12622
Differential Revision: https://secure.phabricator.com/D17767
Summary:
Ref T12612. This updates the rate limiting code to:
- Support a customizable token, like the client's X-Forwarded-For address, rather than always using `REMOTE_ADDR`.
- Support APCu.
- Report a little more rate limiting information.
- Not reference nonexistent documentation (removed in D16403).
I'm planning to put this into production on `secure` for now and then we can deploy it more broadly if things work well.
Test Plan:
- Enabled it locally, used `ab -n 100` to hit the limit, saw the limit enforced.
- Waited a while, was allowed to browse again.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12612
Differential Revision: https://secure.phabricator.com/D17758
Summary: Fixes T12619.
Test Plan: Faked `return array()` in ConpherneceThreadQuery, got a NUX instead of fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12619
Differential Revision: https://secure.phabricator.com/D17764
Summary:
Closes T7829 as wontfix. Closes T7965 as wontfix. Closes T7800 as wontfix. Closes T2731 as wontfix. Closes T1271 as wontfix.
We aren't maintaining this at all (see, e.g., T7829) and a user reported a technically accurate security issue via HackerOne: <https://hackerone.com/reports/222870>
Just throw it away until we get to the eventual Conphernece bot/API update and can do this stuff correctly.
Test Plan: Grepped for `phabricatorbot`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7965, T7829, T7800, T2731, T1271
Differential Revision: https://secure.phabricator.com/D17756
Summary: Uses more standard objects and more padding for reading. Removes the ToC, which is visually broken anyways.
Test Plan: Review a README.md in a local repository.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17752
Summary: Adds the ability to set a pager onto an object box directly and pick up appropriate styles.
Test Plan: grep for renderTablePagerBox, test layouts with and without a pager.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12604
Differential Revision: https://secure.phabricator.com/D17754
Summary: This adds some basic per user / per room theming for Conpherence, which should hopefully let users identify rooms from just the sidebar color.
Test Plan: Lots of threads with different colors.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17747
Summary: Makes it more clear whose authority actions have been taken under.
Test Plan: {F4916376}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17741
Summary: Ref T12591. These preferences are per user and we need to reload the whole UI on a change anyways. Simplifies future expansion.
Test Plan: Option click into new window, set preferences. View in Conpherence, see saved settings, change sound. Hear new sound.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12591
Differential Revision: https://secure.phabricator.com/D17740
Summary:
Ref T11476. This is a bit hacky, but makes `Application` extend `LiskDAO` so we can apply transactions to it with an `Editor` class.
Also fixes schema stuff so builds should produce a clean bill of health again.
This might only get you slightly further, yell if you run into more trouble.
Test Plan:
- Ran `bin/storage upgrade -f` and got no warnings.
- Browsed around, nothing exploded?
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17738
Summary: Part of the groundwork for T11476.
Test Plan: ran `./bin/storage upgrade` and observed expected DB tables
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17736
Summary: Sets notification and sound preferences in a single array()
Test Plan: Change email preference, save, set sound preference, save. Email preference still OK.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17735
Summary: Moves participants over to ModularTransactions, simplified a lot of the code. Fixes T12550
Test Plan:
Create a new room with just myself and myself + fake accounts.
Remove a person.
Remove myself.
Edit a room, topic.
Type some messages.
???
Profit
Reviewers: chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T12550
Differential Revision: https://secure.phabricator.com/D17685
Summary:
Pathway to D17685. We no longer have "behindTransactionPHID", so we no longer need the latest transaction.
This allows some code to be removed.
Test Plan:
- Grepped for callsites to `markUpToDate()` and variables used in the calls.
- Sent messages in a couple threads, viewed them, saw unread counts go away.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17733
Summary:
Pathway to D17685. This fixes an issue idenified in D17731: if any caller ever queried for more than one participant, some results could get thrown away by re-keying the results on thread PHID: two different participants can be members of the same thread!
This also fixes an issue from D17683, where a `needParticipantCache()` callsite was overlooked.
Test Plan:
- Viewed Conpherence dropdown.
- Sent messages, saw unread count / thread order still work properly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17732
Summary:
Pathway to D17685. This column is (mostly) a denormalization of `dateModified` on the thread.
Just use a JOIN instead.
This isn't //exactly// the same: we'll bump threads to the top now for non-message changes (e.g., a topic or title change). That seems fine, but we could put a `lastMessageDate` on Thread later if we want to refine it.
Also got rid of a lot of other unused stuff. There's a big garbage TODO here, I'll fix that in the next change.
Test Plan:
- Grepped for `dateTouched`.
- Grepped for `participantCursor`.
- Grepped for `ConpherenceParticipantQuery::LIMIT`.
- Looked for callsites to `setOrder()`, found none.
- Added a message to an older thread, saw it bump up to the top.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17731
Summary:
Pathway to D17685. This column is a very complicated cache of: is participant.messageCount equal to thread.messageCount?
We can just ask this question with a JOIN instead and simplify things dramatically.
Test Plan:
- Ran migration.
- Browsed around.
- Sent a message, saw unread count go up.
- Read the message, saw unread count go down.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17730
Summary: Pathway to D17685. Nothing reads this field and it has no use or value.
Test Plan:
- Ran migration.
- Grepped for `behindTransactionPHID`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17729
Summary: A few more mp3s to choose from for Conpherence.
Test Plan: Test each sound in a new room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17734
Summary: Ref T7567. This adds some constants (for adding new sounds), global setting for turning on and off sound (setting) and per thread preference for sound choice. Also specc'd out Mentions, if added.
Test Plan: I tested all the preference wiring, but need to set up notifications locally to verify if this works. Feel free to test.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: amckinley, Korvin
Maniphest Tasks: T7567
Differential Revision: https://secure.phabricator.com/D17726
Summary:
Fixes T12596. A query for a token (like "having") which stems to a stopword (like "have") currently survives filtering. Stem it first so it gets caught.
Also, for InnoDB, a custom stopword table can be configured. If it is, read that instead of the default stopword list (I configured it locally, but the default list is reasonable so we never formally recommended installs configure it).
Test Plan:
Queried for words that stem to stopwords, saw them filtered:
{F4915843}
Queried for the original problem query and saw "having" caught with "have" in the stopword list:
{F4915844}
Fiddled with local InnoDB stopword table config and saw the stopword list get loaded correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12596
Differential Revision: https://secure.phabricator.com/D17728
Summary:
Ref T7567. In T8266 I fixed a bunch of obscure "Range" issues, but only for file downloads -- not for Celerity.
Extend all that stuff to Celerity, which is fortunately much easier.
I believe this will fix Conpherence sounds in Safari.
Test Plan:
- Wrote out an HTTP request in a text file with `Range: bytes=0-1` and similar, piped it to localhost with `cat request.txt | nc localhost 80`, saw server return appropriate range responses consistent with file behavior after T8266, which all seems to work.
- Also did that for files to try to make sure I wasn't breaking anything.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T7567
Differential Revision: https://secure.phabricator.com/D17724
Summary: Fixes T12587. Adds a new `PhabricatorFileDeleteTransaction` that enqueues `File` delete tasks.
Test Plan:
- hack `PhabricatorFileQuery` to ignore isDeleted state
- stop daemons
- upload a file, delete it from the UI
- check that the DB has updated isDeleted = 1
- check timeline rendering in `File` detail view
- start daemons
- confirm rows are deleted from DB
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, thoughtpolice
Maniphest Tasks: T12587
Differential Revision: https://secure.phabricator.com/D17723
Summary:
Ref T12566. Ref T12563. This fixes three bugs with Aphlict replay stuff:
First, Conphernece would try to repaint the UI even if no thread was open. Only repaint when a thread is open.
Second, although we deduplicate JX.Leader messages, we didn't deduplicate actual notification messages. If you browsed the leader window, then it re-elected itelf as a leader and replayed history, it could rebroadcast notifications and other windows could show doubles. Deduplicate notifications to prevent this.
Third, we always replayed the last 60 seconds of history. When you browsed the leader window, whichever window became the new leader (possibly the one you just browsed) could replay messages from before it had opened, leading to duplicate messages. Particularly, after receiving a message and then browsing you could see that message again. Instead, only replay history as far back as when the window first opened.
Test Plan:
- Clicked "Repaint" with a thread open, saw a repaint. Clicked "Repaint" with Conpherence open but no thread, no repaint and no 404 request to `/update/null/`.
- In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away twice in a row. Observed that the window which never became a leader doesn't duplicate notifications.
- In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away over and over again. Observed that replay requests issued with appropriate history windows.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12566, T12563
Differential Revision: https://secure.phabricator.com/D17722
Summary:
Deletion is a possibly time-intensive process, especially with large
files that are backed by high-latency, chunked storage (such as
S3). Even ~200mb objects take minutes to delete, which makes for an
unhappy experience. Fixes T10828.
Test Plan:
Delete a large file, and stare in awe of the swiftness with
which I am redirected to the main file application.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: thoughtpolice, Korvin
Maniphest Tasks: T10828
Differential Revision: https://secure.phabricator.com/D15743
Summary: Fixes T12579. Unclear why the user ran this command.
Test Plan: Ran with `--id cat`. Ran with `--id 123`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12579
Differential Revision: https://secure.phabricator.com/D17719
Summary: Ensures that newly-made `File` objects get indexed into the new ngrams index. Fixes T8788.
Test Plan:
- uploaded a file with daemons stopped; confirmed no new rows in ngrams table
- started daemons; confirmed indexing of previously-uploaded files happened
- uploaded a new file with daemons running; confirmed it got added to the index
Not sure how to test the changes to `PhabricatorFileUploadSource->writeChunkedFile()` and `PhabricatorChunkedFileStorageEngine->allocateChunks()`. I spent a few minutes trying to find their callers, but the first looks like it requires a Diffusion repo and the 2nd is only accessible via Conduit. I can test that stuff if necessary, but it's such a small change that I'm not worried about it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8788
Differential Revision: https://secure.phabricator.com/D17718
Summary: Follows the outline in D15656 for implementing ngram search for names of File objects. Also created FileFullTextEngine, because without implementing `PhabricatorFulltextInterface`, `./bin/search` complains that `File` is not an indexable type.
Test Plan:
- ran `./bin/storage upgrade` to apply the schema change
- confirmed the presence of a new `file_filename_ngrams` table
- added a couple file objects
- ran `bin/search index --type file --force`
- confirmed the presence of rows in `file_filename_ngrams`
- did a few keyword searches and saw expected results
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8788
Differential Revision: https://secure.phabricator.com/D17702
Summary: Check the strlen of topic before adding a tag to the header in Conpherence.
Test Plan: Remove a topic, no longer see indigo bubble.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17715
Summary: Ref T12566. When we reconnect, refresh the current thread even if we replayed notifications.
Test Plan:
- Clicked the "Repaint" button, saw the thread refresh.
- Clicked the "Reconnect" button, saw the thread reresh.
- Launched `aphlict debug`, killed it, restarted it, saw the thread refresh after reconnect.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12566
Differential Revision: https://secure.phabricator.com/D17713
Summary:
Fixes T12564. We already had some code which seems to deal with this properly, it just wasn't getting used.
Assign each application-level notification a unique ID, then ignore messages with duplicate IDs.
Test Plan:
- In browser A, loaded `/T123`.
- In browser B, loaded `/T123`.
- Made a comment as B.
- Saw notification as A.
- Mashed "Replay" a bunch.
- Before patch: piles of duplicate notifications.
- After patch: no duplicates.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12564
Differential Revision: https://secure.phabricator.com/D17710
Summary: Chrome and Safari both zoom in on form (input, select, textarea) when it thinks the text is too small (less than 16px... which is huge). This turns user-scalable off. The only drawback is double-tap to zoom will be disabled as well, but given we already responsively design, I don't think thats an issue.
Test Plan: iOS simulator on secure and local test instances. Click on an input, no longer see UI zoom in.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17714
Summary:
Fixes T12563. If we've ever seen an "open", mark all future connections as reconnects. When we reconnect, replay recent history.
(Until duplicate messages (T12564) are handled better this may cause some notification duplication.)
Also emit a reconnect event (for T12566) but don't use it yet.
Test Plan: {F4912044}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12563
Differential Revision: https://secure.phabricator.com/D17708
Summary:
Ref T12563. Before broadcasting messages from the server, store them in a history buffer.
A future change will let clients retrieve them.
Test Plan:
- Used the web frontend to look at the buffer, reloaded over time, sent messages. Saw buffer size go up as I sent messages and fall after 60 seconds.
- Set size to 4 messages, sent a bunch of messages, saw the buffer size max out at 4 messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12563
Differential Revision: https://secure.phabricator.com/D17707
Summary: Ref T12568. Ref T12567. Allows you to force a reconnect, and shows the reconnect delay on connection close/failure.
Test Plan: {F4911879}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12568, T12567
Differential Revision: https://secure.phabricator.com/D17705
Summary: Ref T12568. This begins building toward a more useful realtime debugging console for Leader/Aphlict/general realtime stuff.
Test Plan: {F4911521}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12568
Differential Revision: https://secure.phabricator.com/D17701
Summary: Ref T8788. See D17702. This allows `bin/search index` to index stuff which only implements `Ngrams`, not `Fulltext`.
Test Plan: Kinda poked around `bin/search index` a bit, yell if you hit more issues deeper down the stack?
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8788
Differential Revision: https://secure.phabricator.com/D17704
Summary: Fixes T12556 Uses more common components in ConpherenceThreadList by moving to PHUIListItemView. Reduces clutter by moving privacy into the header. Gets ride of "See More" double interchanges.
Test Plan:
I need to test this more, doesn't seem to auto-select top room any more, also might build a lipsum generator.
- Create lots of rooms with various policies
- Test clicking on policy object
- Click on different rooms
- Post in rooms
- Load up second account, see room numbers
- Clear room message count by clicking on room
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12556
Differential Revision: https://secure.phabricator.com/D17698
Test Plan: Created a phurl, added some comments, confirmed that "Change Subscribers" and "Change Project Tags" are now available in the comment form.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, Korvin
Maniphest Tasks: T11661
Differential Revision: https://secure.phabricator.com/D17686
Summary: Updates the language to use "Remove Participant" instead of "Banish User"
Test Plan: Read through the various cases, test them by removing myself or others
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17697
Summary:
I think these got munged when I removed CAN_JOIN.
- If you can view the room, you can join it.
- ~~If you can view the room, you can add others to it.~~ This rule adjustment was removed, see discussion on the revision.
- If you are a participant in the room, you can remove yourself.
- If you can edit a room, you can remove anyone.
Test Plan:
Normal feature set:
- Create a new room that only I can edit, viewable by all users.
- Leave room (bye k thx)
- Create another room, myself only
- Join room from second account
- See ability to only remove myself
- Remove myself
- Rejoin
- Add third account
- Log into first account
- Boot off randos
- Test joining by green button, message, and by + sign.
Policy consistency:
- As a user who can not edit the room, tried to add other members. Received policy exception. The `+` button is currently visible and enabled for all users (even users who have not joined the room) but this is pre-existing.
Reviewers: chad
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17696
Summary:
Ref T12451. This is a GREAT comment (A++) but we only need one copy of it.
This uses a pattern similar to Projects, which is a little weird but works well enough.
Test Plan:
- Viewed all four tabs of an account.
- Viewed a page with a bad account ID which 404'd properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17694
Summary: Ref T12451. This code is the same as the other code.
Test Plan: Went through the default-account case with this code, worked the same as the other code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17693
Summary:
Ref T12451. Ref T12484. This should deal with all the `+` / `-` / `=` cases correctly, I think.
Also makes sure that members are real users, not commits or tokens or whatever. And expands the creation test case to make some other basic sanity checks.
Test Plan:
- Went through implicit first-time creation flow.
- Went through explicit second-time creation flow.
- Unit test now passes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12484, T12451
Differential Revision: https://secure.phabricator.com/D17692
Summary:
Ref T12451. Ref T12484. I think D17657 fixed this, but caused the bug in D17690. The fix for that causes this bug again.
Put a unit test on it. This test currently fails; I'll correct the bug in the next change.
Test Plan: Ran `arc unit`, saw a failure.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12484, T12451
Differential Revision: https://secure.phabricator.com/D17691
Summary:
Ref T12451. When you explicitly created a second or third account or whatever, you wouldn't be added as a member.
(The editor sees that you're "already a member", so it doesn't add you.)
Test Plan:
- Go to `/phortune/`.
- Click "Switch Accounts".
- Click "Create Account".
- Create an account.
- Before patch: unable to view it since you don't get added as a member.
- After patch: account created with you as member.
- Also created an accont with multiple members.
- Tried to create an account with no members.
- Tried to create an account with just someone else as a member.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17690
Summary: Ref T12451. `$this->getAccount()` may not return an account.
Test Plan:
- Visit `/phortune/X/`, where `X` is the ID of an account you don't have permission to view.
- Before patch: fatal.
- After patch: normal policy exception page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17689
Summary:
Via HackerOne (<https://hackerone.com/reports/220909>). When we close commits in response to "Fixes Txxx", we currently act as the omnipotent user. This allows users to close tasks they can't see by pushing commits with "Fixes Txxx" in the message.
However, we can't actually tell who authored or committed a change: we're just using the "Author" and "Committer" values from Git in most cases, and anyone can forge those. So we can't really get this right, in a security sense.
(We can tell who //pushed// a change if we host it, but that's often not the right user. If GPG signing was more prevalent, we could use that. In the future, we could use side channels like having `arc land` tell Phabrcator who was pushing changes.)
Since I think the impact of this is fairly minor and this isn't //really// a security issue (more of a confusion/abuse/product issue) I think the behavior is okay more-or-less as-is, but we can do better when we do identify an author: drop permissions, and use their privileges to load the tasks which the commit "fixes".
This effectively implements this rule:
> If we identify the author of a commit as user X, that commit can only affect tasks which user X can see and edit.
Note that:
- Commits which we can't identify the author for can still affect any task.
- Any user can forge any other user's identity (or an invalid identity) and affect any task.
So this is just a guard rail to prevent mistakes by good-faith users who type the wrong task IDs, not a real security measure.
Also note that to perform this "attack" you must already have commit access to a repository (or permission to create a repository).
Test Plan:
- Used `bin/repository reparse --message <commit> --force-autoclose` to run the relevant code.
- Made the code `throw` before it actually applied the edit.
- Verified that the edit was rejected if the author was recognized and can not see or could not edit the task.
- Verified that the edit is accepted if the author can see+edit the task.
- Verified that the edit is accepted if we can't figure out who the author is.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17688
Summary:
Fixes T12554. The SSH key cache contains usernames, but is not currently dirtied on username changes.
An alternative solution would be to use user PHIDs instead of usernames in the file, which would make this unnecessary, but that would make debugging a bit harder. For now, I think this small added complexity is worth the easier debugging, but we could look at this again if cache management gets harder in the future.
Test Plan:
- Added a key as `ducksey`, ran `bin/ssh-auth`, saw key immediately.
- Renamed `ducksey` to `ducker`, ran `bin/ssh-auth`, saw username change immediately.
- Added another key as `ducker`, ran `bin/ssh-auth`, saw key immediately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12554
Differential Revision: https://secure.phabricator.com/D17687
Summary: Also fixes insufficiently-escaped regex examples
Test Plan: Made several changes to http://local.phacility.com/config/edit/syntax.filemap/ and observed validation failures on malformed regexes, and success on well-formed regexes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12532
Differential Revision: https://secure.phabricator.com/D17684
Test Plan:
Created new paste with title '.arcconfig' without choosing a language; observed that the paste gets highlighted as JSON.
JSON mode:
{F4901762}
Javascript mode:
{F4901763}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11667
Differential Revision: https://secure.phabricator.com/D17682
Summary: We no longer display this any more in the UI, so go ahead and remove the callsites and db column.
Test Plan: New Room, with and without participants.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17683
Summary: removes old phabricator.com/countdown/{id} route and code that uses that URL scheme
Test Plan: loaded phabricator.com/countdown, verified that generated links point to phabricator.com/CXXX
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12524
Differential Revision: https://secure.phabricator.com/D17681
Summary: Swaps this transaction over.
Test Plan: Load up a few rooms with date markers, still render as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12550
Differential Revision: https://secure.phabricator.com/D17680
Summary: Will see how this goes in practice. Uses violet where color is used for non responsive peeps.
Test Plan: Create a user without email verification, test hover card, profile, mentions and lists.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17678
Summary: This moves the count on the Conpherence Menu Item into a phui-list-item-count, and removes the CSS call to the entire Conphrence stack when durable column is open.
Test Plan: Test with and without the chat column, and a menu with a count
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17677
Summary: Fixes T12178, Fixes T11704 Not sure this feature gets any use and I can't find a similar option in other software, so removing it I think simiplifies a number of things. Removes CAN_JOIN and joinable is basically now CAN_VIEW and !$participating. Also removed some old transaction strings for other policies. Don't seem used.
Test Plan: Create a new room, edit room policies, see changes. Log into second account, search for rooms, everything now is visible.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12178, T11704
Differential Revision: https://secure.phabricator.com/D17675
Summary: Does a few things. Turns off feed stories (again), removes "action" transactions from notificiations, and only updates message count on actual messages. This feels a bit cleaner and less spammy... I guess... I think @epriestley will really like it and do me a favor or something.
Test Plan: Pull up two windows. test a message, see message count on second screen. Edit a topic or title, get no notification. At all. Ever.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17674
Summary:
Ref T12137. If a database is missing the InnoDB or MyISAM table engines, the big combined query to get both will fail.
Instead, try InnoDB first and then MyISAM.
(I have both engines locally so this worked until I deployed it.)
Test Plan: Faked an InnoDB error like `secure`, got a MyISAM result.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137
Differential Revision: https://secure.phabricator.com/D17673
Summary:
Depends on D17670. Fixes T12137. Fixes T12003. Ref T2632.
This shows users a readout of which terms were actually searched for.
This also drops those terms from the query we submit to the backend, dodging the weird behaviors / search engine bugs in T12137.
This might need some design tweaking.
Test Plan: {F4899825}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137, T12003, T2632
Differential Revision: https://secure.phabricator.com/D17672
Summary:
Depends on D17669. Ref T12137. Ref T12003. Ref T2632. Ref T7860.
Converts Phabricator to the new parse + compile workflow with intermediate tokens.
Also fixes a bug where searches for `cat"` or similar (unmatched quotes) wouldn't produce a nice exception.
Test Plan:
- Fulltext searched.
- Fulltext searched in Conpherence.
- Fulltext searched with bad syntax.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137, T12003, T7860, T2632
Differential Revision: https://secure.phabricator.com/D17670
Summary:
Fixes T8285. Fulltext search relies on an underlying engine which can not realistically use cursor paging. This is unusual and creates some oddness.
Tweak a few numbers -- and how offsets are handled -- to separate the filtered offset and unfiltered offset.
Test Plan:
- Set page size to 2.
- Ran a query.
- Paged forward and backward through results sensibly, seeing the full result set.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8285
Differential Revision: https://secure.phabricator.com/D17667
Summary: Begin converting Conpherence to ModularTransactions, this converts title, topic, and picture to use modular transactions. Participants seems hairy so I'll do that in another diff
Test Plan: Create a room with a topic, change room name, topic. Add people, remove people. Set a room image. Unset topic.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17668
Summary: Fixes T11730. Removes an old transaction that hasn't been used in a year.
Test Plan: Run sql, check various rooms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11730
Differential Revision: https://secure.phabricator.com/D17666
Summary: looked for places where Countdown monograms/uris were being constructed by hand, and updated with modern versions
Test Plan: clicked around the Countdown UI, looking for broken links
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, Korvin
Maniphest Tasks: T12524
Differential Revision: https://secure.phabricator.com/D17665
Summary: In Conpherence ProfileMenuItem we show an unread count if you're a participant, but all message count if you're not. Just remove that.
Test Plan: Log out of room in Conpherence, leave messages on second account, check menu item on both accounts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17664
Summary: Builds a Conpherence Profile Menu Item, complete with counts for the unreads. This allows pinning to home as well as swapping out thread list in Conpherence for pinning eventually.
Test Plan: Add a menu item, chat in room, log into other account, see room count. Room count disappears after viewing.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17662
Summary: Removes this feature, makes creating a room simpler and less confusing.
Test Plan: Create a room on Conpherence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17661
Summary: Primarily, this splits individual sections of the single account page into a more managable and robust sidenav for subscriptions, billing, and managers. The functionality on the subpages is light, but I expect to build on then in coming diffs. This also starts building out a more effective "status" area on the lead page.
Test Plan:
- Load up default account
- Make some edits
- Click on each of the new navigation items
- Verify links to "see all" work
- Test overdue and no payment states for status
{F4337317}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17589
Summary: There is currently a validation error triggered if you initialize a new account without a member set. I think this is the correct fix, but let me know.
Test Plan: truncate phortune_account database, navigate to phortune, see account automatically created to "Default Account".
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17657
Summary: Fixes T12541. `describeAutomaticCapability()` is no longer required to implement `PolicyInterface`. Use PolicyCodex instead.
Test Plan: {F4889642}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12541
Differential Revision: https://secure.phabricator.com/D17658
Summary: Builds out Phortune Merchant pages to have a sidenav and sub-pages for further expansion. For now this links Orders and Subscriptions to the query engine pages, but could be split out to be more informative (unpaid, upcoming, etc).
Test Plan:
Create a new merchant, edit some information, add a manager in new UI, edit logo, click through to subscriptions, orders.
{F4883013}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17655
Summary: This updates the backend of PhortuneAccount to use EditEngine and Modular Transactions and updates language to "account manager" for clarity of role.
Test Plan:
- Wiped `phortune_account` table
- Visit Phortune, see new account automatically created.
- Edit name and managers
- Try to set no name or remove myself as a manager, get error messages
- Visit `/phortune/` and create another new account
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17585
Summary: Fixes T12536. Nothing reads this parameter; `PhabricatorFile::newChunkedFile` sets the `isPartial` flag automatically.
Test Plan: Grepped for `isPartial`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12536
Differential Revision: https://secure.phabricator.com/D17654
Summary:
Previously, "reject" and "reject older" were separate statuses. Now, they're both shades of "reject".
Set the "older reject" flag properly when we find a non-current reject.
Test Plan:
- User A accepts a revision.
- User B rejects it.
- Author updates it.
- Before patch: incorrectly transitions to "accepted" ("older" reject is ignored).
- After patch: correctly transitions to "needs review".
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17653
Summary: Modernize PhortuneMerchant for Modular Transactions. Also changed the language of "Members" to "Managers", which I think fits better given the power/capability.
Test Plan:
- Create a new Merchant
- Test not filling in a name, see error
- Test removing myself, see error
- Edit an existing Merchant
- Add new managers
- Test removing myself, see error
- Replace Picture
- Update various fields, contact info, email, footer
- Verify transactions are now nice and pretty
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17584
Summary:
Fixes T12531. Strictness fallout from adding typechecking in D17616.
- `chunkedHash` is not a real parameter, so the new typechecking was unhappy about it.
- `mime-type` no longer allows `null`.
Test Plan:
- Ran `arc upload --conduit-uri ... 12MB.zero` on a 12MB file full of zeroes.
- Before patch: badness, failure, fallback to one-shot uploads.
- After patch: success and glory.
Reviewers: chad
Subscribers: joshuaspence
Maniphest Tasks: T12531
Differential Revision: https://secure.phabricator.com/D17651
Summary: Fixes T12138. Test for the presence of being in fullscreen mode, and disable send on enter if present. Side note, I'd love a first class "hasClass" type Javelin function.
Test Plan:
- Go to Conpherence
- Type some smack, see it send on enter
- Go fullscreen like a boss
- Let the words flow
- Close fullscreen, then send on enter.
- (might be nice someday to add a "submit" button to fullscreen editor)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12138
Differential Revision: https://secure.phabricator.com/D17590
Summary:
Fixes T12356.
- In this mail, we currently render "6:00 AM". Instead, render "6:00 AM (PDT)" or similar. This is consistent with times in other modern Transaction mail.
- Previously, we would render "UTC-7". Render "PDT" instead. For obscure zones with no known timezone abbreviation, fall back to "UTC-7".
Test Plan:
- Used `bin/calendar notify --minutes X` to trigger notifications, read email bodies.
- Used this script to list all `T` values and checked them for sanity:
```lang=php
<?php
$now = new DateTime();
$locales = DateTimeZone::listIdentifiers();
foreach ($locales as $locale) {
$zone = new DateTimeZone($locale);
$now->setTimeZone($zone);
printf(
"%s (%s)\n",
$locale,
$now->format('T'));
}
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12356
Differential Revision: https://secure.phabricator.com/D17646
Summary:
Ref T11816. Depends on D17644. When you executed a query like "upcoming, limit 5 events" you might match some recurring events starting from, say, a year ago and repeating every month.
We'd then generate the first 5 ghosts for these events (say, last January, February, ... May) and later throw them out, so the correct events in the query window (say, this April) would never get generated.
Instead, generate ghosts beginning with the start of the window. The fix in D17644 to number results correctly allows us to do this.
Test Plan:
- Made a query panel showing 5 events, scheduled an event long in the past, did not visit any of the instances of it so they didn't generate concrete objects.
- Before the patch, near-future instances failed to show; after the patch, they show.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17645
Summary:
Ref T11816. Two minor issues:
- We used `$event`, not `$next_event`, as the event providing the PHID for "Busy at <event name>". This rendered "Busy at <most future event>" on the profile instead of "Busy at <next upcoming event".
- The TTL computation used the event start, not the event end, so we could end up rebuilding the cache too often for users busy at an event.
Test Plan:
- Attended an event in the near future and one later on.
- Saw profile now say "busy at <near future event>" correctly.
- In DarkConsole "Services" tab, no longer saw unnecessary cache refills while attending an event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17643
Summary: Fixes T11561. Collect guidance about local configuration which hasn't been obvious in the past.
Test Plan:
- Read document carefully.
- Used `./bin/diviner generate` to generate documentation.
- Previewed in Diviner locally:
{F4795021}
Reviewers: amckinley, chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T11561
Differential Revision: https://secure.phabricator.com/D17641
Summary:
Even with `innodb_file_per_table` enabled, individual table files on disk don't normally shrink.
For most tables, like `maniphest_task`, this is fine, since the data in the table normally never shrinks, or only shinks a tiny amount.
However, some tables (like the "worker" and "daemon" tables) grow very large during a huge import but most of the data is later deleted by garbage collection. In these cases, this lost space can be reclaimed by running `OPTIMIZE TABLE` on the tables.
Add a script to `OPTIMIZE TABLE` every table.
My primary goal here is just to reduce storage pressure on `db001` since there are a couple of "import the linux kernel" installs on that host wasting a bunch of space. We're not in any trouble, but this should buy us a good chunk of headroom.
Test Plan: Ran `bin/storage optimize` locally and manually ran `OPTIMIZE TABLE` in production, saw tables get optimized.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17640
Summary: Ref T4245. We disallow `/diffusion/` in robots.txt already because indexers tend to get lost blaming every line of every file throughout history, but didn't update the list for the `/source/` alias. Update it.
Test Plan: Visited `/robots.txt` locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D17637
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.
Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.
I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.
Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17631
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.
The new mechanism also automatically generates and stores HMAC keys.
Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.
We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.
Beyond that, this isn't something users should really have to think about or bother configuring.
Test Plan:
- Added unit tests.
- Used `bin/files integrity` to verify, strip, and recompute hashes.
- Tampered with a generated HMAC key, verified it invalidated hashes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17630
Summary:
Ref T12470. Provides an "integrity" utility which runs in these modes:
- Verify: check that hashes match.
- Compute: backfill missing hashes.
- Strip: remove hashes. Useful for upgrading across a hash change.
- Corrupt: intentionally corrupt hashes. Useful for debugging.
- Overwrite: force hash recomputation.
Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes.
I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files.
Test Plan:
- Ran the script in many modes against various files, saw expected operation, including:
- Verified a file, corrupted it, saw it fail.
- Verified a file, stripped it, saw it have no hash.
- Stripped a file, computed it, got a clean verify.
- Stripped a file, overwrote it, got a clean verify.
- Corrupted a file, overwrote it, got a clean verify.
- Overwrote a file, overwrote again, got a no-op.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12470
Differential Revision: https://secure.phabricator.com/D17629
Summary:
Ref T12298. The PullLocal daemon has had hibernation code for a little while, but it never actually activated because we don't sleep for more than 15 seconds in any case.
Add a maximum sleep instead and use that to control the longest sleep we'll do for hibernation purposes.
Also, when a repository or repository URI is edited, write a NEEDS_UPDATE event into the message table to make sure the daemons de-hibernate.
Test Plan: Used `bin/phd debug pull`, saw the daemon actually hibernate instead of just sleeping for 15 seconds.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17635
Summary:
Ref T12272. I wrote this correctly, then broke it by adding the simplification which treats "accept the defaults" as "accept everything".
This simplification lets us render "epriestley accepted this revision." instead of "epriestley accepted this revision onbehalf of: long, list, of, every, default, reviewer, they, have, authority, over." so it's a good thing, but make it only affect the reviewers it's supposed to affect.
Test Plan:
- Did an accept with a force-accept available but unchecked.
- Before patch: incorrectly accepted all possible reviewers.
- After patch: accepted only checked reviewers.
- Also checked the force-accept box, accepted, got a proper force-accept.
Reviewers: chad, lvital
Reviewed By: lvital
Maniphest Tasks: T12272
Differential Revision: https://secure.phabricator.com/D17634
Summary: Allow API callers to retrieve reviewer information via a new "reviewers" attachment.
Test Plan: {F4675784}
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Differential Revision: https://secure.phabricator.com/D17633
Summary: Fixes T12508. Files don't have an `editPolicy`, and we started actually checking that the keys are real things in D17616.
Test Plan:
- Before patch: created a paste, got an "editPolicy" exception.
- After patch: created a paste that worked properly.
Reviewers: avivey, chad
Reviewed By: avivey
Maniphest Tasks: T12508
Differential Revision: https://secure.phabricator.com/D17628
Summary: Ref T12219. Chrome can send requests with a "Range: bytes=0-" header, which just means "the whole file", but we don't respond correctly because of a `null` vs `0` issue.
Test Plan: Sent a raw `bytes=0-` request, saw a proper resonse.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12219
Differential Revision: https://secure.phabricator.com/D17627
Summary:
Ref T12219. We currently only support Range requests like "bytes=123-456", but "bytes=123-", meaning "until end of file", is valid, and Chrome can send these requests.
I suspect this is the issue with T12219.
Test Plan: Used `nc local.phacility.com 80` to pipe raw requests, saw both "bytes=123-456" and "bytes=123-" requests satisfied correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12219
Differential Revision: https://secure.phabricator.com/D17626
Summary:
Ref T12470. This helps defuse attacks where an adversary can directly take control of whatever storage engine files are being stored in and change data there. These attacks would require a significant level of access.
Such attackers could potentially attack ranges of AES-256-CBC encrypted files by using Phabricator as a decryption oracle if they were also able to compromise a Phabricator account with read access to the files.
By storing a hash of the data (and, in the case of AES-256-CBC files, the IV) when we write files, and verifying it before we decrypt or read them, we can detect and prevent this kind of tampering.
This also helps detect mundane corruption and integrity issues.
Test Plan:
- Added unit tests.
- Uploaded new files, saw them get integrity hashes.
- Manually corrupted file data, saw it fail. Used `bin/files cat --salvage` to read it anyway.
- Tampered with IVs, saw integrity failures.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12470
Differential Revision: https://secure.phabricator.com/D17625
Summary: Ref T8266. Although we compute this correctly above, we ignored it when actually setting the header. Use the computed value to set the "Content-Length" header. This is consistent with the spec/documentation.
Test Plan: Before, some audio (like `rain.mp3`) was pretty spotty about loading in Safari. It now loads consistently for me locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8266
Differential Revision: https://secure.phabricator.com/D17624
Summary:
Fixes T12079. Currently, when a file is encrypted and a request has "Content-Range", we apply the range first, //then// decrypt the result. This doesn't work since you can't start decrypting something from somewhere in the middle (at least, not with our cipher selection).
Instead: decrypt the result, //then// apply the range.
Test Plan: Added failing unit tests, made them pass
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12079
Differential Revision: https://secure.phabricator.com/D17623
The root issue here is actually just that I cherry-picked stable locally
but did not push it. However, this is a minor issue I also caught while
double-checking things.
Auditors: chad
Summary:
Ref T12464. This defuses any possible SHA1-collision attacks by using SHA256, for which there is no known collision.
(SHA256 hashes are larger -- 256 bits -- so expand the storage column to 64 bytes to hold them.)
Test Plan:
- Uploaded the same file twice, saw the two files generate the same SHA256 content hash and use the same underlying data.
- Tried with a fake hash algorihtm ("quackxyz") to make sure the failure mode worked/degraded correctly if we don't have SHA256 for some reason. Got two valid files with two copies of the same data, as expected.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17620
Summary:
Ref T12464. We currently use SHA1 to detect when two files have the same content so we don't have to store two copies of the data.
Now that a SHA1 collision is known, this is theoretically dangerous. T12464 describes the shape of a possible attack.
Before replacing this with something more robust, shore things up so things work correctly if we don't hash at all. This mechanism is entirely optional; it only helps us store less data if some files are duplicates.
(This mechanism is also less important now than it once was, before we added temporary files.)
Test Plan: Uploaded multiple identical files, saw the uploads work and the files store separate copies of the same data.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17619
Summary:
Ref T12464. This is a very old method which let you create a file on the server by referring to data which already existed in another file.
Basically, long ago, `arc` could say "Do you already have a file with hash X?" and just skip some work if the server did.
`arc` has not called this method since D13017, in May 2015.
Since it's easy to do so, just make this method pretend that it never has the file. Very old clients will continue to work, since they would expect this response in the common case and continue by uploading data.
Test Plan:
- Grepped for `uploadhash` in Phabricator and Arcanist.
- Called the method with the console, verified it returned `null`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17618
Summary:
Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.
In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like `newFromFileData()` do this right: they share underlying data in storage, but not the actual `File` records.
Specifically, this is the case where we get into trouble:
- I upload a private file with content "X".
- You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
- If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.
Just get rid of this.
Test Plan:
- Generated an SSH key.
- Viewed a raw diff in Differential.
- (Did not test Phragment.)
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17617
Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.
For example, to mean "this file expires in 60 minutes", you might pass either of these:
- `time() + phutil_units('60 minutes in seconds')`
- `phutil_units('60 minutes in seconds')`
The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.
Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.
Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17616
Summary:
Ref T11357. In D17611, I added `file.search`, which includes a `"dataURI"`. Partly, this is building toward resolving T8348.
However, in some cases you can't GET this URI because of a security measure:
- You have not configured `security.alternate-file-domain`.
- The file isn't web-viewable.
- (The request isn't an LFS request.)
The goal of this security mechanism is just to protect against session hijacking, so it's also safe to disable it if the viewer didn't present any credentials (since that means there's nothing to hijack). Add that exception, and reorganize the code a little bit.
Test Plan:
- From the browser (with a session), tried to GET a binary data file. Got redirected.
- Got a download with POST.
- From the CLI (without a session), tried to GET a binary data file. Go a download.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17613
Summary: Ref T11357. Implements a modern `file.search` for files, and freezes `file.info`.
Test Plan: Ran `file.search` from the Conduit console.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17612
Summary:
Ref T11357. This moves editing and commenting (but not creation) to EditEngine.
Since only the name is really editable, this is pretty straightforward.
Test Plan: Renamed files; commented on files.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17611
Summary: Ref T11357. A lot of file creation doesn't go through transactions, so we only actually have one real transaction type: editing a file name.
Test Plan:
Created and edited files.
{F4559287}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17610
Summary:
Fixes T12502. This transaction probably should not be getting picked for feed rendering, but it currently does get selected in some cases.
This should probably be revisited eventually (e.g., when Maniphest moves to ModularTransactions) but just fix the brokenness for now.
Test Plan:
- Created a task in a space.
- Viewed feed.
- Saw the story render with readable text.
{F4555747}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12502
Differential Revision: https://secure.phabricator.com/D17609
Summary:
Fixes T12496. Sticky accept was accidentally impacted by the "void" changes in D17566.
Instead, don't always downgrade all accepts/rejects: on update, we only want to downgrade accepts.
Test Plan:
- With sticky accept off, updated an accepted revision: new state is "needs review".
- With sticky accept on, updated an accepted revision: new state is "accepted" (sticky accept working correctly).
- Did "reject" + "request review" to make sure that still works, worked fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12496
Differential Revision: https://secure.phabricator.com/D17605
Summary:
Fixes T12461. This returns the field as a dictionary with a `"raw"` value, so we could eventually do this if we want without breaking the API:
```
{
"type": "remarkup",
"raw": "**raw**",
"html": "<strong>raw</strong>",
"text": "raw"
}
```
Test Plan: Called `maniphest.search`, reviewed output.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12461
Differential Revision: https://secure.phabricator.com/D17603
Summary: Ref T12450. These are like 95% my fault, but Elastic appears to spell the name "Elasticsearch" consistently in their branding.
Test Plan: `grep ElasticSearch`
Reviewers: chad, 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17601
Summary:
Ref T12450. Currently, if a write fails, we stop and don't try to write to other index services. There's no technical reason not to keep trying writes, it makes some testing easier, and it would improve behavior in a scenario where engines are configured as "primary" and "backup" and the primary service is having some issues.
Also, make "no writable services are configured" acceptable, rather than an error. This state is probably goofy but if we want to detect it I think it should probably be a config-validation issue, not a write-time check. I also think it's not totally unreasonable to want to just turn off all writes for a while (maybe to reduce load while you're doing a background update).
Test Plan:
- Configured a bad ElasticSearch engine and a good MySQL engine.
- Ran `bin/search index ... --force`.
- Saw MySQL get updated even though ElasticSearch failed.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17599
Summary:
Ref T12450. We track a "document version" for updating search indexes, so that if a document is rapidly updated many times in a row we can skip most of the work.
However, this version doesn't consider "cluster.search" configuration, so if you add a new service (like a new ElasticSearch host) we still think that every document is up-to-date. When you run `bin/search index` to populate the index (without `--force`), we just do nothing.
This isn't necessarily very obvious. D17597 makes it more clear, by printing "everything was skipped and nothing happened" at the end.
Here, fix the issue by considering the content of "cluster.search" when computing fulltext document versions: if you change `cluster.search`, we throw away the version index and reindex everything.
This is slightly more work than we need to do, but changes to "cluster.search" are rare and this is much easier than trying to individually track which versions of which documents are in which services, which probably isn't very useful anyway.
Test Plan:
- Ran `bin/search index --type project`, saw everything get skipped.
- Changed `cluster.search`.
- Ran `search index` again, saw everything get updated.
- Ran a third time without changing `cluster.search`, everything was properly skipped.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17598
Summary:
Ref T12450. There's currently a bad behavior where inserting a document into one search service marks it as up to date everywhere.
This isn't nearly as obvious as it should be because `bin/search index` doesn't make it terribly clear when a document was skipped because the index version was already up to date.
When running `bin/seach index` without `--force` or `--background`, keep track of updated vs not-updated documents and print out some guidance. In other configurations, try to provide more help too.
Test Plan: {F4452134}
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17597
Summary:
Ref T12450. This was added a very very long time ago (D2298).
I don't want to put this in the upstream index anymore because I don't want to encourage third parties to develop software which reads the index directly. Reading the index directly is a big skeleton key which bypasses policy checks.
This was added before much of the policy model existed, when that wasn't as much of a concern. On a tecnhnical note, this also doesn't update when `phabricator.base-uri` changes.
This can be written as a search index extension if an install relies on it for some bizarre reason, although none should and I'm unaware of any actual use cases in the wild for it, even at Facebook.
Test Plan: Indexed some random stuff into ElasticSearch.
Reviewers: chad, 20after4
Reviewed By: chad
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17600
Summary:
Ref T12450. General adjustments:
- Try to make "Cluster: Search" more about "stuff in common + types" instead of pretty much all being Elastic-specific, so we can add Solr or whatever later.
- Provide guidance about rebuilding indexes after making a change.
- Simplify the basic examples, then provide a more advanced example at the ed.
- Really try to avoid suggesting anyone configure Elasticsearch ever for any reason.
Test Plan: Read documents, previewed in remarkup.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17602
Summary:
D17384 added a "keywords" field but only partially implemented it.
- Remove this field.
- Index project slugs as part of the document body instead.
Test Plan:
- Ran `bin/search index PHID-PROJ-... --force`.
- Found project by searching for a unique slug.
Reviewers: chad, 20after4
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17596
Summary:
If you have `maniphest.custom-field-definitions` set to include "required" fields, a bunch of tests which create tasks can fail.
To avoid this, reset this config while running tests.
This mechanism should probably be more general (e.g., reset all config by default, only whitelist some config) but just fix this for now since it's a one-liner and doesn't make eventual cleanup any harder.
Test Plan: Ran `arc unit`, hitting tests that create tasks.
Reviewers: chad, 20after4
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17595
Summary: When building a tokenizer-based edit control for a custom field (e.g. a datasource type), preserve a field validation error whilst building edit controls.
Test Plan:
- Create custom datasource field, set it to required
- Observe that 'Required' does not appear next to control
- Apply patch
- Observe 'Required' appears next to control
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17592
Summary: Minor, uses 'user-circle' for account, and merchant logo for merchants in lists.
Test Plan: View the landing page, see updated logos and icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17586
Summary: Move individual controller files into cooresponding folders. Makes it easier to locate sections and expand without clutter. Also made "chargelist" part of account since it's tied to having an account specifically.
Test Plan: Vist charges, merchants, subscription, accounts, and other pages. No errors from file move.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17587
Summary:
Two little issues
1. there was an extra call to getHostForWrite,
2. The engine instance was shared between multiple service definitions so it
was overwriting the list of writable hosts from one service with hosts from another.
Test Plan:
tested in wikimedia production with multiple services defined like this:
```language=json
[
{
"hosts": [
{
"host": "search.svc.codfw.wmnet",
"protocol": "https",
"roles": {
"read": true,
"write": true
},
"version": 5
}
],
"path": "/phabricator",
"port": 9243,
"type": "elasticsearch"
},
{
"hosts": [
{
"host": "search.svc.eqiad.wmnet",
"protocol": "https",
"roles": {
"read": true,
"write": true
},
"version": 5
}
],
"path": "/phabricator",
"port": 9243,
"type": "elasticsearch"
}
]
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17581
Summary:
Elasticsearch really wants a raw json body and it fails to accept
the request as of es version 5.3
Test Plan:
Tested with elasticsearch 5.2 and 5.3.
Before this change 5.2 worked but 5.3 failed with
`HTTP/406 "Content-Type header [application/x-www-form-urlencoded] is not supported"` [1]
After this change, both worked.
[1] https://phabricator.wikimedia.org/P5158
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17580
Summary:
These exception messages & comments didn't quite match reality.
Fixed and added pht() around a couple of them.
Test Plan: I didn't test this :P
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17578
Summary:
Fixes T12460. Also ":)", ":(", ":/", and oldschool ":-)" variants.
Not included are variants with actual letters (`:D`, `:O`, `:P`) and obscure variants (`:^)`, `:*)`).
Test Plan: Typed `:3` (no emoji summoned). Typed `:dog3` (emoji summoned). Typed `@3` (user autocomplete summoned).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12460
Differential Revision: https://secure.phabricator.com/D17577
Summary:
Ref T12450. The way that config repair and setup issues interact is kind of complicated, and if `cluster.search` is invalid we may end up using `cluster.search` before we repair it.
I poked at things for a bit but wasn't confident I could get it to consistently repair before we use it without doing a big messy change.
The only thing that really matters is whether "type" is valid or not, so just put a slightly softer/more-tailored check in for that.
Test Plan:
- With `"type": "elastic"`, loaded setup issues.
- Before patch: hard fatal.
- After patch: softer fatal with more useful messaging.
{F4321048}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17576
Summary:
Ref T12450. Normally, we validate config when:
- You restart the webserver.
- You edit it with `bin/config set ...`.
- You edit it with the web UI.
However, you can also change config by editing `local.json`, `some_env.conf.php`, a `SiteConfig` class, etc. In these cases, you may miss config warnings.
Explicitly re-run search config checks from `bin/search`, similar to the additional database checks we run from `bin/storage`, to try to produce a better error message if the user has made a configuration error.
Test Plan:
```
$ ./bin/search init
Usage Exception: Setting "cluster.search" is misconfigured: Invalid search engine type: elastic. Valid types are: elasticsearch, mysql.
```
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17574
Summary:
Ref T12450. Minor cleanup:
- setRoles() has no callers.
- getRoles() has no callers (these two methods are leftovers from an earlier iteration of the change).
- The `hasRole()` logic doesn't work since nothing calls `setRole()`.
- `hasRole()` has only `isreadable/iswritable` as callers.
- The `isReadable()/isWritable()` logic doesn't work since `hasRole()` doesn't work.
Instead, just check if there are any readable/writable hosts. `Host` already inherits its config from `Service` so this gets the same answer without any fuss.
Also add some read/write constants to make grepping this stuff a little easier.
Test Plan:
- Grepped for all removed symbols, saw only newer-generation calls in `Host`.
- See next diff for use of `isWritable()`.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17571
Summary:
Ref T12450. This is now pointless and just asserts that `cluster.search` has a default value.
We might restore a fancier version of this eventually, but get rid of this for now.
Test Plan: Scruitinized the test case.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17573
Summary:
Ref T12450. This mostly just smooths out the text a little to improve consistency. Also:
- Use `isWritable()`.
- Make the "skipping because not writable" message more clear and tailored.
- Try not to use the word "index" too much to avoid confusion with `bin/search index` -- instead, talk about "initialize a service".
Test Plan: Ran `bin/search init` with a couple of different (writable / not writable) configs, saw slightly clearer messaging.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17572
Summary:
[ ] Write an "Upgrading: ..." guidance task with narrow instructions for installs that are upgrading.
[ ] Do we need to add an indexing activity (T11932) for installs with ElasticSearch?
[ ] We should more clearly detail exactly which versions of ElasticSearch are supported (for example, is ElasticSearch <2 no longer supported)? From T9893 it seems like we may //only// have supported ElasticSearch <2 before, so are the two regions of support totally nonoverlapping and all ElasticSearch users will need to upgrade?
[ ] Documentation should provide stronger guidance toward MySQL and away from Elastic for the vast majority of installs, because we've historically seen users choosing Elastic when they aren't actually trying to solve any specific problem.
[ ] When users search for fulltext results in Maniphest and hit too many documents, the current behavior is approximately silent failure (see T12443). D17384 has also lowered the ceiling for ElasticSearch, although previous changes lowered it for MySQL search. We should not fail silently, and ideally should build toward T12003.
[ ] D17384 added a new "keywords" field, but MySQL does not search it (I think?). The behavior should be as consistent across MySQL and Elastic as we can make it. Likely cleaner is giving "Project" objects a body, with "slugs" and "description" separated by newlines?
[ ] `PhabricatorSearchEngineTestCase` is now pointless and only detects local misconfigurations.
[ ] It would be nice to build a practical test suite instead, where we put specific documents into the index and then search for them. The upstream test could run against MySQL, and some `bin/search test` could run against a configured engine like ElasticSearch. This would make it easier to make sure that behavior was as uniform as possible across engine implementations.
[ ] Does every assigned task now match "user" in ElasticSearch?
[x] `PhabricatorElasticFulltextStorageEngine` has a `json_encode()` which should be `phutil_json_encode()`.
[ ] `PhabricatorSearchService` throws an untranslated exception.
[ ] When a search cluster is down, we probably don't degrade with much grace (unhandled exception)?
[ ] I haven't run bin/search init, but bin/search index doesn't warn me that I may want to. This might be worth adding. The UI does warn me.
[ ] bin/search init warns me that the index is "incorrect". It might be more clear to distinguish between "missing" and "incorrect", since it's more comforting to users to see "everything is as we expect, doing normal first-time setup now" than "something is wrong, fixing it".
[ ] CLI message "Initializing search service "ElasticSearch"" does not end with a period, which is inconsistent with other UI messages.
[ ] It might be nice to let bin/search commands like init and index select a specific service (or even service + host) to act on, as bin/storage --ref ... now does. You can generally get the result you want by fiddling with config.
[ ] When a service isn't writable, bin/search init reports "Search cluster has no hosts for role "write".". This is accurate but does not provide guidance: it might be more useful to the user to explain "This service is not writable, so we're skipping index check for it.".
[x] Even with write off for MySQL, bin/search index --type task --trace still updates MySQL, I think? I may be misreading the trace output. But this behavior doesn't make sense if it is the actual behavior, and it seems like reindexAbstractDocument() uses "all services", not "writable services", and the MySQL engine doesn't make sure it's writable before indexing.
[x] Searching or user fails to find task Grant users tokens when a mention is created, suggesting that stemming is not working.
[x] Searching for users finds that task, but fails to find a task containing "per user per month" in a comment, also suggesting that stemming is not working.
[x] Searching for maniphest fails to find task maniphest.query elephant, suggesting that tokenization in ElasticSearch is not as good as the MySQL tokenization for these words (see D17330).
[x] The "index incorrect" warning UI uses inconsistent title case.
[x] The "index incorrect" warning UI could format the command to be run more cleanly (with addCommand(), I think).
refs T12450
Test Plan:
* Stared blankly at the code.
* Disabled 'write' role on mysql fulltext service.
* Edited a task, ran search indexer, verified that the mysql index wasn't being updated.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17564
Summary:
Ref T12272. If you own a package which owns "/", this allows you to force-accept package reviews for packages which own sub-paths, like "/src/adventure/".
The default UI looks something like this:
```
[X] Accept as epriestley
[X] Accept as Root Package
[ ] Force accept as Adventure Package
```
By default, force-accepts are not selected.
(I may do some UI cleanup and/or annotate "because you own X" in the future and/or mark these accepts specially in some way, particularly if this proves confusing along whatever dimension.)
Test Plan: {F4314747}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12272
Differential Revision: https://secure.phabricator.com/D17569
Summary: Ref T10967. This change is similar to D17566, but for rejects.
Test Plan:
- Create a revision as A, with reviewer B.
- Reject as B.
- Request review as A.
- Before patch: stuck in "rejected".
- After patch: transitions back to "needs review".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17568
Summary: Ref T10967. This moves all remaining "request review" pathways (just `differential.createcomment`) to the new code, and removes the old action.
Test Plan: Requested review on a revision, `grep`'d for the action constant.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17567
Summary:
Ref T10967. This is explained in more detail in T10967#217125
When an author does "Request Review" on an accepted revision, void (in the sense of "cancel out", like a bank check) any "accepted" reviewers on the current diff.
Test Plan:
- Create a revision with author A and reviewer B.
- Accept as B.
- "Request Review" as A.
- (With sticky accepts enabled.)
- Before patch: revision swithced back to "accepted".
- After patch: the earlier review is "voided" by te "Request Review", and the revision switches to "Review Requested".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17566
Summary: Before the speling pollice lock us in prisun.
Test Plan: Used a dicationairey.
Reviewers: chad, jmeador
Reviewed By: jmeador
Differential Revision: https://secure.phabricator.com/D17570
Summary:
This implements a simplistic `PhabricatorRepositoryFulltextEngine`
Currently only the repository name, description, timestamps and
status are indexed.
Note: I had to change the `search index` workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit
Test Plan:
* ran `./bin/search index --type PhabricatorRepository --force`
* searched for some repositories. Saw reasonable results matching on either title or description.
* Edited a repository in the web ui
* Added unique key words to the repo description.
* I was then able to find that repo by searching for the new keywords.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Tags: #search, #diffusion
Differential Revision: https://secure.phabricator.com/D17300
Summary: General CSS and usability touchup of the Remarkup bar states for fullscreen and preview. Larger fonts, more spacing, some hint of the underlying page. Disable buttons that can't be used in preview mode.
Test Plan:
Formal test coming with mobile, browsers. This is a kick the tires upload.
{F4283448}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17563
Summary:
The goal is to make fulltext search back-ends more extensible, configurable and robust.
When this is finished it will be possible to have multiple search storage back-ends and
potentially multiple instances of each.
Individual instances can be configured with roles such as 'read', 'write' which control
which hosts will receive writes to the index and which hosts will respond to queries.
These two roles make it possible to have any combination of:
* read-only
* write-only
* read-write
* disabled
This 'roles' mechanism is extensible to add new roles should that be needed in the future.
In addition to supporting multiple elasticsearch and mysql search instances, this refactors
the connection health monitoring infrastructure from PhabricatorDatabaseHealthRecord and
utilizes the same system for monitoring the health of elasticsearch nodes. This will
allow Wikimedia's phabricator to be redundant across data centers (mysql already is,
elasticsearch should be as well).
The real-world use-case I have in mind here is writing to two indexes (two elasticsearch clusters
in different data centers) but reading from only one. Then toggling the 'read' property when
we want to migrate to the other data center (and when we migrate from elasticsearch 2.x to 5.x)
Hopefully this is useful in the upstream as well.
Remaining TODO:
* test cases
* documentation
Test Plan:
(WARNING) This will most likely require the elasticsearch index to be deleted and re-created due to schema changes.
Tested with elasticsearch versions 2.4 and 5.2 using the following config:
```lang=json
"cluster.search": [
{
"type": "elasticsearch",
"hosts": [
{
"host": "localhost",
"roles": { "read": true, "write": true }
}
],
"port": 9200,
"protocol": "http",
"path": "/phabricator",
"version": 5
},
{
"type": "mysql",
"roles": { "write": true }
}
]
Also deployed the same changes to Wikimedia's production Phabricator instance without any issues whatsoever.
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Tags: #elasticsearch, #clusters, #wikimedia
Differential Revision: https://secure.phabricator.com/D17384
Under some workloads, the taskmaster may hibernate and launch more rapidly
than it should. Require 15 seconds of inactivity before hibernating. Also
hibernate for longer.
Auditors: chad
The `min()` vs `max()` fix in D17560 meant that the Trigger daemon only
hibernates for 5 seconds, so we do a full GC sweep every 5 seconds. This ends
up eating a fair amount of CPU for no real benefit.
The GC cursors should move to persistent storage, but just bump this default
up in the meantime.
Auditors: chad
If we try to render an edge transaction which uses unknown edge constants,
it turns out we fatal. Degrade instead. This happened when viewing very old
badges.
Auditors: chad
Summary: Ships Badges. I can write up some basic docs too if needed.
Test Plan: /applications/
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17360
Summary: Ref T12270. These no longer have any callsites.
Test Plan: Used `grep` to search for each edge class constant, found no hits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17562
Summary: Ref T12270. Adds a pager, plus a few little cleanups from copy/paste and accumulated cruft.
Test Plan:
- Paginated a user with 180 badges.
- Viewed a user with 0 badges.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17561
Summary: Ref T12298. Like PullLocal daemons, this allows the last daemon in the pool to hibernate if there's no work to be done, and awakens the pool when work arrives.
Test Plan:
- Ran `bin/phd debug task --trace`.
- Saw the pool hibernate and look for tasks.
- Commented on an object.
- Saw the pool wake up and process the queue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17559
Summary:
Ref T11050. The old rule was "you can only resign if you're a reviewer".
With the new behavior of "resign", the rule should be "you can resign if you're a reviewer, or you have authority over any reviewer". Make it so.
Also fixes T12446. I don't know how to reproduce that but I'm pretty sure this'll fix it?
Test Plan:
- Could not resign from a revision with no authority/reviewer.
- Resigned from a revision with myself as a reviewer.
- Resigned from a revision with a package I owned as a reviewer.
- Could not resign from a revision I had already resigned from.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12446, T11050
Differential Revision: https://secure.phabricator.com/D17558
Summary:
Ref T12298. Two minor daemon improvements:
- Make the "waiting" message reflect hibernation.
- Don't trigger a reload right after launching.
Test Plan:
- Read "waiting" message.
- Ran "bin/phd start", didn't see an immediate SIGHUP in the log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17550
Summary: Fixes T9363. This drops empty buckets from dashboard panel context. Still see full results in Audit.
Test Plan: Create an "Active Audits" panel, add to Dashboard. See no commits found. Check Audit, see all buckets.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9363
Differential Revision: https://secure.phabricator.com/D17545
Summary: Ref T9363, If we're in a dashboard panel, only show buckets with data, or a fallback if nothing exists.
Test Plan: Test 'active revisions' panel in a dashboard and in Differential.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9363
Differential Revision: https://secure.phabricator.com/D17544
Summary: Ref T12298. This allows the PullLocal daemon to hibernate like the Trigger daemon, but automatically wakes it back up when it needs to do something.
Test Plan:
- Ran `bin/phd debug pulllocal --trace`.
- Saw the daemon hibernate after doing a checkup on repositories.
- Saw periodic queries to look for new update messages.
- After clicking "Update Now" in the web UI to schedule an update, saw the daemon wake up immediately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17540
Summary:
Ref T12444. A few issues:
- `x % (y - z)` doesn't generate values in the full range: the largest value is never generated. Instead, use `x % (1 + y - z)`.
- `digestToRange(1, count)` never generates 0. After fixing the first bug, it could generate `count`. The range of the arrays is `0..(count-1)`, inclusive. Generate the correct range instead.
- `unpack('L', ...)` can unpack a negative number on a 32-bit system. Use `& 0x7FFFFFFF` to mask off the sign bit so the result is always a positive integer.
- FileFinder might return arbitrary keys, but we rely on sequential keys (0, 1, 2, ...)
Test Plan:
- Used `bin/people profileimage ... --force` to regenerate images.
- Added some debugging to verify that the math seemed to be working.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12444
Differential Revision: https://secure.phabricator.com/D17543
Summary:
Fixes T12369. When you create objects they may technically be locked: either because the default state is legitimately locked, or because the default policies prevent you from viewing so we sort of technically end in a locked state.
Regardless, don't prompt during creation, since this prompt isn't useful even if the lock detection is completely legitimate.
Test Plan:
- In {nav Applications > Maniphest > Configure}, set "Default View Policy" to "No One".
- Tried to create a task.
- Before patch: prompted to override lock.
- After patch: no override prompt.
Reviewers: chad
Reviewed By: chad
Subscribers: d.maznekov
Maniphest Tasks: T12369
Differential Revision: https://secure.phabricator.com/D17541
Summary:
Ref T12271. Don't do anything with this yet, but store who accepted/rejected/whatever on behalf of reviewers.
In the future, we could use this to render stuff like "Blessed Committers (accepted by epriestley)" or whatever. I don't know that this is necessarily super useful, but it's easy to track, seems likely to be useful, and would be a gigantic pain to backfill later if we decide we want it.
Test Plan: Accepted/rejected a revision, saw reviewers update appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17537
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
Summary: Hit this while `arc diff`'ing something which is triggering 2+ rules which add reviewers, I think.
Test Plan: Dug this out of a production stack trace; will push and `arc diff` again.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17534
Summary: Fixes T12439. This pathway was just missing a `setContinueOnMissingFields(...)` to skip enforcement of required fields.
Test Plan:
- Added a required custom field.
- Mentioned any task without a field value in a comment.
- Edited that comment.
- Saved changes.
- Before fix: fatal in log.
- After fix: clean edit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12439
Differential Revision: https://secure.phabricator.com/D17536
Summary: Fixes T11865. Part of a 'clean up remarkup' pass, removing Aleo helps simplify coding, is lighter on the wire, and gives a more consistent, clean look.
Test Plan: run celerity, grep for 'aleo' and 'Aleo', test Phriction, tasks
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11865
Differential Revision: https://secure.phabricator.com/D17535
Summary:
Ref T10967. I'm not 100% sure we need this, but the old edge table had it and I recall an issue long ago where not having this key left us with a bad query plan.
Our data doesn't really provide a way to test this key (we have many revisions and few reviewers, so the query planner always uses revision keys), and building a convincing test case would take a while (lipsum needs some improvements to add reviewers). But in the worst case this key is mostly useless and wastes a few MB of disk space, which isn't a big deal.
So I can't conclusively prove that this key does anything to the dashboard query, but the migration removed it and I'm more comfortable keeping it so I'm not worried about breaking stuff.
At the very least, MySQL does select this key in the query plan when I do a "Reviewers:" query explicitly so it isn't //useless//.
Test Plan: Ran `bin/storage upgrade`, ran dashboard query, the query plan didn't get any worse.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17532
Summary:
Fixes T11050. Today, when a user resigns, we just delete the record of them ever being a reviewer.
However, this means you have no way to say "I don't care about this and don't want to see it on my dashboard" if you are a member of any project or package reviewers.
Instead, store "resigned" as a distinct state from "not a reviewer", and treat it a little differently in the UI:
- On the bucketing screen, discard revisions any responsible user has resigned from.
- On the main `/Dxxx` page, show these users as resigned explicitly (we could just hide them, too, but I think this is good to start with).
- In the query, don't treat a "resigned" state as a real "reviewer" (this change happened earlier, in D17517).
- When resigning, write a "resigned" state instead of deleting the row.
- When editing a list of reviewers, I'm still treating this reviewer as a reviewer and not special casing it. I think that's sufficiently clear but we could tailor this behavior later.
Test Plan:
- Resigned from a revision.
- Saw "Resigned" in reviewers list.
- Saw revision disappear from my dashboard.
- Edited revision, saw user still appear as an editable reviewer. Saved revision, saw no weird side effects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11050
Differential Revision: https://secure.phabricator.com/D17531
Summary: Fixes T12434. I accidentally copy/pasted this too much in D17442.
Test Plan: Viewed a form edit page, no longer saw two copies of this action.
Reviewers: chad, cspeckmim
Reviewed By: chad, cspeckmim
Maniphest Tasks: T12434
Differential Revision: https://secure.phabricator.com/D17530
Summary: Ref T10390. Catch if the user doesn't have any dashboards they can edit and give them a helpful message instead.
Test Plan: Clean install, no dashboards, Click "Add to Dashboard" on ApplicationSearch results, see no dashboards message
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17528
Summary: Ref T10390. Dashboard usability is high enough that I think we should pin it by default for users to create custom home pages.
Test Plan: Review order of applications in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17527
Summary: Ref T10390. Fixes the missing "fa-dashboard" icon and adds a few more for an even 25.
Test Plan: Create new dashboard, see dashboard icon, select new dashboard icon.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17526
