Tpac - missing added content reviews

[Ben Shum]

Evergreen version: master

While exploring the problem of "missing" Syndetics reviews in our Tpac, we noticed that the block of code for additional added contents doesn't seem to do anything yet. See lines 11-15 on http://git.evergreen-ils.org/?p=Evergreen.git;a=blob;f=Open-ILS/src/templates/opac/parts/record/awards.tt2;h=94c72a24d67f2a703e30a668c0cbf140ca872724;hb=refs/heads/master

So I suppose this is a future bug ticket describing the lack of added content integration in Tpac's record display.

Specifically Syndetics for us, though I'm sure there are other added content providers that could be added in as well?

[Michael Peters (mrpeters)]

This would be a major blocker for Evergreen Indiana. We spend thousands on Syndetics content. We couldn't even begin to consider upgrading to 2.2 without that content being available, due to a regression in features.

[Dan Scott (denials)]

There is an (outdated) list of gaps between TPAC and JSPAC at http://evergreen-ils.org/dokuwiki/doku.php?id=dev:opac:template-toolkit:plan where "some added content components" is flagged.

There's a proposed Hackfest topic to update and add to this to help plan for the transition to TPAC at http://evergreen-ils.org/dokuwiki/doku.php?id=dev:hackfest:eg2012

[Bill Erickson (berick)]

The plan discussed with Jeff G. and Mike R. at the hackfest looks something like this:

Create a new horizontal tab, similar to the Shelf Browser, MARC Record, etc. tabs, dedicated specifically to remote added content. If any content for the current record is available, make the tab visible. Once expanded, the tab will contain the content from one type (undecided) of added content and simple tabs to hide the current content and show the alternate selected type of content. There will be one tab per available class of content: toc, anotes, excerpt, summary, and reviews.

Remote added content will require Javascript. It will use dojo for XHR, both to determine availability of content and to fetch the content when selected. There will likely be some other small UI bits handled with dojo (no, not dijit).

[Bill Erickson (berick)]

A few points of clarification... by "remote" added content, I mean added content provided and proxied by Evergreen's added content modules. This includes content from Syndetics, ContentCafe, and OpenLibrary. (Note, not all providers offer every type of content and some providers offer content not proxied by Evergreen). The point of requiring JS is that retrieving content has to be done (a) asynchronously to avoid page rendering delays and (b) only when the content is available. We could solve problem (a) w/o JS using iframes, but problem (b) persists. Consensus thus far has been that requiring JS is an acceptable compromise, at least in the short term.

[Thomas Berezansky (tsbere)]

I would like to disagree with the requirement of JavaScript to view added content. In particular, if added content information is already cached I don't see why it can't be used more directly.

I also dislike the idea of JavaScript inserting content into the pages after the fact, which seems implied by the proposed solution. I feel that the actual content of pages served by TPac should, as much as possible, be dictated by the actual templates.

If added content information is not cached I don't have as much of an issue with JavaScript being used to look up and load it after the fact (beyond previous notes about disliking JavaScript dumping more content into the page after the fact), but once it is cached I don't think the JavaScript needs to be triggered at all. In addition, I think that if at all possible the request to look up the information should be fired off server side so that those without JavaScript enabled may be able to see added content. One possible solution there is showing the header bars if we had no information about added content on the initial page load (so that those without JavaScript may look anyway) and using JavaScript to *hide* the ones that have no content once the lookup is complete? If someone requested one of those bars then we would, of course, need to wait for the appropriate added content to finish being loaded in order to show them the content or a "Sorry, there is no information here" message.

[Bill Erickson (berick)]

proposal:

1. In the mod_perl, at the start of load_record(), we fire off one async (via Net::HTTP::NB or similar) HTTP HEAD request per added content type.

2. At the end of load_record(), we collect header information for all completed HTTP requests. We do not wait any additional time for responses.

3. A request that returns HTTP 200 is marked as available in the template context and any that returns HTTP 404 is marked as unavailable. All other (non-complete) requests are marked as status unknown.

4. In the template, available content results in visible links to retrieve the content. Unavailable content results in invisible links.

5. Status-unknown content results in visible links when JS is disabled. When JS is enabled (testing for want_dojo), unknown content results in invisible links, then a small chunk of JS tests for the content via XHR and un-hides links for any content proven to exist.

6. A user clicking on a link will result in the page reloading with the selected content visible (similar to the existing tabs). Content will once again be fetched via net::http::nb, but this time via a full (not just HEAD) request and passed to the context for insertion into the page. Any content that cannot be retrieved will result in a "no content available" message within the tab, which should only happen for users that do not use JS, where it's not always possible to quickly guarantee the content exists or not.

[Thomas Berezansky (tsbere)]

I recommend that the various HTTP calls in the proposal be wrapped in functions that would be easily swapped out for non-HTTP variants if/when backend calls can accomplish the same things. Otherwise I think that is largely ok with me, with the exception of the wording of #4...

"invisible" links for things we know aren't there would, to me, be "we don't even transmit the link block" but here seem to be more of a "apply a hide_me css class". Using the "we don't include the link block" with conditional TPac code the JavaScript for the "unknowns" could then be a simple loop over the potential links looking for a custom tag, class, or similar. "Known not to be there" won't even show up in the DOM, and thus the JavaScript will have no reason to go looking (and no special checks need to happen for that). Written generically enough it would hopefully require no additional code for future things pulled from Added Content providers, even.

[Bill Erickson (berick)]

I agree with both of these points.

[Bill Erickson (berick)]

Initial cut pushed to collab/berick/tpac-added-content. It's still a little rough and has some TODO items in the commit.

[Bill Erickson (berick)]

Pushed fixes for tab styling, hiding the main tab when no content is available, and selecting a default tab when content is known to be available.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/berick/tpac-added-content

[Bill Erickson (berick)]

There remains an edge case that leads to opening the main tab with no default sub-tab selected. This happens when no content is known to be available until after the JS is run. We'll need to add code to the JS to set the ac= variable on the main added content tab link to select a default when this happens. I was having trouble creating the scenario, though, to test the code. If others can make it happen, I have an idea on how to fix it.

[Bill Erickson (berick)]

Edge case mentioned above is now repaired. Additional fixes, cleanup, and master merge pushed to collab/berick/tpac-added-content @ working.

[Ben Shum (bshum)]

Tried applying all the changes in collab/berick/tpac-added-content

Seemed to work for a few records (we saw Syndetics content under the new tab), but then we encountered major problems on several other records.

On the browser side of things, all we got were internal server errors. The server error logged read something like:

2012-05-23 12:32:39 theory gateway: [error] [client 10.129.3.149] egweb: Context Loader error: Bad method or uri at /usr/local/share/perl/5.10.1/OpenILS/WWW/EGCatLoader/Record.pm line 444\n, referer: http://theory.biblio.org/eg/opac/results?fi%3Aitem_type=&query=harry+potter&qtype=keyword&locg=1

[Ben Shum (bshum)]

Ack, that line number 444 might be a little off on that test system since it's also got some changes to Record.pm for KPac testing. I'll try to get the exact line as it would appear if we were testing properly on a clean master...

[Bill Erickson (berick)]

Fix pushed to the same branch (collab/berick/tpac-added-content @ working). Thanks, Ben, for spotting and testing my patch.

[Mike Rylander (mrylander)]

Aaaaaaaaaand ... merged to master. Awesome work, all.