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

3140 commits

Author SHA1 Message Date
epriestley
c5960c71f9 Splice in a patch to remove Phriction content rows with no document
The unique key on <documentPHID, version> may fail to apply if any content
rows don't have a valid document. This is rare, but we have some old random
garbage rows on "secure.phabricator.com" which prevent the next patch from
applying. Just toss these rows, they're junk.
2018-08-31 12:02:52 -07:00
epriestley
3b1294cf45 Store Phriction max version on Document, improve editing rules for editing documents with drafts
Summary:
Ref T13077. We need to know the maximum version of a document in several cases, so denormalize it onto the Document object.

Then clean up some behaviors where we edit a document with, e.g., 7 versions but version 5 is currently published. For now, we: edit starting with version 7, save as version 8, and immediately publish the new version.

Test Plan:
  - Ran migration.
  - Edited a draft page without hitting any weird version errors.
  - Checked database for sensible `maxVersion` values.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19625
2018-08-30 10:12:51 -07:00
epriestley
0a77b0e53e Work around an issue in MariaDB where dropping a column from a UNIQUE KEY fails
Summary:
See T13193. See T13077. If we drop a column which is part of a UNIQUE KEY, MariaDB raises an error.

This is probably a bad idea on our side anyway, but in this case it wasn't an obviously bad idea.

To get around this:

  - Drop the unique key, if it exists, before dropping the column.
  - Explicitly add the new unique key afterward.

Test Plan: Ran `bin/storage upgrade` locally without issue, but I'm on MySQL. Will follow up on T13193.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19624
2018-08-30 06:25:39 -07:00
epriestley
876638e428 Add a UI element for navigating between versions of a Phriction document
Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear.

Test Plan: {F5841997}

Reviewers: amckinley

Maniphest Tasks: T13077, T4815

Differential Revision: https://secure.phabricator.com/D19622
2018-08-29 13:49:15 -07:00
epriestley
50f4adef64 Remove on-object mailkeys from Phriction
Summary: Depends on D19619. Ref T13065. Ref T13077. Migrate Phriction mail keys to the new infrastructure and drop the column.

Test Plan: Ran migrations, spot-checked the database.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13077, T13065

Differential Revision: https://secure.phabricator.com/D19620
2018-08-29 13:43:13 -07:00
epriestley
64cee4a902 Move Phriction internal document/content references from IDs to PHIDs
Summary:
Ref T13077. This is mostly just a small cleanup change, even though the actual change is large.

We currently reference content and document objects from one another with `contentID` and `documentID`, but this means that `contentID` must be nullable. Switching to PHIDs allows the column to be non-nullable.

This also supports reorienting some current and future transactions around PHIDs, which is preferable for the API. In particular, I'm adding a "publish version X" transaction soon, and would rather callers pass a PHID than an ID or version number, since this will make the API more consistent and powerful.

Today, `contentID` gets used as a cheaty way to order documents by (content) edit time. Since PHIDs aren't orderable and stuff is going to become actually-revertible soon, replace this with an epoch timestamp.

Test Plan:
  - Created, edited, moved, retitled, and deleted Phriction documents.
  - Grepped for `documentID` and `contentID`.
  - This probably breaks //something// but I'll be in this code for a bit and am likely to catch whatever breaks.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19619
2018-08-29 13:41:24 -07:00
epriestley
4afb6446d9 Allow DocumentView to render with a curtain, and make Phriction use a curtain
Summary:
Depends on D19616. Ref T13077. Fixes T8172. In the last round of design updates, a lot of actions got stuffed into "Actions" menus.

I never really got used to these and think they're a net usability loss, and broadly agree with the feedback in T8172. I'd generally like to move back toward a state where actions are available on the page, not hidden in a menu.

For now, just put a curtain view on these pages. This could be refined later (e.g., stick this menu to the right hand side of the screen) depending on where other Phriction changes go.

