1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-04 04:32:43 +01:00
Commit graph

445 commits

Author SHA1 Message Date
epriestley
54bcbdaba9 Fix an XSS issue with certain high-priority remarkup rules embedded inside lower-priority link rules
Summary:
See <https://hackerone.com/reports/758002>. The link rules don't test that their parameters are flat text before using them in unsafe contexts.

Since almost all rules are lower-priority than these link rules, this behavior isn't obvious. However, two rules have broadly higher priority (monospaced text, and one variation of link rules has higher priority than the other), and the latter can be used to perform an XSS attack with input in the general form `()[ [[ ... | ... ]] ]` so that the inner link rule is evaluated first, then the outer link rule uses non-flat text in an unsafe way.

Test Plan:
Tested examples in HackerOne report. A simple example of broken (but not unsafe) behavior is:

```
[[ `x` | `y` ]]
```

Differential Revision: https://secure.phabricator.com/D20937
2019-12-13 10:37:50 -08:00
epriestley
de66a8ece1 Remove "stronger/weaker" policy color hints from object headers
Summary:
Fixes T13461. Some applications provide hints about policy strength in the header, but these hints are inconsistent and somewhat confusing. They don't make much sense for modern objects with Custom Forms, which don't have a single "default" policy.

Remove this feature since it seems to be confusing things more than illuminating them.

Test Plan:
  - Viewed various objects, no longer saw colored policy hints.
  - Grepped for all removed symbols.

Maniphest Tasks: T13461

Differential Revision: https://secure.phabricator.com/D20918
2019-11-18 22:05:26 -08:00
epriestley
a0ade503e1 Remove "Moved Document from ..." notice in Phriction
Summary:
Ref T13410. See PHI1431. Currently, when you move a document in Phriction, the target shows a "This document was moved from ..." banner until it is edited.

This banner isn't particularly useful, and it's distracting and it isn't obvious how to dismiss it, and making a trivial edit to dismiss it is awkward.

This information is also already available in the transaction log.

Just remove this banner since it doesn't really serve any clear purpose.

Test Plan:
  - Moved a page in Phriction, then loaded the destination page. Before change: header banner. After change: nothing.
  - Viewed a normal (non-moved) page, saw normal behavior.
  - Reviewed transactions, saw "Moved from ..." in the timeline.

Maniphest Tasks: T13410

Differential Revision: https://secure.phabricator.com/D20800
2019-09-12 09:32:26 -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
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
e5fe4dffe1 Add a "Published document changed" rule to Herald for Phriction documents
Summary: Depends on D20519. Ref T13283. See PHI1202. Add a new rule which triggers when the current/most-recent transaction group includes a "content" or "publish" transaction, which means the published document content has changed.

Test Plan:
  - Wrote a Herald rule using this field.
  - Created a document (rule matched).
  - Edited a document (rule matched).
  - Edited a document, saving as a draft (no match).
  - Edited a draft, updating it (no match).
  - Published a draft docuemnt (rule matched).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

Differential Revision: https://secure.phabricator.com/D20520
2019-05-16 10:40:52 -07:00
epriestley
89d038f53e Make Portals indexable with Ferret
Summary:
Ref T13275. Add portals to the search index so that:

  - they show up in fulltext global search; and
  - the typeahead actually uses an index.

Also make them taggable with projects as an organizational aid.

Test Plan: Indexed portals with `bin/serach index`, searched for a portal with "Query", with fulltext search in main menu, with typehead on "Install Dashboard...", changed the name of a portal and searched again to check that the index updates properly.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13275

Differential Revision: https://secure.phabricator.com/D20389
2019-04-10 13:33:54 -07:00
epriestley
aae7bdbbdd Reference raw "title" row in Phriction paging, not "c.title"
Trivial mistake -- the query "SELECT x.y ..." selects a column named "y".
The other calls to "getRawRowProperty()" get this right.
2019-03-23 07:27:42 -07:00
epriestley
c16a8c7ab2 Fix a content join condition in Phriction
After the cursor changes, we may fatal on pages with a large number of children
because "c.title" is not a selected column. We currently join the "content"
table if "updated" is part of the order vector, but not if "title" is part of
the order vector. This isn't right: "updated" is on the primary table, and only
"content" is on the joined table.
2019-03-23 07:18:01 -07:00
epriestley
8449c1793a Convert complex query subclasses to use internal cursors
Summary:
Depends on D20292. Ref T13259. This converts the rest of the `getPagingValueMap()` callsites to operate on internal cursors instead.

