1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-16 03:42:41 +01:00
Commit graph

17473 commits

Author SHA1 Message Date
epriestley
c0dc411d23 Update "phabricator/" for "topological" API changes
Summary: Ref T13325.

Test Plan:
  - Grepped for `topograph`.
  - Viewed a task graph since that's easy, looked fine.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13325

Differential Revision: https://secure.phabricator.com/D20599
2019-06-20 16:11:56 -07:00
epriestley
53f8ad14fa Fix an issue in Owners where a transaction change could show too many effects
Summary:
Fixes T13324. Ref PHI1288. Currently, if you edit an Owners package that has some paths with no trailing slashes (like `README.md`) so their internal names and display names differ (`/README.md` display, vs `/README.md/` internal), the "Show Details" in the transaction log shows the path as re-normalized even if you didn't touch it.

Instead, be more careful about handling display paths vs internal paths.

(This code on the whole is significantly less clear than it probably could be, but this issue is so minor that I'm hesitant to start ripping things out.)

Test Plan:
  - In a package with some paths like `/src/` and some paths like `/src`:
  - Added new paths.
  - Removed paths.
  - Changed paths from `/src/` to `/src`.
  - Changed paths from `/src` to `/src/`.

In all cases, the "paths" list and the transaction record identically reflected the edit in the way I expected them to.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13324

Differential Revision: https://secure.phabricator.com/D20596
2019-06-20 16:08:31 -07:00
epriestley
37e26f1b45 Improve rendering of "default value changed" custom form transactions to at least have all the information
Summary:
Ref T13319. Currently, transactions about changes to a default form value use a raw internal key for the affected field and don't show the actual value change.

An ideal implementation will likely require us to specialize a great deal of rendering, but we can do much better than we currently do without too much work:

  - Try to pull the actual `EditField` object for the key so we can `getLabel()` it and get a human-readable label (like `Visible To` instead of `policy.view`).
  - Add a "(Show Changes)" action that dumps the raw values as more-or-less JSON, so you can at least figure out what happened if you're sophisticated enough.

Test Plan:
Before:

{F6516640}

After:

{F6516642}

The quality of "Show Details" varies a lot. For some fields, like "Description", it's pretty good:

{F6516645}

For others, like "Assigned To", it's better than nothing but pretty technical:

{F6516647}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13319

Differential Revision: https://secure.phabricator.com/D20594
2019-06-19 13:47:07 -07:00
epriestley
caaa1394ef Don't count "Cc: x@y.com" as a legitimate recipient if the user who has "x@y.com" attached to their account has not verified the address
Summary:
Fixes T13317. On `admin.phacility.com`, an enterprising user added `noreply@admin.phacility.com` to their account. This caused them to become CC'd on several support issues over the last year, because we send mail "From" this address and it can get CC'd via reply/reply all/whatever else.

The original driving goal here is that if I reply to a task email and CC you on my reply, that should count as a CC in Phabricator, since this aligns with user intent and keeps them in the loop.

This misfire on `noreply@` is ultimately harmless (being CC'd does not grant the user access permission, see T4411), but confusing and undesirable. Instead:

  - Don't allow reserved addresses ("noreply@", "ssladmin@", etc) to trigger this subscribe-via-CC behavior.
  - Only count verified addresses as legitimate user recipients.

Test Plan:
  - Added a `bin/mail receive-test --cc ...` flag to make this easier to test.
  - Sent mail as `bin/mail receive-test --to X --as alice --cc bailey@verified.com`. Bailey was CC'd both before and after the change.
  - Sent mail as `bin/mail receive-test --to X --as alice --cc unverified@imaginary.com`, an address which Bailey has added to her account but not verified.
    - Before change: Bailey was CC'd on the task anyway.
    - After change: Bailey is not CC'd on the task.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13317

Differential Revision: https://secure.phabricator.com/D20593
2019-06-19 12:51:00 -07:00
epriestley
9f44ee3933 Use "link.getAttribute('href')", not "link.href", to bypass dark browser magic
Summary:
Ref T13302. In at least some browsers (including Safari and Chrome), when you write this:

```
<a href="#">...</a>
```

...and then access `<that node>.href`, you get `http://local-domain-whatever.com/path/to/current/page#` back.

This is wonderful, but not what we want. Access the raw attribute value instead, which is `#` in all browsers.

Test Plan:
  - In Safari, Chrome, and Firefox:
  - Clicked "Edit Subtasks" from a task.
  - Clicked "Select" buttons to select several tasks.
  - Before: Clicking these button incorrectly closed the dialog (because of D20573).
  - After: Clicking these buttons now selects tasks without closing the dialog.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13302

Differential Revision: https://secure.phabricator.com/D20590
2019-06-19 10:44:42 -07:00
epriestley
5d8ee504d6 Replace weird, redundant list of branches in Diffusion "Manage" UI with a link to the main branch list
Summary:
Fixes T13312. Currently, {nav Manage > Branches} has a list of branches on the same page. This has a few minor issues:

  - Pager is at the top (see T13312), which is weird.
  - "Default" icon is mystery meat.
  - Table is kind of pointless/redundant in general?

Previously, this table had more information about technical status of each branch (autoclose/track/publish) but most of these details have been simplified/eliminated, and the main "Branches" view now has more information than it did before.

Get rid of this and just link to the main view.

Test Plan: Viewed "Branches" in UI, saw a link to the main view instead of a weird table.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13312

Differential Revision: https://secure.phabricator.com/D20584
2019-06-18 15:19:53 -07:00
epriestley
dcf3ca8e04 When a user clicks a navigation link in a dialog, close the dialog
Summary:
Ref T13302. Currently, if you enable Quicksand (by clicking "Persistent Chat"), open a dialog with links in it (like "Create Subtask" with multiple available subtypes), and then follow a navigation link, the page content reloads behind the dialog but the dialog stays in the foreground.

Fix this by closing dialogs when users click navigation links inside them.

Test Plan:
With Quicksand enabled and disabled, clicked a subtask type in the "Create Subtask" dialog.

  - Before, Quicksand Disabled: Dialog stays on screen, then navigation occurs.
  - After, Quicksand Disabled: Dialog vanishes, then navigation occurs.
  - Before, Quicksand Enabled: Dialog stays on screen, navigation occurs behind it.
  - After, Quicksand Enabled: Dialog vanishes, then navigation occurs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13302

Differential Revision: https://secure.phabricator.com/D20573
2019-06-18 15:16:32 -07:00
epriestley
dda5c13ef5 Parse "shallow" frames in the Git "upload-pack" wire protocol parser
Summary:
Fixes T13309. If you void the warranty on a repository on disk and turn it into a shallow clone, Phabricator currently can't serve it.

We don't support hosting shallow working copies, but we should still parse and proxy the protocol rather than breaking in a mysterious way.

Test Plan:
  - Created a shallow working copy with `mv X X.full; git clone --depth Y file://.../X.full X` in the storage directory on disk.
  - Cloned it with `git clone <uri>`.
  - Deleted all the refs inside it so the wire only has "shallow" frames; cloned it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13309

Differential Revision: https://secure.phabricator.com/D20577
2019-06-18 15:14:52 -07:00
epriestley
7538286499 Fix missing link targets for "View Object" header buttons in HTML email
Summary:
See <https://discourse.phabricator-community.org/t/view-task-from-maniphest-e-mail-doesnt-have-url/2827>.

I added "View Task" / "View Commit" buttons recently but the logic for generating URIs isn't quite right. Fix it up.

Test Plan:
  - Commented on a task.
  - Used `bin/mail show-outbound --id ... --dump-html > out.html` to dump the HTML.
  - Previewed the HTML in a browser.
  - This time, actually clicked the button to go to the task.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20586
2019-06-18 13:20:56 -07:00
epriestley
6219d30f6b Recommend dumping database backups with "--compress --output X" instead of "| gzip > X"
Summary:
Fixes T13304. Shell pipes and redirects do not have robust behavior when errors occur. We provide "--compress" and "--output" flags as robust alternatives, but do not currently recommend their use.

  - Recommend their use, since their error handling behavior is more robust in the face of issues like full disks.
  - If "--compress" is provided but won't work because the "zlib" extension is missing, raise an explicit error. I believe this extension is very common and this error should be rare. If that turns out to be untrue, we could take another look at this.
  - Also, verify some flag usage sooner so we can exit with an error faster if you mistype a "bin/storage dump" command.

Test Plan: Read documentation, hit affected error cases, did a dump and spot-checked that it came out sane looking.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13304

Differential Revision: https://secure.phabricator.com/D20572
2019-06-18 11:37:14 -07:00
epriestley
1dd62f79ce Fix more "msort()" vs "msortv()" callsites
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-when-logging-in-with-mfa/2828>. The recent changes to turn `msort()` on a vector an error have smoked out a few more of these mistakes.

These cases do not meaningfully rely on sort stability so there's no real bug being fixed, but we'd still prefer `msortv()`.

Test Plan: Viewed MFA and External Account settings panels. Did a `git grep 'msort(' | grep -i vector` for any more obvious callsites, but none turned up.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20587
2019-06-18 11:36:22 -07:00
epriestley
731b45d818 In "bin/repository reparse", continue on "PhabricatorWorkerPermanentFailureException"
Summary:
Fixes T13315. See that task for discussion.

Without `--background`, we currently treat this as a catastrophic failure, but it's relatively routine for some repository states. We can safely continue reparsing other steps.

Test Plan: Ran `bin/repository reparse --all X --message` with commits faked to all be unreachable. Got warnings instead of a hard failure on first problem.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13315

Differential Revision: https://secure.phabricator.com/D20588
2019-06-18 11:32:54 -07:00
epriestley
aba7c98bae Skip loading transaction handles in an old migration
Summary:
Ref T13305. See that task for discussion.

This old migration may indirectly cause search index worker tasks to queue by loading handles. They'll fail since we later added `dateCreated` to the worker task table.

Use `needHandles(false)` (since we don't use them) to disable loading handles and avoid the problem.

Test Plan:
  - Ran `bin/storage upgrade -f` on an older instance (late 2016) and hit this issue.
  - Applied the patch, got a clean migration to modernity.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13305

Differential Revision: https://secure.phabricator.com/D20570
2019-06-17 13:57:47 -07:00
epriestley
bab35f28e4 Respect repository identities when selecting author vs auditor actions
Summary:
Depends on D20580. Fixes T13311. When we choose which actions to show a user, we can either show them "auditor" actions (like "raise concern") or "author" actions (like "request verification").

Currently, we don't show "author" actions if you're the author of the commit via an identity mapping, but we should. Use identity mappings where they exist.

(Because I've implemented `getEffectiveAuthorPHID()` in a way that requires `$data` be attached, it's possible this will make something throw a "DataNotAttached" exception, but: probably it won't?; and that's easy to fix if it happens.)

Test Plan:
See D20580. As `@alice`, viewed the commit in the UI.

  - Before: got auditor actions presented to me.
  - After: got author actions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13311

Differential Revision: https://secure.phabricator.com/D20581
2019-06-17 13:56:38 -07:00
epriestley
4450c90881 When triggering audits, respect committer identities when importing commits
Summary:
Ref T13311. We currently don't use committer identity mappings when triggering audits, so if a user is only associated with an identity via manual mapping we won't treat them as the author.

Instead, use the identity and manual mapping if they're available.

Test Plan:
  - Pushed a commit as `xyz <xyz@example.org>`, an address with no corresponding user.
  - In the UI, manually associated that identity with user `@alice`.
  - Ran `bin/repository reparse --publish <hash>` to trigger audits and publishing for the commit.
  - Before: observed the `$author_phid` was `null`.
  - After: observed the `$author_phid` is Alice.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13311

Differential Revision: https://secure.phabricator.com/D20580
2019-06-17 13:47:31 -07:00
epriestley
4af73a625f Don't require users be logged in to access the Logout controller, so users with no Spaces can log out
Summary:
Fixes T13310. Use cases in the form "users with no access to any spaces can not <do things>" are generally unsupported (that is, we consider this to mean that the install is misconfigured), but "log out" is a somewhat more reasonable sort of thing to do and easy to support.

Drop the requirement that users be logged in to access the Logout controller. This skips the check for access to any Spaces and allows users with no Spaces to log out.

For users who are already logged out, this just redirects home with no effect.

Test Plan:
  - As a user with access to no Spaces, logged out. (Before: error; after: worked).
  - As a logged-out user, logged out (was redirected).
  - As a normal user, logged out (normal logout).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13310

Differential Revision: https://secure.phabricator.com/D20578
2019-06-17 13:44:02 -07:00
epriestley
14b076578f In "Download Raw Diff", engage the chunk engine to handle 8MB+ changes
Summary:
Fixes T13313. The "Download Raw Diff" workflow in Differential currently uses an older way of interacting with Files that doesn't engage the chunk engine and can't handle 8MB+ files.

Update to `IteratorFileUploadSource` -- we're still passing in a single giant blob, but this approach can be chunked.

This will still break somewhere north of 8MB (it will break at 2GB with the PHP string limit if nowhere sooner, since we're putting the entire raw diff in `$raw_diff` rather than using a rope/stream) but will likely survive diffs in the hundreds-of-megabytes range for now.

Test Plan:
  - Added `str_repeat('x', 1024 * 1024 * 9)` to the `$raw_diff` to create a 9MB+ diff.
  - Configured file storage with no engine explicitly configured for >8MB chunks (i.e., "reasonably").
  - Clicked "Download Raw Diff".
  - Before: misleading file storage engine error ("no engine can store this file").
  - After: large, raw diff response.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13313

Differential Revision: https://secure.phabricator.com/D20579
2019-06-17 13:31:35 -07:00
epriestley
2bc045bab8 Fix another stray "msort()/msortv()" issue
Summary: Ref T13303. See B22967. This should be "msortv()" but didn't get updated properly.

Test Plan: The system works!

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13303

Differential Revision: https://secure.phabricator.com/D20585
2019-06-17 13:20:10 -07:00
epriestley
874282db75 Correct "msort()" vs "msortv()" to more fully stabilize transaction sorts after recent changes
Summary: Ref T13303. I upgraded this to a vector-based sort but forgot to type a "v", which means the sort has different stability under PHP 5.5. See D20582 for a root cause fix.

Test Plan: Locally, on PHP7, not much changes. I expect this to fix the odd selection of title stories in mail and notification stories on `secure`, which is running PHP 5.5.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13303

Differential Revision: https://secure.phabricator.com/D20583
2019-06-17 13:07:38 -07:00
epriestley
d3112392d1 Allow "Sign with MFA" to be applied as a comment action without requiring "CAN_EDIT"
Summary:
Fixes T13307. We currently require "CAN_EDIT" to sign actions, but it's fine to sign a comment with only "CAN_INTERACT".

Since the actions like "Accept Revision" already work like this, the fix is one line.

Test Plan: {F6488135}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13307

Differential Revision: https://secure.phabricator.com/D20574
2019-06-17 10:41:42 -07:00
epriestley
64a9500078 Add "bin/file migrate" options to support import of a local-disk backup for Phacility instances
Summary:
Ref T13306. Currently, there's no easy way to import a third-party local-disk file dump into a Phacility instance.

Add some more options to `bin/files migrate` to support this. In particular, this enables:

```
$ ./bin/files --from-engine local-disk --engine amazon-s3 --local-disk-source path/to/backup
```

...to import these files into S3 directly.

These are general-purpose options and theoretically useful in other use cases, although realistically those cases are probably very rare.

Test Plan: Used `bin/files` with the new options to move files in and out of local disk storage in an arbitrary backup directory. Got clean exports/imports.

Reviewers: amckinley

Maniphest Tasks: T13306

Differential Revision: https://secure.phabricator.com/D20571
2019-06-15 08:12:11 -07:00
epriestley
f1a588c771 Add a basic profiler to Herald transcripts
Summary:
Ref T13298. Add a simple profiler as a starting point to catch any egregiously expensive rules or conditions.

This doesn't profile rule actions, so if "Add subscriber" (or whatever) is outrageously expensive it won't show up on the profile. Right now, actions get evaluated inside the Adapter so they're hard to profile. A future change could likely dig them out without too much trouble. I generally expect actions to be less expensive than conditions.

This also can't pin down a //specific// condition being expensive, but if you see that `H123` takes 20s to evaluate you can probably guess that the giant complicated regex is the expensive part.

Test Plan: {F6473407}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13298

Differential Revision: https://secure.phabricator.com/D20566
2019-06-05 08:50:41 -07:00
epriestley
b7aacaa4d3 Differentiate Remarkup header sizes more clearly
Summary:
Ref PHI1275. Previously, see T591. See also T7963. Headers are currently very visually similar to one another, and similar to the text size:

{F6485441}

I think the design intent was to make it hard to make bad-looking documents, but all the headers end up being very samey.

Differentiate the sizes of the headers better so they're much more obvious (e.g., when scrolling through a document) and the different levels are more distinct.

This might be a little overboard, but we can always pull it back a bit if it's too much, and I think giving users more control in Remarkup (in cases where it doesn't create some weird syntax/parsing nightmare) is generally a good thing.

Test Plan: {F6485447}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20569
2019-06-04 16:03:32 -07:00
epriestley
d7890d08b8 Add "bin/herald rule ..." to modify Herald rules from the CLI
Summary:
Depends on D20566. Ref T13298. See PHI1280. Currently, there's no clean way to disable problematic personal rules. This comes up occasionally and sometimes isn't really the best approach to solving a problem, but is a generally reasonable capability to provide.

Allow Herald rules (including personal rules) to be disabled/enabled via `bin/herald rule ... --disable/--enable`.

Test Plan: Used the CLI to disable and enable a personal rule.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jmeador

Maniphest Tasks: T13298

Differential Revision: https://secure.phabricator.com/D20567
2019-06-04 07:12:15 -07:00
epriestley
760406762a When a revision is closed, always promote it out of draft
Summary:
Fixes T13300. Currently, if you create a revision and then immediately land it (either using `--draft` or just beating Harbormaster to the punch) it can be stuck in "Draft" forever.

Instead, count landing changes like this as a publishing action.

Test Plan:
  - Used `arc diff --hold` to create a revision, then pushed the commit immediately.
  - Before change: revision closed, but was stuck in draft.
  - After change: revision closed and was promoted out of draft.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13300

Differential Revision: https://secure.phabricator.com/D20565
2019-06-04 06:56:22 -07:00
epriestley
67f062b004 When triggering audits, count "Accepted" revisions as successfully reviewed
Summary:
See PHI1118. That issue may describe more than one bug, but the recent ordering changes to the import pipeline likely make this at least part of the problem.

Previously, commits would always close associated revisions before we made it to the "publish" step. This is no longer true, so we might be triggering audits on a commit before the associated revision actually closes.

Accommodate this by counting a revision in either "Accepted" or "Published (Was Previously Accepted)" as "reviewed".

Test Plan:
  - With commit C affecting paths in package P with "Audit Unreviewed Commits and Commits With No Owner Involvement", associated with revision R, with both R and C authored by the same user, and "R" in the state "Accepted", used `bin/repository reparse --publish <hash>` to republish the commit.
  - Before change: audit by package P triggered.
  - After change: audit by package P no longer triggered.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20564
2019-05-30 17:18:11 -07:00
epriestley
81134d7e7d After reloading transactions for the recipient while building transaction mail, put them in the input order
Summary:
Ref T13303. In D20525 I fixed an issue where transaction rendering could use cached values with the wrong viewer by reloading transactions.

However, reloading transactions may also reorder them as a side effect, since `withPHIDs(...)` does not imply an order. This can make transaction rendering order in mail wrong/inconsistent.

Instead, reorder the transactions before continuing so mail transaction order is consistent.

Test Plan: Applied a group of transactions to a task, saw a more consistent rendering order in mail after the change.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13303

Differential Revision: https://secure.phabricator.com/D20563
2019-05-30 17:14:54 -07:00
epriestley
9a32a563f0 Add a "View Task" button to HTML mail from Maniphest
Summary:
See downstream <https://phabricator.wikimedia.org/T1050>. Some time ago, we added a "View Revision" button to Differential mail. This hasn't created any problems and generally seems good / desirable.

It isn't trivial to just add everywhere since we need a translation string in each case, but at least add it to Maniphest for now. Going forward, we can fill in more applications as they come up.

Test Plan:
Used `bin/mail show-outbound --id <x> --dump-html`:

{F6470461}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20561
2019-05-30 15:24:22 -07:00
epriestley
fb5dec4c03 Require valid comments to contain at least one non-whitespace character
Summary:
See downstream <https://phabricator.wikimedia.org/T88655>. This is very marginal, but we currently allow comments consisting of //only// whitespace.

These are probably always mistakes, so treat them like completely empty comments.

(We intentionally do not trim leading or trailing whitespace from comments when posting them becuase leading spaces can be used to trigger codeblock formatting.)

Test Plan:
  - Posted empty, nonempty, and whitespace-only comments.
  - Whitespace-only comments now have the same behavior as truly empty comments (e.g., do not actually generate a transaction).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20562
2019-05-30 15:15:24 -07:00
epriestley
8747204807 Clean up "phabricator.timezone" configuration instructions a little bit
Summary: These instructions are fairly old and can be a little fancier and more clear in the context of modern Phabricator. Drop the reference to "HPHP", link the actual timezone list, wordsmith a little.

Test Plan: d( O_o )b

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20560
2019-05-30 15:08:11 -07:00
epriestley
e5a1681903 Render timezone names more readably, with spaces rather than underscores ("America/Los Angeles", not "America/Los_Angeles").
Summary:
See downstream <https://phabricator.wikimedia.org/T902>. Currently, timezones are rendered with their raw internal names (like `America/Los_Angeles`) which include underscores.

Replacing underscores with spaces is a more human-readable (and perhaps meaningfully better for things like screen readers, although this is pure speculation).

There's some vague argument against this, like "administrators may need to set a raw internal value in `phabricator.timezone` and this could mislead them", but we already give a pretty good error message if you do this and could improve hinting if necessary.

Test Plan: Viewed timezone list in {nav Settings} and the timezone "reconcile" dialog, saw a more-readable "Los Angeles".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20559
2019-05-30 15:03:11 -07:00
epriestley
53b9acfb7d Test for "CAN_INTERACT" on comment edits in a way that survives objects which only implement "CAN_VIEW"
Summary:
Ref T13289. See D20551. In D20551, I implemented some "CAN_INTERACT" checks against certain edits, but these checks end up testing "CAN_INTERACT" against objects like Conpherence threads which do not support a distinct "CAN_INTERACT" permission. I misrembered how the "CAN_INTERACT" fallback to "CAN_VIEW" actually works: it's not fully automatic, and needs some explicit "interact, or view if interact is not available" checks.

Use the "interact" wrappers to test these policies so they fall back to "CAN_VIEW" if an object does not support "CAN_INTERACT". Generally, objects which have a "locked" state have a separate "CAN_INTERACT" permission; objects which don't have a "locked" state do not.

Test Plan: Created and edited comments in Conpherence (or most applications other than Maniphest).

Reviewers: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20558
2019-05-28 10:14:43 -07:00
epriestley
ce6fc5be90 Fix a looping workflow when trying to submit a partially-effectless transaction group
Summary:
Ref T13289. If you do this:

  - Subscribe to a task (so we don't generate a subscribe side-effect later).
  - Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
  - Submit the transaction group.

...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".

If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.

Instead, retain the "continue" bit through the MFA.

Also, don't show "You can't sign an empty transaction group" if there's a comment.

See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.

Test Plan:
  - Went through the workflow above.
  - Before: looping workflow.
  - After: "Continue" carries through the MFA gate.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20552
2019-05-23 19:16:17 -07:00
epriestley
719dd6d3f4 Remove the "search_documentfield" table
Summary: Ref T11741. See PHI1276. After the switch to "Ferret", this table has no remaining readers or writers.

Test Plan:
  - Ran `bin/storage upgrade -f`, no warnings.
  - Grepped for class name, table name, `stemmedCorpus` column; got no relevant hits.
  - Did a fulltext search.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11741

Differential Revision: https://secure.phabricator.com/D20549
2019-05-23 19:11:38 -07:00
epriestley
aacc62463d Prevent editing and deleting comments in locked conversations
Summary:
Ref T13289. This tightens up a couple of corner cases around locked threads.

Locking is primarily motivated by two use cases: stopping nonproductive conversations on open source installs (similar to GitHub's feature); and freezing object state for audit/record-keeping purposes.

Currently, you can edit or remove comments on a locked thread, but neither use case is well-served by allowing this. Require "CAN_INTERACT" to edit or remove a comment.

Administrators can still remove comments from a locked thread to serve "lock a flamewar, then clean it up", since "Remove Comment" on a comment you don't own is fairly unambiguously an administrative action.

Test Plan:
  - On a locked task, tried to edit and remove my comments as a non-administrator. Saw appropriate disabled UI state and error dialogs (actions were disallowed).
  - On a locked task, tried to remove another user's comments as an administrator. This works.
  - On a normal task, edited comments normally.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20551
2019-05-23 19:04:55 -07:00
epriestley
f838ad1827 Fix two straggling pagination issues in Drydock
Summary:
Ref T13289. See <https://discourse.phabricator-community.org/t/fatal-error-in-pagination-in-drydock-resources-host-logs-all-logs/2735>.

`bin/drydock lease` and the web UI for reviewing all object logs when there is more than one page of logs didn't get fully updated to the new cursors.

  - Use a cursor pager in `bin/drydock lease`.
  - Implement `withIDs()` in `LeaseQuery` so the default paging works properly.

Test Plan:
  - Ran `bin/drydock lease`, got a lease with log output along the way.
  - Set page size to 2, viewed host logs with multiple pages, paged to page 2.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20553
2019-05-23 18:50:06 -07:00
epriestley
2e2dc47f07 In the Herald test console, don't consider transactions that Herald rules applied
Summary:
Depends on D20546. Ref T13283. Currently, if you do something (transactions "A", "B") and Herald does some things in response (transaction "C"), Herald acts only on the things you did ("A", "B") since the thing it did ("C") didn't exist yet, until it ran.

However, if you use the test console to test rules against the object we'll pick up all three transactions since they're all part of the same group. This isn't ideal.

To fix this, skip transactions which Herald applied, since it obviously didn't consider them when it was evaluating.

Test Plan:
  - Created a revision, in the presence of a Herald rule that adds reviewers.
  - Then, ran the revision through the test console.
  - Before: saw the "Herald added reviewers: ..." transaction in the transaction group Herald evaluated.
  - After: saw only authentic human transactions.

{F6464064}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

Differential Revision: https://secure.phabricator.com/D20547
2019-05-22 16:34:34 -07:00
epriestley
31e623afcc Use the same transaction group ID for transactions applied indirectly by a sub-editor
Summary:
Ref T13283. Currently, each Editor sets its own group ID, so if you create a revision and then Herald does some stuff, the two groups of transactions get different group IDs.

This means the test console is slightly misleading (it will only pick up the Herald transactions). It's going to be misleading anyway (Herald obviously can't evaluate Herald transactions) but this is at least a little closer to reality and stops Herald actions from masking non-Herald actions.

Test Plan:
  - Created a revision. Herald applied one transaction.
  - Used the test console.
  - Before: The test console only picked up the single most recent Herald transaction.
  - After: The test console picked up the whole transaction group.

{F6464059}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

Differential Revision: https://secure.phabricator.com/D20546
2019-05-22 16:33:03 -07:00
epriestley
f6af1c4374 When creating a Phriction document, mark initial transactions as "create" transactions to fix weird email
Summary:
Ref T13289. When you create a Phriction document, you currently get an email with the whole new content as a "diff".

You also get extra transactions in the email and on the page.

This is because Phriction isn't on EditEngine and doesn't mark "create" transactions in a modern way. Get them marked properly to fix these obviously-broken behaviors. This can all go away once Phriction switches to EditEngine, although I don't have any particular plans to do that in the immediate future.

Test Plan:
  - Created a new document, viewed email, no longer saw redundant "edited content" transaction or "CHANGES TO CONTENT" diff.
  - Updated a document, viewed email, got interdiff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20548
2019-05-22 16:28:25 -07:00
epriestley
b95bf722d5 Drop the "update revision with commit diff" transaction if the revision is already closed
Summary:
Ref T13290. Prior to recent changes, if we parsed some commit C which was associated with a revision R, but R was already closed, we'd skip the whole set of updates because the "close the revision" transaction would fail and we'd throw because we did not `setContinueOnNoEffect()`.

We now continue on no effect so we can get the edge ("commit has revision" / "revision has commit"), since we want it in all cases, but this means we may also apply an extra "Updated revision to reflect committed changes" transaction and new diff. This can happen even if we're careful about not trying to apply this transaction to closed revisions, since two workers may race. (Today, we aren't too careful about this.)

To fix this, just make this transaction no-op itself if the revision is already closed by the time it tries to apply.

This happened on D20451 because a merge commit with the same hash as the last diff was pushed, but it's easiest to reproduce by just running `bin/repository reparse --message <commit>`, which updates related revisions with a new diff every time.

Test Plan:
  - Ran `bin/repository reparse --messsage <commit>` several times, on a commit with an associated revision.
  - Before: each run attached a new diff and created a new "updated to reflect committed changes" transaction.
  - After: repeated runs had no effects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13290

Differential Revision: https://secure.phabricator.com/D20545
2019-05-22 16:26:45 -07:00
epriestley
1eff4fdca3 Prevent "Differential Revision: ..." from counting as a mention in commit messages
Summary:
Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention.

Stop it from doing that, since these mentions are silly/redundant/unintended.

The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set".

Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291, T13290

Differential Revision: https://secure.phabricator.com/D20544
2019-05-22 16:22:01 -07:00
epriestley
fa4dcaa3aa Stabilize sorting of feed stories with similar strength
Summary:
See PHI1222. When we publish several transactions to feed at once, we sort them by "action strength" to figure out which one gets to be the title story.

This sort currently uses `msort()`, which uses `asort()`, which is not a stable sort and has inconsistent behavior across PHP versions:

{F6463721}

Switch to `msortv()`, which is a stable sort. Previously, see also T6861.

If all transactions have the same strength, we'll now consistently pick the first one.

This probably (?) does not impact anything in the upstream, but is good from a consistency point of view.

Test Plan:
Top story was published after this change and uses the chronologically first transaction as the title story.

Bottom story was published before this change and uses the chronologically second transaction as the title story.

Both stories have two transactions with the same strength ("create" + "add reviewer").

{F6463722}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20540
2019-05-22 15:50:59 -07:00
epriestley
f91bef64f1 Stack chart functions in a more physical way
Summary:
Ref T13279. See that task for some discussion.

The accumulations of some of the datasets may be negative (e.g., if more tasks are moved out of a project than into it) which can lead to negative area in the stacked chart.

Introduce `min(...)` and `max(...)` to separate a function into points above or below some line, then mangle the areas to pick the negative and positive regions apart so they at least have a plausible physical interpretation and none of the areas are negative.

This is presumably not a final version, I'm just trying to produce a chart that isn't a sequence of overlapping regions with negative areas that is "technically" correct but not really possible to interpret.

Test Plan: {F6439195}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20506
2019-05-22 05:40:39 -07:00
epriestley
f190c42bcd Store charts earlier and build them out a little later
Summary:
Ref T13279. Currently, we store a fairly low-level description of functions and datasets in a chart. This will create problems with (for example) translating function labels.

If you view a chart someone links you, it should say "El Charto" if you speak Spanish, not "The Chart" if the original viewer speaks English.

To support this, store a slightly higher level version of the chart: the chart engine key, plus configuration parameters. This is very similar to how SearchEngine works.

For example, the burndown chart now stores a list of project PHIDs, instead of a list of `[accumulate [sum [fact task.open <project-phid>]]]` functions.

(This leaves some serialization code with no callsites, but we may eventually have a "CustomChartEngine" which stores raw functions, so I'm leaving it for now.)

As a result, function labels provided by the chart engine are now translatable.

(Note that the actual chart is meaningless since the underlying facts can't be stacked like they're being stacked, as some are negative in some areas of their accumulation.)

Test Plan: {F6439121}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20504
2019-05-22 05:39:32 -07:00
epriestley
493a6b72c1 Automatically select the range for charts in a general way
Summary:
Ref T13279. Replace the hard-coded default range with a range computed by examining the chart data.

Instead of having a "Dataset" return a blob of wire data, "Dataset" now returns a structure with raw wire data plus a range. I expect to add more structured data here in future changes (tooltip/hover event data, maybe function labels).

Test Plan: {F6439101}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20503
2019-05-22 05:36:58 -07:00
epriestley
e90360c289 Wrap "<min, max>" chart domain pairs in an "Interval" class
Summary: Ref T13279. Slightly simplify domain handling by putting all the "[x, y]" stuff in an Interval class. I'm planning to do something similar for ranges next, so this should make that easierr.

Test Plan: Viewed chart, saw same chart as before.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20502
2019-05-22 05:28:49 -07:00
epriestley
a80426b339 Provide chart function labels over the wire instead of making them up
Summary: Ref T13279. Makes charts incrementally more useful by allowing the server to provide labels and colors for functions.

Test Plan: {F6438872}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20501
2019-05-22 05:22:59 -07:00
epriestley
c6052b41a6 Label important data on charts
Summary:
Ref T13279. Adds client-side support for rendering function labels on charts, then labels every function as important data.

Works okay on mobile, although I'm not planning to target mobile terribly heavily for v0.

Test Plan: {F6438860}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20500
2019-05-22 05:21:26 -07:00
epriestley
81456db559 Roughly support stacked area charts
Summary:
Ref T13279. This adds support for:

  - Datasets can have types, like "stacked area".
  - Datasets can have multiple functions.
  - Charts can store dataset types and datasets with multiple functions.
  - Adds a "stacked area" dataset.
  - Makes D3 actually draw a stacked area chart.

Lots of rough edges here still, but the result looks slightly more like it's supposed to look.

D3 can do some of this logic itself, like adding up the area stacks on top of one another with `d3.stack()`. I'm doing it in PHP instead because I think it's a bit easier to debug, and it gives us more options for things like caching or "export to CSV" or "export to API" or rendering a data table under the chart or whatever.

Test Plan: {F6427780}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20498
2019-05-22 05:19:41 -07:00
epriestley
0776b5ca2c Update D3 to the current version
Summary:
Ref T13279. Old D3 seems perfectly fine, but most of the good references seem to have been written by people who update D3 more than once every 10 years (???).

This requires some minor API changes, see next diff.

Test Plan: See next diff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20497
2019-05-22 05:16:00 -07:00