Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Syndetics tables of contents and summaries. #456

Merged
merged 19 commits into from Oct 27, 2017

Conversation

Projects
None yet
3 participants
@johnjung
Copy link
Contributor

commented Sep 1, 2015

Please take a look at this and let me know what you think- One thing that you might not want involves tables of contents- I have our system set up so that if Syndetics content for TOCs isn't there, it falls back to getting that info from the record driver.

Thanks again for looking, and please let me know if there are things I can change around!

@demiankatz

This comment has been minimized.

Copy link
Member

commented Sep 2, 2015

Thanks for sharing this. A few comments:

1.) There are some code style issues here. If you look at the Travis report, it will offer all the details. Some of these things can be fixed automatically -- see https://vufind.org/wiki/vufind2:recommended_tools for details.

2.) In the toc.phtml template, rather than hard-coding Syndetics, I think it would make sense to do a foreach loop on the data coming back from $this->tab->getContent(); that way, if other TOC provider plug-ins are written, this will continue to work with no adjustments needed.

3.) Do summaries really need their own tab, or would it make sense to incorporate them into the existing Description tab the way author notes have been inserted? (I don't mind having another tab if you feel it's a requirement, but if we don't actually need it, we can shrink the code a bit by consolidating the display).

4.) If we do go ahead with the separate Summaries tab, it looks like that may introduce some new language strings, in which case we should add them to en.ini so that we don't forget to request translations on the next round of language updates.

5.) Do you have any recommended records for testing these features? I haven't actually tried the code in action yet, but when the time comes, it would be helpful to know where I can see the new features working.

Thanks again!

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

Thanks very much, Demian! I'm following up on a few of these- I'll get back
to you soon.

From: Demian Katz notifications@github.com
Reply-To: vufind-org/vufind
<reply+009b9cd329674032eb68d84534728e2490f8f0f959ff4a7092cf0000000111febaec9
2a169ce06385972@reply.github.com>
Date: Wednesday, September 2, 2015 at 8:26 AM
To: vufind-org/vufind vufind@noreply.github.com
Cc: John Jung john@johnjung.us
Subject: Re: [vufind] Syndetics tables of contents and summaries. (#456)

Thanks for sharing this. A few comments:

1.) There are some code style issues here. If you look at the Travis report,
it will offer all the details. Some of these things can be fixed
automatically -- see https://vufind.org/wiki/vufind2:recommended_tools for
details.

2.) In the toc.phtml template, rather than hard-coding Syndetics, I think it
would make sense to do a foreach loop on the data coming back from
$this->tab->getContent(); that way, if other TOC provider plug-ins are
written, this will continue to work with no adjustments needed.

3.) Do summaries really need their own tab, or would it make sense to
incorporate them into the existing Description tab the way author notes have
been inserted? (I don't mind having another tab if you feel it's a
requirement, but if we don't actually need it, we can shrink the code a bit
by consolidating the display).

4.) If we do go ahead with the separate Summaries tab, it looks like that
may introduce some new language strings, in which case we should add them to
en.ini so that we don't forget to request translations on the next round of
language updates.

5.) Do you have any recommended records for testing these features? I
haven't actually tried the code in action yet, but when the time comes, it
would be helpful to know where I can see the new features working.

Thanks again!


Reply to this email directly or view it on GitHub
#456 (comment) .

@demiankatz

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@johnjung, it's been a couple of months, so just checking in to see if you've had time to get back to this. If you need help from me, please let me know.

@crhallberg crhallberg added the devcall label Aug 29, 2017

@crhallberg crhallberg removed this from Waiting for Contributor in Development Roadmap Oct 10, 2017

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2017

Sorry this one got away from me, Demian. Please take a look at the new changes I just added.

There is one thing I don't understand here: right now, in RecordTab\AbstractContent's getContent() method, I'm setting $this->results two times to get it to pull a table of contents from Syndetics. Can you tell what's going on there? I see that Content\TOC\Syndetics's loadByIsbn() method is only running the second time, but I don't know why that is.

In the meantime I'll look at why the Travis build failed.

John Jung added some commits Oct 19, 2017

John Jung
John Jung
John Jung
@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

More debugging notes: I see that my hack has VuFind\Content\TOC\Factory:getAbstractSyndetics() running twice, but it is only able to instantiate VuFind\Content\TOC\Syndetics the second time. The first time it produces the following error log message: 'An exception was raised while creating "Syndetics"; no instance returned.'

Still digging :)

John Jung added some commits Oct 20, 2017

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

Ah...I played with the code a bit and got the following error message:

Declaration of VuFind\Content\AbstractSyndetics::getHttpClient($url) should be compatible with VuFind\Content\AbstractBase::getHttpClient($url = NULL)

Once I added that default value I could instantiate Syndetics the first time.

@demiankatz

This comment has been minimized.

Copy link
Member

commented Oct 20, 2017

Thanks, @johnjung! Are you still working through the earlier suggestions? Please let me know when you're ready for another review, or if you need help with anything else in the meantime.

John Jung
@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

I'm ready for another review when you are. I removed the references to syndetics in toc.phtml, and I moved summaries to the description tab. I have a copy of this running on a development machine with debugging turned on- here are some urls you can look at:

"Color: the secret influence" has a table of contents from Syndetics:
http://moss.lib.uchicago.edu:8080/vufind/Record/4050074

