Page MenuHomePhabricator

Allow books to be archived
Needs ReviewPublic

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
Subscribers

Details

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 6916
Build 6938: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

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)

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

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

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?

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 edited edge metadata.
joshuaspence marked 2 inline comments as done.

Rebase

joshuaspence edited edge metadata.

Change to isArchived

Hide archived books from /diviner/

joshuaspence marked an inline comment as done.

Use $this->newDialog()

Mark archived books as closed in the search index

  • 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.

Modernize query and search engine classes

src/applications/diviner/query/DivinerBookQuery.php
163

This doesn't feel right to me