These are pretty one-off for the most part, so I'll annotate them inline.

Test Plan:
  - Grouped tasks by project, sorted by title, paged through them, saw consistent outcomes.
  - Queried edges with "edge.search", paged through them using the "after" cursor.
  - Poked around the other stuff without catching any brokenness.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20293
2019-03-19 13:02:16 -07:00
epriestley
b09cf166a8 Clean up a couple more URI alter() calls
Summary:
See <https://discourse.phabricator-community.org/t/create-new-phriction-document-fails-with-unhandled-exception-invalidargumentexception/2406>.

These weren't obviously nullable from a cursory `grep`, but are sometimes nullable in practice.

Test Plan: Created, then saved a new Phriction document.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20184
2019-02-15 14:07:17 -08:00
epriestley
c125ab7a42 Remove "metamta.*.subject-prefix" options
Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.

These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.

Newer applications stopped providing these options and no one has complained.

You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.

If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.

Test Plan: Grepped for `subject-prefix`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19993
2019-01-17 19:18:50 -08:00
epriestley
e98ee73602 Fix the last remaining (?) continue inside switch
Summary: See <https://discourse.phabricator-community.org/t/error-on-project-creation-or-edition-with-php7-3/2236/>. This is the only remaining case that the linter rule in D19931 detected in libphutil, arcanist, or Phabricator.

Test Plan: Ran `arc lint --everything ...` in all three repositories, only hit this one.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19932
2018-12-28 00:04:25 -08:00
epriestley
11cf8f05b1 Remove "getApplicationTransactionObject()" from ApplicationTransactionInterface
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).

The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.

The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.

So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.

Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.

Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19920
2018-12-20 15:16:19 -08:00
epriestley
937e88c399 Remove obsolete, no-op implementations of "willRenderTimeline()"
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.

All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.

Test Plan: Used `grep willRenderTimeline` to find callsites, found none.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19919
2018-12-20 15:04:49 -08:00
Austin McKinley
da4341cf8b Make it less confusing to create root-level Phriction doc
Summary: Without an existing root document, Phriction shows a nice little "fake" document as the landing page, which has its own nice "Edit this document" button. When showing that page, don't also render the standard "New Document" breadcrumb in the top right. That button always prompts first for a slug name, which is silly when the root document doesn't exist (because the slug name is required to be '').

Test Plan: Loaded Phriction with and without a root document.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19863
2018-12-10 14:10:18 -08:00
Austin McKinley
00a7071e2d Fix handling of Phriction conduit edits
Summary: See https://discourse.phabricator-community.org/t/conduit-method-phriction-edit-requires-title-while-the-docs-say-its-optional/2176. Make code consistent with documentation by not requiring either `content` or `title`.

Test Plan: Hit the method via the UI and no longer got an error on missing `content` or `title` fields.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19862
2018-12-10 13:38:13 -08:00
epriestley
933462b487 Continue cleaning up queries in the wake of changes to "%Q"
Summary: Depends on D19810. Ref T13217. Ref T13216. I mostly used `grep implode | grep OR` and `grep implode | grep AND` to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing `%LO` / `%LA`.

Test Plan:
These are tricky to test exhaustively, but I made an attempt to hit most of them:

- Browsed Almanac interfaces.
- Created/browsed Calendar events.
- Enabled/disabled/showed the lock log.
- Browsed repositories.
- Loaded Facts UI.
- Poked at Multimeter.
- Used typeahead for users and projects.
- Browsed Phriction.
- Ran various fulltext searches.

Not sure these are reachable:

- All the lint stuff might be dead/unreachable/nonfunctional?

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13217, T13216

Differential Revision: https://secure.phabricator.com/D19814
2018-11-16 12:49:44 -08:00
epriestley
5ba66e56fd Fix Phriction rendering for initial install and 404 pages
Summary:
Depends on D19682. Ref T13202. We currently fatal when trying to render a timeline if:

  - an install is fresh, so there are no pages yet, and you look at "/w/"; or
  - you're looking at a Phriction page which doesn't exist (yet) like "/w/aadsflknadsflnf/".