"La Strada" has a description from Syndetics:
http://moss.lib.uchicago.edu:8080/vufind/Record/4582044

Please let me know what you think. Thanks!

@demiankatz
Copy link
Member

left a comment

A few small questions and comments based on another review. Once you've responded to these one way or another, I'll see if I can actually get this running using my own Syndetics test account for further testing.

Also, one other side question: does this work with both Syndetics and SyndeticsPlus? Have you tested both scenarios?

@@ -739,6 +739,11 @@ public function getLCCN()
// Get LCCN from Index
$raw = isset($this->fields['lccn']) ? $this->fields['lccn'] : '';
// First call number only.

This comment has been minimized.

Copy link
@demiankatz

demiankatz Oct 20, 2017

Member

Was this added by accident? It doesn't seem related to Syndetics, and I don't think it should be necessary in core VuFind (but maybe it's related to one of your local customizations).

This comment has been minimized.

Copy link
@johnjung

johnjung Oct 20, 2017

Author Contributor

This might be unique to us. I removed it and I'll push up a copy of this file without the array checking in my next push.

@@ -56,6 +56,6 @@ public function getDescription()
public function isActive()
{
$toc = $this->getRecordDriver()->tryMethod('getTOC');
return !empty($toc);
return parent::isActive() || !empty($toc);

This comment has been minimized.

Copy link
@demiankatz

demiankatz Oct 20, 2017

Member

I'd recommend reversing the order of these clauses -- parent::isActive could be an expensive operation, so if !empty($toc) evaluates to true, we want to be able to short-circuit out and avoid it.

This comment has been minimized.

Copy link
@johnjung

johnjung Oct 20, 2017

Author Contributor

Sounds good!

<?
$summaries = $this->driver->getSummary();
foreach ($summaries as $summary):
?><?=$summary ?><br /><?

This comment has been minimized.

Copy link
@demiankatz

demiankatz Oct 20, 2017

Member

I think we need $this->escapeHtml() around $summary here; VuFind generally assumes everything needs to be escaped.

$summaries = $this->summaries($isbn);
foreach ($summaries as $provider => $list) {
foreach ($list as $summary) {
?><?=$summary ?><br /><?

This comment has been minimized.

Copy link
@demiankatz

demiankatz Oct 20, 2017

Member

Do we also need to escape this summary, or is the API giving us HTML?

This comment has been minimized.

Copy link
@johnjung

johnjung Oct 20, 2017

Author Contributor

Added. I'll push these changes up in a minute.

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

The travis tests are running on those changes now. I haven't checked this with Syndetics Plus, but I can do that now.

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

I added SyndeticsPlus for tables of contents, and I tried to add it for summaries, but as far as I can tell we don't have access to that data. Can you see if that one works?

Merge branch 'master' into syndeticspull
# Conflicts:
#	themes/bootstrap3/theme.config.php
@demiankatz

This comment has been minimized.

Copy link
Member

commented Oct 26, 2017

@johnjung, I tried to resolve the conflicts in this PR for you, but it looks like I don't have write access to your repository, so I can't push anything up to it. Do you mind giving me access? If that's not possible, I can open a PR against your PR to keep things moving. :-)

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2017

Lets save recursive PRs for something bigger, if we can! I added you as a collaborator- please let me know if you have trouble pushing to that repository.

@demiankatz

This comment has been minimized.

Copy link
Member

commented Oct 26, 2017

Thanks, that did it! Hopefully I'll find time to actually do some testing later today.

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2017

That's excellent, Demian, thanks very much. Please let me know if there are other things I can fix or help with.

demiankatz added some commits Oct 26, 2017

@demiankatz

This comment has been minimized.

Copy link
Member

commented Oct 26, 2017

@johnjung, I've made a few template adjustments and fixed the SyndeticsPlus "summaries" feature (which had an incorrect div id). I think this is in pretty good shape to merge!

There is one significant problem: the SyndeticsPlus functionality doesn't work correctly in combination with our AJAX-loaded record tabs. However, this problem also affects the existing AuthorNotes functionality, so I don't think we need to fix that before merging this -- it's a separate known bug, which I'll open a JIRA ticket about and see if I can address.

Anyway, please take a look at the latest version of the code and let me know if it still works for you. If so, I'll merge it, write up some documentation, and create that bug tracker ticket.

Thanks again for all of your help with this!

@johnjung

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2017

@demiankatz I've been looking at the changes as the come in over the last few minutes. The changes look great to me, thank you!

@demiankatz

This comment has been minimized.

Copy link
Member

commented Oct 26, 2017

@johnjung, thanks for the review! I'm heading home for the day now but will take care of the finishing touches tomorrow.

@demiankatz demiankatz merged commit eaf5024 into vufind-org:master Oct 27, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new issues
Details
@demiankatz

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

@johnjung, as promised, the PR has been merged. Here is the ticket about the Syndetics Plus issues: https://vufind.org/jira/browse/VUFIND-1255

Do you need Syndetics Plus for your instance, or are you using regular Syndetics? I think we have to discourage use of Plus until we can solve this problem. Perhaps as an actual subscriber to the service, you would be in a better position to contact them and ask if there is a solution for this problem -- but I don't know if it's worth the time to do it.

@gibsonjc gibsonjc referenced this pull request Oct 31, 2018

Closed

TOC from Syndetics #114

cedelis added a commit to CARLI/vufind that referenced this pull request Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.