Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4558: Make Diviner useful for third-parties

Commits

Restricted Diffusion Commit
rP6b7d7401ca6d: Modernize Diviner

Summary

Ref T4558. This diff modernizes the Diviner application. Basically:

Add an edit controller, accessible at /book/$BOOK/edit/.
Add edit/view policies.
Added an action menu to the DivinerBookController to expose the edit interface.
Allows projects to be associated with books.
Implement edges and transactions.
Implemented PhabricatorApplicationTransactionInterface in DivinerLiveBook.

Test Plan

Generated a Diviner book with ./bin/diviner generate.
Added projects to a book and ensured that they persisted.
Changed the view policy on a book and made sure it was effective.

Diff Detail

Repository

rP Phabricator

Branch

master

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6400
Build 6422: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I mean that in Differential, the implicit policy ("when a revision belongs to a repository, you must be able to see the repository in order to see the revision") ends up pretty much covering every case on its own, without much need for additional policy rules. For example, there's no arc diff --view-policy X and no one has asked us for it, at least so far. We do have a default view policy in Differential, but I'm not sure we actually need it (I think I just added them to a bunch of apps all at once a while back).

It's possible that almost every Diviner book is generated from a repository, and a similar implicit policy is sufficient in the overwhelming majority of cases.

(I think this is fine to have, regardless.)

At some point relatively soon-ish there should probably be an "Edit Book" option to let you set policies/projects from the web UI explicitly.

joshuaspence mentioned this in T4558: Make Diviner useful for third-parties.Jun 1 2015, 2:05 AM

Add a whole bunch of stuff

joshuaspence retitled this revision from Add policy support to Diviner to Modernize Diviner.Jun 1 2015, 8:25 AM

joshuaspence updated this object.

Further progress

joshuaspence mentioned this in D13070: Add repositories to Diviner.Jun 1 2015, 8:32 AM

Couple of minor inlines, things look good so far.