Rendering a timeline and comment area doesn't make sense in these cases, so don't render them.

Test Plan: Hit both cases described above, got "new/empty page" prompts instead of fatals.

Reviewers: amckinley

Maniphest Tasks: T13202

Differential Revision: https://secure.phabricator.com/D19683
2018-09-17 20:02:59 -07:00
epriestley
e5c6a5749a Fix fatal in rendering Phriction "Moved Away" stories
Summary:
Ref T13202. See PHI881. These stories have bad rendering methods, but they didn't previously render into the timeilne (since Phriction documents didn't have a timeline).

Update the rendering to work.

The rendered outcome isn't great (it isn't very clear or explicit about exactly what moved where), but I'll fix that in a followup. This is a net improvement since it doesn't fatal the page, at least.

Test Plan:
  - Moved page "X" to "Y".
  - Viewed the old page "X".
  - Before patch: bad timeline story would fatal rendering.
  - After patch: story renders, at least, just not great.

Reviewers: amckinley

Maniphest Tasks: T13202

Differential Revision: https://secure.phabricator.com/D19682
2018-09-17 20:02:06 -07:00
epriestley
0abf29765e Make Phriction diff-of-changes view draft-aware and clarify some language
Summary:
Ref T13077. Ref T13197. See PHI840.

  - In the "History > Diff/Compare" view, the button language wasn't draft-aware.
  - Revise language to avoid the word "Revert", since this can be ambiguous.
    - "Edit this page, starting with an older version of the text" is now "Edit Older|Current|Draft Version X".
    - "Mark this older version of the page as the current published version" is now "Publish Older Version".
  - Let the user edit the current published version, too, since this is a reasonable operation if there are drafts.

Test Plan: Navigated the history diff view, saw better button and action text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197, T13077

Differential Revision: https://secure.phabricator.com/D19668
2018-09-13 14:12:49 -07:00
epriestley
0089ef4b60 Don't show Phriction draft edit events in feed
Summary: Depends on D19663. Ref T13077. When you edit a Phriction draft, don't publish a feed story. (The eventual "Publish" event gets a story.)

Test Plan: Made draft / non-draft / publish edits, only saw feed stories for non-draft and publish edits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19664
2018-09-12 13:56:10 -07:00
epriestley
b6ba75d991 Add a Phriction hint when a draft exists, and fix some draft editing bugs
Summary:
Depends on D19662. Ref T13077. See PHI840.

  - If you're looking at the published version of a document, but a draft version exists and you can edit it, add a hint/link.
  - Fix an issue where the "draft" transaction would complain when you created a document since the initial content is empty and no "draft" transaction is adding any content.

Test Plan: Created new documents, viewed documents with current published versions and unpublished drafts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19663
2018-09-12 13:45:33 -07:00
epriestley
550028a882 Allow Phriction document edits to be saved as drafts
Summary:
Depends on D19661. Ref T13077. See PHI840.

When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.

Then there are a few minor rules:

  - If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
  - Highlight "Publish" if it's a likely action that you might want to take.

Internally, there are two types of edits. Both types create a new version with the new content. However:

  - A "content" edit sets the version shown on the live page to the newly-created version.
  - A "draft" edit does not update the version shown on the live page.

Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19662
2018-09-12 13:30:40 -07:00
epriestley
152e7713eb Remove obsolete, nonfunctional draft auto-saving in Phriction
Summary:
Depends on D19660. Ref T5811. Ref T13077.

Long ago, if you started editing a Phriction document but didn't save it, we'd save the draft in the background as part of the preview.

D11169 updated the preview to use shared infrastructure and broke this function, since we never save drafts.

Since this doesn't work right now, I want to add another thing called "draft", and the future of this feature should be more integrated with modern drafts and EditEngine (which fixed some bugs related to versioning), just get rid of this code for the moment.

Test Plan: Edited documents. This code doesn't do anything since D11169, so no behavior changed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077, T5811

Differential Revision: https://secure.phabricator.com/D19661
2018-09-12 13:22:16 -07:00
epriestley
e19c555913 Support (basic) commenting on Phriction documents
Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.

  - Add an EditEngine, although it currently supports no fields.
  - Add (basic, top-level-only) commenting (we already had the table in the database).