(Broadly, I'm also not satisfied with where we ended up on the fixed-width pages like Diffusion > Manage, Config, and Instances. In contrast, I //do// like where we ended up with Phortune in terms of overall design. I anticipate revisiting some of this stuff eventually.)

Test Plan:
  - Looked at Phriction pages on desktop/tablet/mobile/printable -- actions are now available on the page.
  - Looked at other DocumentView pages (like Phame blogs) -- no changes for now.

Reviewers: amckinley

Maniphest Tasks: T13077, T8172

Differential Revision: https://secure.phabricator.com/D19617
2018-08-28 14:58:05 -07:00
epriestley
614f9ba1fb Allow unit test results to specify that their details are formatted with remarkup when reporting to "harbormaster.sendmessage"
Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc.

Test Plan: {F5840098}

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13189, T13088, T9951

Differential Revision: https://secure.phabricator.com/D19615
2018-08-28 13:26:11 -07:00
epriestley
2f5c6541fc Add an "Activated Epoch" and an "Acquired Epoch" to Drydock Leases
Summary: Ref T13189. See PHI690. When a lease is first acquired or activated, note the time. This supports better visibility into queue lengths. For now, this is only queryable via DB and visible in the UI, but can be more broadly exposed in the future.

Test Plan: Landed a revision, saw the leases get sensible timestamps for acquisition/activation.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19613
2018-08-27 11:27:45 -07:00
epriestley
415de4ce37 Repaint filetree more consistently for mobile/device views
Summary:
Ref T13189. See <https://discourse.phabricator-community.org/t/diffusion-differential-mobile-layout-broken-when-enabling-file-tree/1751>.

We currently call a nonexistent `resetdrag()` which does nothing. Some sequences of interactions can result in a blank left column in mobile/device widths.

Repaint the filetree away more consistently on device change.

Test Plan: Viewed a revision, toggled filetree off + on, resized to narrow width. Before: bad left margin, JS console error. After: proper repaint at device breakpoint, no JS console error.

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19611
2018-08-27 10:32:47 -07:00
epriestley
8a6d767843 Fix a minor text alignment issue for static text comment actions like "Accept Revision"
Summary:
Ref T13187. See PHI836. The "action" comment actions in Differential (Accept, Reject, etc) render a single line of descriptive text. This is currently slightly misaligned.

Give it similar sizing information to the label element to the left, so it lines up properly.

Test Plan:
Note that "Request Review" and "This revision will be..." are now aligned:

{F5828077}

Reviewers: amckinley

Maniphest Tasks: T13187

Differential Revision: https://secure.phabricator.com/D19600
2018-08-24 10:12:06 -07:00
epriestley
75a5dd8d8c Add more accessibility labels for screen readers
Summary:
Depends on D19594. See PHI823. Ref T13164.

  - Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
  - Add a label for the floating header display options menu in Differential.
  - Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.

Test Plan:
Viewed a revision with `?__aural__=true`:

  - Saw "Remove Action: ..." label.
  - Saw "Display Options" label.
  - Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19595
2018-08-17 13:31:51 -07:00
epriestley
9a15129b40 Remove 750ms timeout on owners path validation
Summary:
Ref T13164. See PHI748. Path validation has a 750ms timeout which blames to rP5038ab850c, in 2011.

Production path validation is sometimes taking more than 750ms, particularly on the initial page load where we may validate many paths simultaneously.

I have no idea why we have this timeout, and it isn't consistent with how we perform other AJAX requests. Just remove it.

Test Plan:
  - Reproduced issue in production, saw all validation calls failing at 750ms. Actual underlying calls succeed, they just take more than 750ms to resolve.
  - Loaded path validator locally, got green checkmark.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19575
2018-08-13 13:52:26 -07:00
epriestley
e5906f4e12 In Differential standalone views, disable some keyboard shortcuts which don't work
Summary:
Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files.

We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI.

Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled.

Test Plan:
  - Viewed a revision in normal and standalone views.
  - No changes in normal view, and all keys still work ("N", "P", etc).
  - In standalone view, "?" no longer shows nonfunctional key commands.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19571
2018-08-13 08:59:05 -07:00
Austin McKinley
a6951a0a5a Add migration to encourage rebuilding repository identities
Summary: Ref T12164. Defines a new manual activity that suggests rebuilding repository identities before Phabricator begins to rely on them.

Test Plan:
- Ran migration, observed expected setup issue: {F5788217}
- Ran `bin/config done identities` and observed setup issue get marked as done.
- Ran `/bin/storage upgrade --apply phabricator:20170912.ferret.01.activity.php` to make sure I didn't break the reindex migration; observed reindex setup issue appear as expected.
- Ran `./bin/config done reindex` and observed reindex issue cleared as expected.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19497
2018-08-10 13:47:03 -07:00
epriestley
8d8086fccf Add Spaces support to Phriction
Summary:
Ref T13164. See PHI774. Fixes T12435.

Since Phriction is hierarchical, there isn't a super strong motivation to support Spaces: you can generally set policies on a small number of documents to get the desired effective policy behavior.

However, it still improves consistency and there's no reason //not// to support Spaces. In the case where you have some moderately weird/complex policy on one or more Spaces, using Spaces to define the policy behavior can make things a bit simpler and easier to understand.

This probably doesn't actually fix whatever the root problem in T12435 was (complicated, non-hierarchical access policies?). See also a bunch of discussion in T12442. So we might end up going beyond this to address other use cases, but I think this is reasonable regardless.

Test Plan: Created and edited Phriction documents and shifted them between Spaces. Searched by Space, etc.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13164, T12435

Differential Revision: https://secure.phabricator.com/D19553
2018-07-31 10:24:28 -07:00
epriestley
8374201620 Add a more specific CSS rule to make Spaces headers in projects colored red
Summary:
Depends on D19551. Ref T13164. Projects use a special kind of header setup that has a more specific CSS rule to make content black. Add an even more specific rule to make it red.

(This could probably be disentangled a bit and isn't necessarily the cleanest fix, but I poked at it for a few minutes and didn't come up with anything cleaner.)

Test Plan: Viewed projects in spaces, saw the space names colored red properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19552
2018-07-31 10:23:35 -07:00
epriestley
13cac5c362 Add Spaces to Projects
Summary:
See PHI774. Ref T13164. There is no reason projects //don't// support Spaces, just a vague concern that it's not hugely useful and might be a bit confusing.

However, it's at least somewhat useful (to improve consistency and reduce special casing) and doesn't necessarily seem more confusing than Projects are anyway. Support is trivial from a technical point of view, so just hook it up.

Test Plan: Created new projects, shifted projects between spaces. The support is all pretty much automatic.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19549
2018-07-31 10:15:41 -07:00
epriestley
4e84d4d458 Allow the haunted comment panel ("Z") to take up more vertical room
Summary:
Ref T13151. See PHI685. When you haunt the panel, we only let it take up part of the screen. Let it take up slightly more of the screen so that it's more likely to fit completely on-screen without needing to scroll.

The behavior when it does scroll is fine (you get a scrollbar if your OS/browser is set up to show them) so this is a bit trivial/silly, but seems fine and doesn't have a big JS maintenance cost or anything.

Test Plan: Pressed "Z", resized my window to a weird tiny useless size, got slightly better (I guess) behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19480
2018-06-07 13:19:35 -07:00
epriestley
7729c51cc4 Fix an issue where scrolling down, then up, then down fails to show changeset header in Differential
Summary: Ref T13151. See PHI616. There's a bug where the current banner changeset isn't cleared correctly when we hide the banner.

Test Plan:
  - View revision with several changesets.
  - Scroll down slowly through first changeset until banner appears.
  - Scroll up until banner disappears.
  - Scroll back down.
  - Before: banner fails to reappear (code still thinks it's visible and we don't want to update it).
  - After: banner reappears correctly.

Reviewers: amckinley, jmeador

Reviewed By: jmeador

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19474
2018-06-07 12:02:18 -07:00
epriestley
a894c99935 Add "max-width: 100%;" to stop large images from overflowing the new rendering engine UI
Summary:
Fixes T13148. Ref T13105. The new document rendering engine for images let them overflow the UI bounds.

Add `max-width: 100%;` to keep them contained.

Test Plan:
  - Viewed a very wide image in Safari, Firefox and Chrome. Saw sensible rendering.
  - Also viewed a normal image, saw normal behavior.

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T13148, T13105

Differential Revision: https://secure.phabricator.com/D19457
2018-06-01 14:53:10 -07:00
epriestley
31ee49b14d Fix Javascript busy loop when trying to delete tokens from an empty tokenizer
Summary:
Fixes T13147. In D19437, I changed this logic to support deleting the `""` (empty string) token, but `[].pop()` returns `undefined`, not `null`, if the list is empty and I didn't think to try deleting an empty input.

Fix the logic so we don't end up in a loop if the input is empty.

Test Plan:
  - In any browser, deleted all tokens in a tokenizer; then pressed delete again.
  - Before: tab hangs in an infinte loop.
  - After: smooth sailing.

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T13147

Differential Revision: https://secure.phabricator.com/D19456
2018-06-01 14:51:06 -07:00
Austin McKinley
fe5fde5910 Assign RepositoryIdentity objects to commits
Summary: Depends on D19429. Depends on D19423. Ref T12164. This creates new columns `authorIdentityPHID` and `committerIdentityPHID` on commit objects and starts populating them. Also adds the ability to explicitly set an Identity's assignee to "unassigned()" to null out an incorrect auto-assign. Adds more search functionality to identities. Also creates a daemon task for handling users adding new email address and attempts to associate unclaimed identities.

Test Plan: Imported some repos, watched new columns get populated. Added a new email address for a previous commit, saw daemon job run and assign the identity to the new user. Searched for identities in various and sundry ways.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19443
2018-05-31 07:28:23 -07:00
Austin McKinley
f191a66490 Add controllers/search/edit engine functionality to RepositoryIdentity
Summary: Depends on D19423. Ref T12164. Adds controllers capable of listing and editing `PhabricatorRepositoryIdentity` objects. Starts creating those objects when commits are parsed.

Test Plan: Reparsed some revisions, observed objects getting created in the database. Altered some `Identity` objects using the controllers and observed effects in the database. No attempts made to validate behavior under "challenging" author/committer strings.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19429
2018-05-31 07:03:25 -07:00
Austin McKinley
cd84e53c44 Begin building out RepositoryIdentity indirection layer
Summary: Ref T12164. Start building initial objects for managing `RepositoryIdentity` objects. This won't land until much more of the infrastructure is in place.

Test Plan: Ran `bin/storage upgrade` and observed expected table.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19423
2018-05-31 07:01:16 -07:00
epriestley
d280b24239 Fix "arc paste" to stop creating pastes with an empty string ("") as the "language"
Summary:
See PHI652. When you `echo x | arc paste` today, you end up with a Paste object that has the empty string as its "language".

This is normally not valid. Pastes where the language should be autodetected should have the value `null`, not the empty string.

This behavior likely changed when `paste.create` got rewritten in terms of `paste.edit`. Adjust the implementation so it only adds the LANGUAGE transaction if there's an actual language.

Also, fix an issue where you can't use the "delete" key to delete tokens with the empty string as their value.

Test Plan:
  - Created a paste with `echo x | arc paste`, got a paste in autodetect mode instead of with a bogus language value.
  - Created a paste with `echo x | arc paste --lang rainbow`, got a rainbow paste.
  - Deleted an empty string token with the keyboard.
  - Deleted normal tokens with the keyboard.
  - Edited subscribers/etc normally with the keyboard and mouse to make sure I didn't ruin anything.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19437
2018-05-09 13:22:58 -07:00
epriestley
26c0db8dd7 Allow navigation breadcrumbs to be marked as "always visible" so they show up on phones
Summary:
See PHI624. Some of the mobile navigation and breadcrumbs in support pacts aren't as good as they could be.

In particular, we generally collapse crumbs on mobile to just the first and last crumbs. The first crumb is the application; the last is the current page.

On `/PHIxxx` pages, the first crumb isn't very useful since the Support landing page is two levels up: you usually want to go back to the pact, not all the way back to the Support landing page.

We also don't need the space since the last crumb (`PHIxxx`) is always small.

Allow Support and other similar applications to tailor the crumb behavior more narrowly if they end up in situations like this.

Test Plan:
  - With an additional change to instances (see next diff), viewed a support issue page (`/PHI123`) on mobile and desktop.
  - Saw a link directly back to the pact on both mobile and desktop.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19438
2018-05-09 13:21:47 -07:00
epriestley
4a98e0ff65 Allow Owners packages to be configured to ignore generated paths in Differential
Summary:
Depends on D19427. Ref T13130. See PHI251. Support configuring owners packages so they ignore generated paths.

This is still a little rough. A couple limitations:

  - It's hard to figure out how to use this control if you don't know what it's for, but we don't currently have a "CheckboxesEditField". I may add that soon.
  - The attribute ignore list doesn't apply to Diffusion, only Differential, which isn't obvious. I'll either try to make it work in Diffusion or note this somewhere.
  - No documentation yet (which could mitigate the other two issues a bit).

But the actual behavior seems to work fine.

Test Plan:
  - Set a package to ignore paths with the "generated" attribute. Saw the package stop matching generated paths in Differential.
  - Removed the attribute from the ignore list.
  - Tried to set invalid attributes, got sensible errors.
  - Queried a package with Conduit, got the ignored attribute list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19428
2018-05-05 08:47:29 -07:00
epriestley
dc510354c3 Remove explicit "mailKey" from Owners packages
Summary:
Depends on D19426. Ref T13130. Ref T13065. While I'm making changes to Owners for "Ignore generated paths", clean up the "mailKey" column.

We recently (D19399) added code to automatically generate and manage mail keys so we don't need a ton of `mailKey` properties in the future. Migrate existing mail keys and blow away the explicit column on packages.

Test Plan: Ran migration, manually looked at the database and saw sensible data. Edited a package to send some mail, which looked good.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130, T13065

Differential Revision: https://secure.phabricator.com/D19427
2018-05-05 08:47:08 -07:00
epriestley
afc3099ee7 Add a view option to disable blame in Diffusion and fix some view transition bugs
Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.

Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.

Test Plan:
  - Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
  - Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
  - Viewed stuff in Files (no blame UI options).
  - Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13105

Differential Revision: https://secure.phabricator.com/D19414
2018-04-30 15:32:23 -07:00
epriestley
28517110c6 Fix an issue in the new Harbormaster build log view where clicking the "^" icon doesn't work right
Summary:
Ref T13130. See PHI617.

The new build log UI has tags like `<a href="...">Show More Above <span icon>^</span></a>`. If you click the little "^" icon, the event target is the `<span />` instead of the `<a />` so we expand on the wrong node.

Instead, select the `<a />` by sigil explicitly.

Test Plan: Viewed new log UI in Harbormaster, clicked "^" icon and text, got the same (correct) behavior on both.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19410
2018-04-27 11:51:59 -07:00
epriestley
1b24b486f5 Manage object mailKeys automatically in Mail instead of storing them on objects
Summary:
Ref T13065. `mailKey`s are a private secret for each object. In some mail configurations, they help us ensure that inbound mail is authentic: when we send you mail, the "Reply-To" is "T123+456+abcdef".

  - The `T123` is the object you're actually replying to.
  - The `456` is your user ID.
  - The `abcdef` is a hash of your user account with the `mailKey`.

Knowing this hash effectively proves that Phabricator has sent you mail about the object before, i.e. that you legitimately control the account you're sending from. Without this, anyone could send mail to any object "From" someone else, and have comments post under their username.

To generate this hash, we need a stable secret per object. (We can't use properties like the PHID because the secret has to be legitimately secret.)

Today, we store these in `mailKey` properties on the actual objects, and manually generate them. This results in tons and tons and tons of copies of this same ~10 lines of code.

Instead, just store them in the Mail application and generate them on demand. This change also anticipates possibly adding flags like "must encrypt" and "original subject", which are other "durable metadata about mail transmission" properties we may have use cases for eventually.

Test Plan:
  - See next change for additional testing and context.
  - Sent mail about Herald rules (next change); saw mail keys generate cleanly.
  - Destroyed a Herald rule with a mail key, saw the mail properties get nuked.
  - Grepped for `getMailKey()` and converted all callsites I could which aren't the copy/pasted boilerplate present in 50 places.
  - Used `bin/mail receive-test --to T123` to test normal mail receipt of older-style objects and make sure that wasn't broken.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13065

Differential Revision: https://secure.phabricator.com/D19399
2018-04-25 06:46:58 -07:00
epriestley
33da9f833f Fix odd line number line wrapping on embedded pastes ({Pxxx})
Summary: Ref T13126. After SourceView changes, embedded pastes with the `{Pxxx}` syntax are line-wrapping line numbers in Safari, at least. Put a stop to this.

Test Plan: Viewed a `{Pxxx}` with more than 10 lines. Before: weird line wrapping; after: nice consistent display.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13126

Differential Revision: https://secure.phabricator.com/D19393
2018-04-20 14:20:20 -07:00
Austin McKinley
4dc8e2de56 Add unique constraint to AlmanacInterfaces
Summary: See discussion in D19379. The 4-tuple of (device, network, address, port) should be unique.

Test Plan: Created lots of duplicate interfaces, bound those interfaces to various services, observed migration script clean things up correctly.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19388
2018-04-19 19:16:50 -07:00
epriestley
19403fdb8e Improve color use in "[+++- ]" element for colorblind users
Summary:
Ref T13127. Users with red/green colorblindness may have difficulty using this element in its current incarnation.

We could give it different behavior if the "Accessibility" option is set for red/green colorblind users, but try a one-size-fits-all approach since the red/green aren't wholly clear anwyay.

Test Plan: {F5530050}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13127

Differential Revision: https://secure.phabricator.com/D19385
2018-04-19 17:24:44 -07:00
epriestley
70d67a3908 Fix the most significant "phantom notification" badness
Summary:
Ref T13124. Ref T13131. Fixes T8953. See PHI512.

When you receieve a notification about an object and then someone hides that object from you (or deletes it), you get a phantom notification which is very difficult to clear.

For now, test that notifications are visible when you open the menu and clear any that are not.

This could be a little more elegant than it is, but the current behavior is very clearly broken. This unbreaks it, at least.

Test Plan:
  - As Alice, configured task stuff to notify me (instead of sending email).
  - As Bailey, added Alice as a subscriber to a task, then commented on it.
  - As Alice, loaded home and saw a notification count. Didn't click it yet.
  - As Bailey, set the task to private.
  - As Alice, clicked the notification bell menu icon.
    - Before change: no unread notifications, bell menu is semi-stuck in a phantom state which you can't clear.
    - After change: bad notifications automatically cleared.

{F5530005}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13131, T13124, T8953

Differential Revision: https://secure.phabricator.com/D19384
2018-04-19 17:24:19 -07:00
Austin McKinley
e81b2173ad Add edge tables for Phlux
Summary: Fixes T13129. This at least makes the existing UI work again before we banish Phlux to the shadow realm.

Test Plan: Edited the visibility for a Phlux variable, didn't get an error. Nothing showed up in the edge tables when I made those changes, but at least it doesn't error out anymore.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13129

Differential Revision: https://secure.phabricator.com/D19387
2018-04-19 15:49:08 -07:00
Austin McKinley
0a83f253ed Add unique constraint for Almanac network names
Summary:
The name of networks should be unique.

Also adds support for exact-name queries for AlamanacNetworks.

Test Plan: Applied migration with existing duplicates, saw networks renamed, attempted to add duplicates, got a nice error message.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19379
2018-04-19 13:41:15 -07:00
epriestley
665529ab60 Restore coverage reporting to Diffusion browse UI
Summary:
Depends on D19377. Ref T13125. Ref T13124. Ref T13105. Coverage reporting in Diffusion didn't initially survive the transition to Document Engine; restore it.

This adds some tentative/theoretical support for multiple columns of coverage, but no way to actually produce them in the UI. For now, the labels, codes, and colors are hard coded.

Test Plan:
Added coverage with `diffusion.updatecoverage`, saw coverage in the UI:

{F5525542}

Hovered over coverage, got labels and highlighting.

Double-checked labels for "N" (Not Executable) and "U" (Uncovered). See PHI577.

Faked some multi-column coverage, but you can't currently get this yourself today:

{F5525544}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13125, T13124, T13105

Differential Revision: https://secure.phabricator.com/D19378
2018-04-17 14:51:47 -07:00
epriestley
21bb0215db Remove obsoleted "diffusion-browse-file" behavior for coverage
Summary: Ref T13105. After moving Diffusion to DocumnentEngine, this no longer has callers. It will become part of the document behavior.

Test Plan: Grepped for calls to the `diffusion-browse-file` behavior, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19377
2018-04-17 14:51:12 -07:00
epriestley
37a03402bc When following a link to a particular line ("/example.txt$12"), scroll to that line
Summary:
Depends on D19349. Ref T13105. This was the behavior in Diffusion before with a little hard-coded snippet.

Remove that snippet ("diffusion-jump-to") and add a more general-purpose snippet to SourceView.

This is a tiny bit hacky still (and probably doesn't work quite right with Quicksand) but gets things working again and works in all of Files, Paste, and Diffusion.

Test Plan: Followed links to particular lines in Paste, Files and Diffusion; got scrolled to the right place.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19350
2018-04-11 17:29:22 -07:00
epriestley
5b3a351852 Use pseudoelements, not Zero Width Space, to implement copy/paste behavior in Paste/Diffusion
Summary:
Depends on D19348. Ref T13105. When copying text from Paste or Diffusion, we'd like to copy only source, not line numbers.

We currently accomplish this with zero-width spaces plus a trigger that fires on "copy" in Paste and Diffusion. This is quite gross.

In the new-style Harbormaster logs, we use an approach that seems slightly better: CSS psuedoelements.

This isn't a complete solution (see also PHI504 / T5032) but puts us in a slightly better place.

Use it in Paste/Files/Diffusion too.

This gives us good behavior in all browsers in Files and Paste.

This gives us good behavior in Chrome and Firefox in Diffusion. Safari will copy (but not visually select) blame information in Diffusion. I think we can live with that for now.

Test Plan: Selected and copy/pasted stuff in Diffusion, Files, and Paste. Got good behavior everywhere except Safari + Diffusion.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19349
2018-04-11 17:28:46 -07:00
epriestley
c5c53e277a Make line selection in source code views less fragile and more consistent
Summary:
Depends on D19347. Ref T13105. See PHI565. The "highlight lines" behavior is interacting poorly with the new blame element in Diffusion.

Make the behavior a little simpler and hopefully more robust.

Test Plan:
  - Clicked commit/revision links in Diffusion, saw the links get followed instead of the lines highlighted.
  - Highlighted lines in Diffusion, saw just the line/code highlight instead of the whole thing.
  - Highlighted lines in Paste and new-style Harbormaster build logs, saw consistent behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19348
2018-04-11 17:27:34 -07:00
epriestley
ac570fd4bc When you make the file tree huge, scroll to the right, and then toggle it, stop it from growing
Summary: Depends on D19346. Ref PHI568. I love Javascript.

Test Plan:
  - Viewed a revision.
  - Dragged file tree view really wide.
  - Scrolled document to the right.
  - Toggled file tree off and on by pressing "f" twice.
    - Before patch: file tree grew wider and wider after it was toggled.
    - After patch: file tree stayed the same size after it was toggled.
  - Dragged to various widths and reloaded to make sure the "sticky across reloads" behavior still works.
  - Scrolled right, dragged the tree a bit, then reloaded and didn't see it flip out.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19347
2018-04-11 17:25:31 -07:00
epriestley
55619e8964 Restore an explicit white background color to files in Paste
Summary:
Ref T13105. Previously, the "source code" view in Paste rendered on a brown/orange-ish background. I've been using this element in more contexts (Files, Diffusion) and removed the colored background to make text (particularly syntax-highlighted text) easier to read and reduce visual noise with the new blame colors.

In Diffusion the view is in a box with a white background so removing the background left us with white, but in Paste it's just directly on the page so the background was bleeding through. Instead, set it to white explicitly.

Test Plan: Viewed source files in Files, Diffusion and Paste; saw text on a white background.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19346
2018-04-11 17:21:33 -07:00
epriestley
d6ef32a7b7 Give the "Filetree" UI element an explicit background color
Summary:
See PHI568. If you make the file tree UI very wide so that the page generates a horizontal scrollbar and then scroll the page, the page content can paint underneath the menu.

The menu already has a z-index to make it render above the content, but doesn't actually have a background. Give it a background.

The "transparent" rule was added in D16346 but I don't see any reason why we actually need it there, so I think this probably won't break anything.

Test Plan: {F5518822}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120

Differential Revision: https://secure.phabricator.com/D19344
2018-04-11 10:42:41 -07:00
epriestley
f9c6a69d9c Add skeleton code for Almanac Interfaces to have real transactions
Summary:
Depends on D19322. Ref T13120. Ref T12414.

Currently, `AlmanacDevice` has a bit of a beast of a `TYPE_INTERFACE` transaction that fully creates a complex Interface object. This isn't very flexible or consistent, and Interfaces are complex enough to reasonably have their own object behaviors (for example, they have their own PHIDs).

The complexity of this transaction makes modularizing `AlmanacDevice` transactions tricky. To simplify this, move Interface toward having its own set of normal transactions.

This change just adds some reasonable-looking transactions; it doesn't actually hook them up in the UI or make them reachable. I'll test that they actually work as I swap the UI over.

We may also have some code using the `TYPE_INTERFACE` transaction in Phacility support stuff, so that may need to wait a week to actually phase out.

Test Plan: Ran `bin/storage upgrade` and `arc liberate`. This code isn't reachable yet.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19323
2018-04-11 10:29:26 -07:00
epriestley
4c4a5a7656 Fix the wrapping/padding behavior of Remarkup code block headers more thoroughly?
Summary: Ref T13118. The first fix there fixed Safari, but made Chrome weird. Try this?

Test Plan: Viewed a code block with `name=...` in Safari, Firefox and Chrome and saw consistent display without weird wrappping.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13118

Differential Revision: https://secure.phabricator.com/D19319
2018-04-10 04:37:14 -07:00
epriestley
472bc3d90a Colorize lines in blame under DocumentEngine, to show relative age of changes
Summary:
Depends on D19313. Ref T13105. Fixes T13015. We lost the coloration for ages in the switch to Document Engine.

Restore it, and use a wider range of colors to make the information more clear.

Test Plan: Viewed some blame, saw a nice explosion of bright colors. This is a cornerstone of good design.

Maniphest Tasks: T13105, T13015

Differential Revision: https://secure.phabricator.com/D19314
2018-04-09 06:11:47 -07:00
epriestley
cf75d63b49 When lines 12, 13, 14, etc all blame to the same change, only show it once
Summary:
Depends on D19312. Ref T13105. For readability, render only one link for each contiguous block of changes.

Also make the actual rendering logic a little more defensible.

Test Plan: Viewed some files with blame, saw one render per chunk instead of one per line.

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19313
2018-04-09 06:11:06 -07:00