Page MenuHomePhabricator
Differential D13256

Allow books to be archived
Needs ReviewPublic
Actions

Authored by joshuaspence on Jun 11 2015, 10:54 PM.
Tags
None
Referenced Files
F14076568: D13256.id32347.diff
Thu, Nov 21, 6:18 PM
F14076430: D13256.id32372.diff
Thu, Nov 21, 5:20 PM
Unknown Object (File)
Tue, Nov 19, 12:26 PM
Unknown Object (File)
Tue, Nov 12, 8:02 AM
Unknown Object (File)
Mon, Nov 11, 2:50 AM
Unknown Object (File)
Mon, Nov 11, 1:42 AM
Unknown Object (File)
Mon, Nov 11, 12:02 AM
Unknown Object (File)
Sat, Nov 9, 1:13 AM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T4558: Make Diviner useful for third-parties
Summary

Ref T4558. Depends on D13091.

Test Plan

This doesn't quite work yet.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 6905
Build 6927: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 32043.Jun 11 2015, 10:54 PM
joshuaspence retitled this revision from to Allow books to be archived.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a task: T4558: Make Diviner useful for third-parties.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 11 2015, 10:54 PM
Herald added a subscriber: epriestley. · View Herald Transcript
joshuaspence updated this object.Jun 12 2015, 7:51 AM
joshuaspence edited edge metadata.
joshuaspence added a parent revision: D13091: Modernize Diviner.
joshuaspence updated this revision to Diff 32051.Jun 12 2015, 8:17 AM

Seems to work now

joshuaspence added a comment.Jun 12 2015, 11:02 AM

I'm not sure if this is actually valuable, I was just mucking around with various pieces of infrastructure.

epriestley requested changes to this revision.Jun 13 2015, 4:00 PM
epriestley added a reviewer: epriestley.

Generally looks good to me. Probably needs a Query integration (withIsArchived(null|true|false) or withStatuses($statuses)?) and a SearchEngine integration (there's some new infrastructure here, check Paste/Macro/People if you want to try modernizing).

resources/sql/autopatches/20150612.diviner.status.sql
2 ↗(On Diff #32051)

Use {$COLLATE_TEXT} instead of literal utf8mb4_bin.

4–6 ↗(On Diff #32051)

I think IS NULL will match nothing since the column is NOT NULL -- you can use = "" instead.

src/applications/diviner/constants/DivinerBookStatus.php
5–6 ↗(On Diff #32051)
  • If you don't anticipate more statuses, consider using a BOOL field like isArchived.
  • If you do anticipate more statuses, consider using string values like "active" and "archived". Particularly, this likely makes the API easier to use down the road.
  • Unless you anticipate a whole lot of different statuses, consider just putting this stuff on DivinerLiveBook directly.
src/applications/diviner/controller/DivinerBookArchiveController.php
58–65

Can be slightly simplified with return $this->newDialog()->...

This revision now requires changes to proceed.Jun 13 2015, 4:00 PM
joshuaspence added a comment.Jun 13 2015, 11:32 PM

There is also the question of what it means for a book to be archived...

  • Is an archived book still rendered for display?
  • Can an archived book be updated with diviner generate?
  • Are links to archived book (or symbols from archived books) rendered differently?
epriestley added a comment.Jun 14 2015, 2:01 PM

Offhand, I'd say:

  • Hide archived books from /diviner/ by default.
  • Oh, although I guess there's no way to query for books, per se, right now. Not clear if there should be eventually (maybe?). Maybe just put the archived books at the bottom in grey for now?
  • Ideally, hide symbols in archived books from /diviner/query/ by default (but provide a way to find them). Not sure how much of a mess this is.
  • When looking at something in an archived book (like /book/phabricator/), show that somehow, to communicate to the user that whatever they're looking at might be obsolete.
joshuaspence updated this revision to Diff 32346.Jun 20 2015, 8:47 AM
joshuaspence edited edge metadata.
joshuaspence marked 2 inline comments as done.

Rebase

Herald added a subscriber: Korvin. · View Herald TranscriptJun 20 2015, 8:47 AM
joshuaspence updated this revision to Diff 32347.Jun 20 2015, 9:07 AM
joshuaspence edited edge metadata.

Change to isArchived

joshuaspence updated this revision to Diff 32367.Jun 21 2015, 12:01 AM

Minor fixes

joshuaspence updated this revision to Diff 32370.Jun 21 2015, 12:13 AM

Hide archived books from /diviner/

joshuaspence updated this revision to Diff 32371.Jun 21 2015, 12:15 AM
joshuaspence marked an inline comment as done.

Use $this->newDialog()

joshuaspence updated this revision to Diff 32372.Jun 21 2015, 12:18 AM

Mark archived books as closed in the search index

joshuaspence added a comment.Jun 21 2015, 1:11 AM
In D13256#140997, @epriestley wrote:
  • Oh, although I guess there's no way to query for books, per se, right now. Not clear if there should be eventually (maybe?). Maybe just put the archived books at the bottom in grey for now?

You can query for books in global search. I was considering allowing DivinerAtomSearchEngine to return books as well as atoms, but this seems messy and I soon decided that it was a bad idea.

joshuaspence updated this revision to Diff 32375.Jun 21 2015, 2:31 AM

Modernize query and search engine classes

joshuaspence added inline comments.Jun 21 2015, 11:56 AM
src/applications/diviner/query/DivinerBookQuery.php
163 ↗(On Diff #32375)

This doesn't feel right to me

joshuaspence updated this revision to Diff 32382.Jun 21 2015, 11:56 AM

Minor fix

Revision Contents
Changeset List

PathSize
resources/
sql/
autopatches/
2 lines
src/
2 lines
applications/
diviner/
application/
1 line
controller/
67 lines
24 lines
editor/
54 lines
storage/
2 lines
84 lines

Diff 32367

View Options

resources/sql/autopatches/20150620.diviner.status.sql

Loading...
View Options

src/__phutil_library_map__.php

Loading...
View Options

src/applications/diviner/application/PhabricatorDivinerApplication.php

Loading...
View Options

src/applications/diviner/controller/DivinerBookArchiveController.php

Loading...
View Options

src/applications/diviner/controller/DivinerBookController.php

Loading...
View Options

src/applications/diviner/editor/DivinerLiveBookEditor.php

Loading...
View Options

src/applications/diviner/storage/DivinerLiveBook.php

Loading...
View Options

src/applications/diviner/storage/DivinerLiveBookTransaction.php

Loading...
Phabricator · Built by Phacility