This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.

This also moves us incrementally toward putting all editing on top of EditEngine.

Test Plan: {F5877347}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077, T1894

Differential Revision: https://secure.phabricator.com/D19660
2018-09-12 13:20:52 -07:00
epriestley
2379c21fbb Give Phriction documents a normal timeline
Summary:
Ref T13077. See PHI840. Ref T1894. I'm planning to just let you comment on Phriction documents. I think this will create a few problems (e.g., around popular documents which collect long comment threads that are eventually obsolete) but nothing should be too terribly critical (e.g., we handle it gracefully when objects have very large number of comments/transactions) and for most documents this is likely just a net improvement.

"Just enable comments" is probably not the final iteration on this, but I think it's probably a step forward on the balance, not a step sideways or a slippery slope down into a dark hole or anything.

Test Plan: {F5877316}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077, T1894

Differential Revision: https://secure.phabricator.com/D19659
2018-09-11 13:31:57 -07:00
epriestley
26d9ee456f Hide the new Phriction "Publish" operation behind the "Prototype" toggle
Summary: Ref T13077. This is currently a little too confusing to go out into the world, mostly because there's no way to edit documents without auto-publishing them. Keep it out of the spotlight for this release.

Test Plan: Viewed Phriction, saw publish operation marked as a prototype.

Reviewers: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19627
2018-08-31 10:30:20 -07:00
epriestley
18e8d9a452 Make the Phriction "History" view more aware of drafts
Summary: Ref T13077. Updates the "History" view to be slightly better organized and draft-aware.

Test Plan: Viewed page history in Phriction.

Reviewers: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19626
2018-08-30 10:36:55 -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
349686319e Allow the published version of a Phriction document to differ from the most recent version
Summary:
Depends on D19620. Ref T13077. This adds a "Publish" operation which points the current version at some historical version of the document -- not necessarily the most recent version. Newer versions become "drafts".

This is still quite rough and missing a lot of hinting in the UI, I'm just making it work so I can start making the UI understand it.

Test Plan: Used the "Publish" action to publish older versions of a document, saw the document revert. Many UI hints are missing and this operation is puzzling and not yet usable for normal users.

Reviewers: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19621
2018-08-29 13:47:36 -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
04f8270a74 Remove some unusual UI policy hints in Phriction
Summary:
Ref T13077. We currently have these weird policy hints in Phriction that we don't use in other applications. Just remove them for consistency to make the eventual swap to EditEngine a little easier.

Also nuke some unreacahble code.

Test Plan: Loaded edit page, saw more standard UI.

Reviewers: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19618
2018-08-29 07:32:45 -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
fd0da4c41f Rename "PHUIDocumentViewPro" to "PHUIDocumentView"
Summary: Ref T13077. There is no "PHUIDocumentView" so toss the "Pro" suffix from this classname.

Test Plan: Grepped for `PHUIDocumentView` and `PHUIDocumentViewPro`.

Reviewers: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19616
2018-08-28 14:53:07 -07:00
epriestley
3b92da22f4 Move the hierarchical edit policy check in Phriction from requireCapabilities() to validateTransactions()
Summary:
Depends on D19583. Ref T13164. This continues the work of getting rid of `requireCapabilities()`.

This check is valid, but can be a `validateTransactions()` check instead. This is generally more consistent with how other applications work (e.g., creating subprojects).

The UI for this isn't terribly great: you get a policy error //after// you try to create the object. But that's how it worked before, so this isn't any worse than it was. The actual policy exception is (very) slightly more clear now (raised against the right object).

Test Plan:
  - Created a child as a user with permission to do so to make sure I didn't break that.
  - Set edit permission on `a/` to just me, tried to create `a/b/` as another user, got a policy exception since they can't edit the parent.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19584
2018-08-16 10:55:11 -07:00
epriestley
f3fa164882 Add a "Last Edited" property to Wiki pages
Summary:
Ref T13164. See PHI797. The last edit is available in the page header, but it's not precise (just says "180 days ago") and a little weird (it's unusual for us to put that kind of information in the header).

Add a precise timestamp to the footer for now. I'd imagine re-examining this the next time Phriction gets some UI work and maybe trying to integrate timeline/transactions more cleanly (see also T1894).