resources/sql/autopatches/20150601.divineredit.sql
6 ↗	(On Diff #31607)	Since `editPolicy` is not nullable, this should probably be `WHERE editPolicy = ''`.
src/applications/diviner/controller/DivinerBookEditController.php
11 ↗	(On Diff #31607)	In modern Controllers, prefer to implement `handleRequest()` instead and access URI data with `$request->getURIData('book')`. (This is just trying to clean up all the `willProcessRequest()` stubs which rarely/never do anything useful.)
13 ↗	(On Diff #31607)	Prefer `$viewer = $this->getViewer()` too.
86 ↗	(On Diff #31607)	(This seems unusual?)

Rebase
Add schema spec

Implement PhabricatorApplicationTransactionInterface

Add action list

Implement view/edit policies per book

joshuaspence updated this object.Jun 9 2015, 8:59 PM

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Set edit policy when creating a new book

Herald added a subscriber: Korvin. · View Herald TranscriptJun 11 2015, 10:06 PM

Remove useless overriding methods

Disable the "Edit Book" button for users without edit permissions

joshuaspence mentioned this in D13254: Publish books with the most-open policy.Jun 12 2015, 7:37 AM

joshuaspence added a child revision: D13256: Allow books to be archived.Jun 12 2015, 7:51 AM

Implement handleRequest()

joshuaspence added inline comments.Jun 15 2015, 11:22 AM

src/applications/diviner/controller/DivinerBookEditController.php
87 ↗	(On Diff #32042)	How so?

Index DivinerLiveBook transactions for the search index

Minor changes

A couple of things that I couldn't immediately work out that are maybe useful here:

Is there any need to implement DivinerBookPHIDType::loadNamedObjects()? I couldn't work out how this was used.
I wasn't sure if there was any reason to implement DivinerBookPHIDType::getPHIDTypeApplicationClass.
I wasn't sure of the difference between URI data and request data in AphrontRequest. For example, getURIData vs getStr.

Minor change

Is there any need to implement DivinerBookPHIDType::loadNamedObjects()? I couldn't work out how this was used.

You don't need to implement this. It's used when you, e.g., type bin/remove destroy OBJECT to figure out what "OBJECT" means, but books don't have a monogram/object name.

I wasn't sure if there was any reason to implement DivinerBookPHIDType::getPHIDTypeApplicationClass.

One thing it does is gets a better icon for objects of that type in feed and typeaheads, if the type doesn't specify an icon explicitly (we can fall back to the application icon).

I wasn't sure of the difference between URI data and request data in AphrontRequest. For example, getURIData vs getStr.

getURIData() reads out of the route. getStr() reads out of the request parameters. For example, if you have this route:

/diviner/(?P<action>\w+)/

And load this page:

/diviner/eat/?action=drink

You'll get these values:

$request->getURIData('action'); // eat
$request->getStr('action');     // drink

Add some translations
Fix atom controller

joshuaspence added inline comments.Jun 15 2015, 9:26 PM

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
1182–1185 ↗	(On Diff #32182)	Not sure if this is possible, but the if second parameter is singular then this should read "... but only %s is documented".

joshuaspence added inline comments.Jun 15 2015, 9:30 PM

src/applications/diviner/workflow/DivinerGenerateWorkflow.php
66 ↗	(On Diff #32182)	Interestingly, with one book this is rendering as "Found 1 books."

Minor tidying
Call didRejectResult() in DivinerAtomQuery

Minor tidying

joshuaspence added a child revision: D13303: Allow atoms to be queried by book.Jun 16 2015, 10:20 AM

Minor fix for migration

joshuaspence updated this object.Jun 16 2015, 12:09 PM

joshuaspence updated this object.

Remove derp

joshuaspence added inline comments.Jun 16 2015, 12:29 PM

src/applications/diviner/controller/DivinerBookController.php
124 ↗	(On Diff #32190)	I feel like this gets repeated alot... maybe this should be a method on `DivinerLiveBook`?

Minor tidying

Remove form divider

joshuaspence added a child revision: D13070: Add repositories to Diviner.Jun 16 2015, 12:57 PM

Handful of minor inlines.

src/applications/diviner/application/PhabricatorDivinerApplication.php
57–60	Add `'template'` with the PHID type for "default X capability" after T5681 / D13251. This will provide a hint about object policies which are suitable for selection in this control (this is mostly groundwork for future changes, it will have no real effect today). Something like: 'template' => DivinerLiveBookPHIDType::TYPECONST,
src/applications/diviner/controller/DivinerAtomController.php
324 ↗	(On Diff #32193)	Per discussion elsewhere, consider not making this or other alignment changes.
src/applications/diviner/controller/DivinerAtomListController.php
10 ↗	(On Diff #32193)	I wrote this weird; it should have no default (so it defaults to `null`), which will make the SearchEngine choose the user's first search query. As written, I'd expect this to not work correctly if you reorder saved search queries.
src/applications/diviner/controller/DivinerBookEditController.php
19–20 ↗	(On Diff #32193)	Should have a `if !$book return 404`.
21 ↗	(On Diff #32193)	Should have a trailing slash.
23 ↗	(On Diff #32193)	Oh, the 404 is here -- but needs to be above `$book->getName()`, or we'll fatal before we get here.
88 ↗	(On Diff #32193)	The "unusual" thing was the random divider control at the bottom of the form.
src/applications/diviner/publisher/DivinerLivePublisher.php
16–19 ↗	(On Diff #32193)	Given that we define default policies, I'd expect this to be something like `DivinerLiveBook::initializeNewBook()` and set the defaults, now.
src/applications/diviner/search/DivinerBookSearchIndexer.php
21–24 ↗	(On Diff #32193)	(I think this is automatic if you implement ApplicationTransactionInterface?)
src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
1128 ↗	(On Diff #32193)	Omit one level of `array()` to fix `1 books`.
1134 ↗	(On Diff #32193)	Omit 1 level of array(), etc, etc.
1182–1185 ↗	(On Diff #32193)	Broadly, each level of `array()` depth corresponds to a parameter. So if you have no parameters, you have no arrays: 'apple' => 'le appelo', If you have one parameter, you should have maximum depth of one array: '%s apple(s)' => array( 'an apple', 'apples', ) (Not two arrays, as some of the cases above do.) If you have two parameters, you'll have a depth of two arrays, and the inner arrays will handle the second parameter: '%s apple(s) and %s banana(s)' => array( array(...), // Cases with 1 apple array(...), // Cases with some other number of apples ); So I'd expect these to produce the correct behavior -- one conversion, maximum depth is one array: 'You have %s apple(s).' => array( 'You have an apple.', 'You have %s apples.', ); Two conversions, maximum depth is two arrays: 'You have %s apple(s) and %s banana(s).' => array( array( 'You have an apple and a banana.', 'You have an apples and %2$s bananas.', ), array( 'You have %s apples and a banana.', 'You have %s apples and %s bananas.', ), );

This revision is now accepted and ready to land.Jun 16 2015, 2:34 PM

joshuaspence added inline comments.Jun 16 2015, 2:35 PM

src/applications/diviner/controller/DivinerAtomController.php
324 ↗	(On Diff #32193)	Yeah sorry... got a bit carried away.

Add templates for custom capabilities
Remove a bunch of alignment changes
Remove default query from atom list controller
Throw a 404 if no such book is found
Fix $view_uri
Fix translation strings

joshuaspence added inline comments.Jun 16 2015, 9:15 PM

src/applications/diviner/controller/DivinerBookEditController.php
88 ↗	(On Diff #32193)	Ah right... the inline comment doesn't line up anymore. Yeah, the divider has been removed now.
src/applications/diviner/search/DivinerBookSearchIndexer.php
21–24 ↗	(On Diff #32193)	I think that it should be, but it doesn't seem to be.

Closed by commit rP6b7d7401ca6d: Modernize Diviner (authored by joshuaspence, committed by joshuaspence). · Explain WhyJun 16 2015, 9:18 PM

This revision was automatically updated to reflect the committed changes.

Modernize Diviner
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 31590

src/__phutil_library_map__.php

src/applications/diviner/application/PhabricatorDivinerApplication.php

src/applications/diviner/capability/DivinerDefaultViewCapability.php

Modernize DivinerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 31590

src/__phutil_library_map__.php

src/applications/diviner/application/PhabricatorDivinerApplication.php

src/applications/diviner/capability/DivinerDefaultViewCapability.php

Modernize Diviner
ClosedPublic
Actions

Revision Contents
Changeset List