Test Plan: Looked at a wiki page, then edited it. Saw precise "Last Edit" timestamp adjacent to "Last Author".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19560
2018-08-03 14:29:17 -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
Austin McKinley
9a0dd55442 Extend PhabricatorPolicyCodex interface to handle "interesting" policy defaults
Summary:
Fixes T13128. Ref PHI590. This is a rough-and-ready implementation of a new `PhabricatorPolicyCodex->compareToDefaultPolicy()` method that subclasses can override to handle special cases of policy defaults. Also implements a `PolicyCodex` for Phriction documents, because the default policy of a Phriction document is the policy of the root document.

I might break this change into two parts, one of which maintains the current behavior and another which implements `PhrictionDocumentPolicyCodex`.

Test Plan: Created some Phriction docs, fiddled with policies, observed expected colors in the header. Will test more comprehensively after review for basic reasonable-ness.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, swisspol

Maniphest Tasks: T13128

Differential Revision: https://secure.phabricator.com/D19409
2018-04-27 16:56:11 -07:00
epriestley
9bb338c038 Revert the alternate menu names for applications
Summary: This reverts D18524. See that revision for discussion.

Test Plan: Viewed home menu, saw application names as menu items.

Differential Revision: https://secure.phabricator.com/D19308
2018-04-08 10:20:24 -07:00
epriestley
e57dbcda33 Hide "abraham landed Dxyz irresponsibly" stories from feed
Summary:
Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance.

These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story.

Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time.

Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly".

Maniphest Tasks: T13099, T12787

Differential Revision: https://secure.phabricator.com/D19179
2018-03-06 17:48:03 -08:00
epriestley
ada4f65565 Stop sending Phriction edit mail to the previous/current content author
Summary:
See <https://discourse.phabricator-community.org/t/previous-author-of-phriction-page-always-receives-mail-when-new-author-edits-the-page/1155>.

After the "send users email when they are removed from the reviewer list" change which includes old To/Cc on the list, Phriction now emails the previous editor.

This is weird and unintended. The behavior isn't necessarily always bad, but if it's sometimes desirable we should make it explicit.

For now, just get rid of the weird side effect.

Test Plan: Edited a Phriction page.

Differential Revision: https://secure.phabricator.com/D19147
2018-02-27 15:04:59 -08:00
epriestley
4cb62ca0d6 Support "phriction.document.search" queries by "parentPaths" or "ancestorPaths"
Summary: Ref T13090. Ref T13077. This adds `parentPaths` and `ancestorPaths` constraints to `phriction.document.query`. These should be a little more usable than the internal `slugPrefix` / `depth` stuff -- that's technically more powerful, but requires callers to know more slug normalization rules. We could perhaps expose `minDepth` / `maxDepth` in the future.

Test Plan: Ran valid and invalid `parentPaths` and `ancestorPaths` queries for `/`, `aaa/`, `AAA/`, etc. Got sensible-seeming results.

Maniphest Tasks: T13090, T13077

Differential Revision: https://secure.phabricator.com/D19125
2018-02-22 12:49:13 -08:00
epriestley
cb2f710606 Provide the document content as a context object when rendering Phriction documents
Summary: Ref T13077. The context object wasn't being passed into the engine properly here, affecting relative link rendering in Phriction.

Test Plan: Viewed rendered Phriction documents with relative links, got clean renders.

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19115
2018-02-16 12:42:41 -08:00
epriestley
66a7ca49b2 Fix incorrect context extraction for relative Phriction links on Phriction pages
Summary: Ref T13077. This content extraction rule wasn't right and caused rendering on Phriction pages to extract context improperly.

Test Plan: Viewed pages in Phriction with relative links to other documents.

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19114
2018-02-16 12:33:40 -08:00
epriestley
db3ef4021a Freeze the "phriction.info" Conduit API method
Summary: Ref T13077. Freeze "phriction.info" in favor of the more modern "phriction.document.search".

Test Plan: Reviewed older method in web UI, saw frozen markers.

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19113
2018-02-16 12:14:23 -08:00
epriestley
6d3177a3bf Allow "phriction.document.search" to query by path
Summary: Ref T13077. Adds a "paths" constraint to the API query.

Test Plan: Used paths constraint to fetch documents.

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19112
2018-02-16 11:40:43 -08:00