Cover backlink to vendor #1805

xmorave2 · 2020-12-08T08:20:42Z

No description provided.

demiankatz · 2020-12-08T15:45:26Z

Thanks, @xmorave2! I'm not in the office this week, but I'll review this as soon as time permits.

demiankatz

A few questions and suggestions based on a quick review....

module/VuFind/src/VuFind/Cover/Router.php

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml

themes/bootstrap3/js/covers.js

xmorave2 · 2020-12-16T14:07:14Z

Thanks for a review @demiankatz, this should be ready for next round.

demiankatz · 2020-12-17T18:26:58Z

Thanks, @xmorave2; this looks good to me, but I'd like to try some hands-on testing before merging it. I should have time for that early next year (time is very short at the moment, unfortunately).

demiankatz · 2021-01-13T18:54:47Z

@xmorave2, apologies for the delays -- I have a lot of post-holiday catch-up to do -- but I finally had time to try this out on my test server. Unfortunately, I cannot get it to work at all.

The first thing I did was merge this with the latest dev branch and resolve conflicts. It's possible something was broken in that process (though the only conflicts had to do with the compiled CSS files). Is this still working for you on your end?

In any case, to be more specific about my problems: I indexed the sample ObalkyKnih records you shared with me, and I confirmed that I get cover images for those records on the current dev branch, both with or without "ajaxcovers = true" in my config.ini. When I switch to this coverBacklink branch, however, turning on "ajaxcovers = true" results in lots of loading spinners; the covers never actually appear. If I set "ajaxcovers = false," I do at least see cover images, but there is no backlink included.

I apologize if I'm just failing to set up my test environment correctly, but it seems there may be some larger problems here. Please let me know if this is enough to help you troubleshoot, or if you need me to provide more details or spend more time debugging on this end.

Also note that I have slated this for the 8.0 milestone. Since the translation work for release 7.1 is already complete, and this adds new strings, it will have to wait for 8.0. Fortunately, since 7.1 should be released next month, 8.0 development will begin very soon... and assuming we can work the bugs out of this, I can merge it to the dev-8.0 branch as soon as it is working, if an early merge would be helpful.

xmorave2 · 2021-03-16T13:27:57Z

@demiankatz: I apologize for delay, but now this is ready for another round I hope.

demiankatz · 2021-03-17T11:52:42Z

Thanks, @xmorave2, the bugs now seem to be fixed. However, I notice a couple inconsistencies:

1.) Cover backlinks show on the record view, but not in search results. (I'm guessing this might be intentional).

2.) Cover backlinks only show when AJAX cover loading is enabled. (This is, of course, a technical limitation).

So this leads to some questions:

1.) Is there a bug regarding inconsistent display locations for the link? If the inconsistent display is intentional, should it be configurable?

2.) I understand why we need AJAX cover loading to display these links... but if displaying the links is a contractual requirement, this could be a problem. Should we add a mechanism that allows cover loaders to determine whether or not they are being used in an AJAX context and refuse to serve content (perhaps with a warning message to the log) if configured incorrectly?

In any case, thanks for the progress on this -- it looks good and addresses a long-standing need!

xmorave2 · 2021-03-17T15:24:47Z

Thanks for review @demiankatz

ad 1) Yes, it is intentional, inserting link into a link (the cover on results are links to records) does not work well... but I might try to move the source information out of link... (in fact, the similar manipulation is now done in covers.js, so it should not much work, I'll try it)

ad 2) I do try to make some mechanism you are recommending, that does make sense, to ensure the legal requirements are met when using covers from external sources...

…s are off

xmorave2 · 2021-03-17T16:15:51Z

@demiankatz I have tried to address both issues and this is so ready for another round

lgtm-com · 2021-03-17T16:16:23Z

This pull request introduces 1 alert when merging e33799a into 1267712 - view on LGTM.com

new alerts:

1 for DOM text reinterpreted as HTML

demiankatz · 2021-03-17T16:20:50Z

Thanks, @xmorave2! I'll try to review this again later in the week. In the meantime, did you see the lgtm-com alert? Any thoughts on that?

lgtm-com · 2021-03-17T16:38:02Z

This pull request introduces 1 alert when merging 743ca28 into 1267712 - view on LGTM.com

new alerts:

1 for DOM text reinterpreted as HTML

xmorave2 · 2021-03-17T16:42:11Z

@demiankatz I just tried something like quick fix, which does not work ;) But i know how to fix it, it just need a bit of time, I will do it tomorrow

xmorave2 · 2021-03-18T12:12:33Z

@demiankatz LGTM issue is fixed

demiankatz · 2021-03-19T18:25:12Z

Thanks for the progress on this! I'm requesting a review from @crhallberg in case he has any comments on the LESS/JS changes, and I will do a bit more review and testing of the remaining parts later today as time permits!

demiankatz

Thanks again -- things are working consistently for me now, though I still have a few questions...

module/VuFind/src/VuFind/Content/Covers/ObalkyKnih.php

module/VuFind/src/VuFind/Content/AbstractCover.php

demiankatz · 2021-03-19T20:11:52Z

themes/bootstrap3/js/covers.js

+          var span = $('<span class="cover-source-text"' + response.data.backlink_text + '</span>');
+          source.append(span);
+        }
+        if (inlink === true && response.data.backlink_on_results === true) {


This seems a bit fragile -- in that the identification of whether the backlink is in results or not seems to be based on a variety of circumstances that might not be entirely specific. I wonder if we should just display backlinks everywhere and rely on CSS customization to hide them in unwanted places. Would that be simpler?

Yes it is not the same (inlink vs search result). The reason I use this type of distinction is that, if a link is inserted into another link in html it simply does not work well and I need to make same dom manipulation to get the inner link out of outer link....

simple solution would be to rename the option to something like 'other the detail page'

or change the non-detail page logic somehow.... but then we may need much more options - channels, related tab, and possibly some more in future...

We currently have cover size settings that can be configured based on template names: see these configurations. Perhaps we could use a similar mechanism for allowing the user to configure where backlinks appear. Maybe we could replace the mandatoryBacklinks boolean property with a mandatoryBacklinkLocations array, and then locations in this array could forcibly override user preferences for those locations (and when this is non-empty, that tells us we have to force AJAX covers).

I think that meets the use case and is flexible... but is it too complicated to implement? Should we go ahead with that approach, or should we do something simpler for now and refine as needed?

To be honest, this seems a bit overcomplicated for our current need, although I agree it would be much more flexible, and I could imagine such a implementation in the future, if it is OK for you, I would like this to be merged as is and probably continue with this on another PR in the future.

I agree that we don't need to fully implement this to get the PR merged, but I wonder if moving a little bit in that direction would make the code simpler and more readable. For example, what if we replace the boolean isBacklinkMandatory() and showBacklinksOnResults() methods with a single mandatoryBacklinkLocations() method that returns an array? Then current code that checks for the boolean can instead check for a non-empty array. For now, we can code this so that backlinks are only displayed when mandatory, which keeps other logic simple... but this makes it easier to add an additional configuration layer if we decide we need it in the future, and it also keeps the cover drivers more independent of the UI behavior -- they tell us details about the provider (where a backlink needs to be displayed, which is constant for each provider), but reconciling that with user preferences can be delegated to a higher layer of code. Does that make sense?

That does make sense, I've done a bit of refactoring here.

Thanks, this is definitely a step in the right direction! Would it be worth taking it a little bit farther and exposing the whole backlink_locations array to the JS instead of mapping it to the backlink_on_results boolean? Again, just trying to make this a little more generic if possible. I'm still not entirely comfortable with assuming things are in results just because of the presence of links around cover images, since I think there are some possible exceptions and edge cases there... but I'm willing to accept this as-is with a "TODO: make location detection more accurate/specific" comment if that's as far as you want to go right now!

That makes sense @demiankatz. Now the whole backlink locations array is passed. I also tried to reuse the context for backlink display

…Find-2.x into coverBacklink

crhallberg · 2021-03-23T15:00:13Z

themes/bootstrap3/js/covers.js

    if (typeof response.data.url !== 'undefined' && response.data.url !== false) {
      img.attr("src", response.data.url);
-      container.children().not("img").hide();
+      var inlink = element.parent().is('a');


Can we add a class to the expected link? This seems easy to trip with customizations. I can come back to this in the future if it proves to be especially tricky.

@crhallberg How do you mean this? Do you think we should add a class into html template and use it then in covers.js for identification to be more specific? Or do you think add a class by this javascript code to be able to select manipulated elemetnt using CSS?

@crhallberg can correct me if I'm wrong, but I suspect he's suggesting the former: adding a class into the HTML template to reduce the odds of this code hooking the wrong element because of a fairly broad selector.

Thanks, I added class to templates

Thanks! @crhallberg, are you happy with the record-link class, or would you prefer cover-link or something else? (I'm happy to make the change if necessary -- not trying to drive @xmorave2 crazy with nitpicky changes :-) ).

I would prefer cover-link or record-cover-link but I don't think it's worth holding this up.

Thanks, @crhallberg! Do you have a preference, @xmorave2? I hope to do final testing and merging of this PR next week, so I can adjust the class name at that time if you have a preference.

I choose record-cover-link. I pushed a commit with this change

demiankatz · 2021-04-07T16:51:09Z

I also resolved the merge conflicts to get our green checkmark back on the build. :-)

xmorave2 · 2021-04-07T17:36:58Z

I also resolved the merge conflicts to get our green checkmark back on the build. :-)

Thanks @demiankatz

demiankatz

Thanks for taking the time to work on this -- I think this is much, much more clear, and also a good foundation for greater flexibility in the future. I just have a few very minor comments. Once you have a chance to address them (and @crhallberg has had a chance to weigh in again), I think I should be able to merge this.

module/VuFind/src/VuFind/Content/AbstractCover.php

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml

themes/bootstrap3/templates/RecordTab/similaritemscarousel.phtml

…Find-2.x into coverBacklink

crhallberg · 2021-04-13T21:05:49Z

This all looks good to me!

demiankatz

Thanks, @xmorave2 -- before I do my final test (and hopefully merge), I spotted one small potential issue that seems worth addressing before it potentially comes up again during static analysis. :-)

demiankatz · 2021-04-15T11:24:13Z

module/VuFind/src/VuFind/Cover/Router.php

+        $metadata = $this->getMetadata(
+            $driver, $size, $resolveDynamic, $testLoadImage
+        );
+        return $metadata['url'] ?? $metadata;


Should this be $metadata['url'] ?? false? If $metadata['url'] is unset, then $metadata is most likely an empty array, which is not one of the documented return types of this method.

It is a bit more complicated - getMetadata from AbstractCover can return empty array, which is then passed from Router::getMetadata to Router::getUrl.

But Router::getMetadata can even return false or null values... we probably want these values to be passed and returned by getUrl I think...

I'll take a look on it and will see...

I apologize if I just skipped a step in the code during my review -- that's very possible. If nothing else, it's a sign that we should add a comment here to clarify what is going on, so we don't ask ourselves this question again in the future.

I tried to make it more clear and also added a case when empty array is returned from getMetadata, what do you think about it @demiankatz

Thanks, @xmorave2. The comment helps to clarify what is going on, but I'm not sure if the implementation matches the comment. If $metadata is null or false, then empty($metadata) will evaluate to true, and the code will normalize the return value to false. If you want to preserve the null return and be sure you never return an array, would it be better to change the !empty() check to !is_array() instead?

You are absolutely right @demiankatz, thanks for catching this. I've fixed it now

Great, thank you! I believe we're all set now -- I've just merged this.

xmorave2 added 2 commits December 8, 2020 09:18

Add ability to create backlinks to cover vendors

b1c71bb

Compile CSS

ef240fc

demiankatz reviewed Dec 14, 2020

View reviewed changes

module/VuFind/src/VuFind/Cover/Router.php Outdated Show resolved Hide resolved

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml Outdated Show resolved Hide resolved

themes/bootstrap3/js/covers.js Outdated Show resolved Hide resolved

xmorave2 added 4 commits December 16, 2020 13:24

Simplify getUrl

421420d

Add ability to show cover source as text

3664827

Show cover source only on record detail page

1c2c5bb

Remove source element if not needed

97d9594

Merge remote-tracking branch 'origin/dev' into coverBacklink

0c19bcf

demiankatz added this to the 8.0 milestone Jan 13, 2021

xmorave2 added 3 commits March 12, 2021 09:33

Merge remote-tracking branch 'origin/dev' into coverBacklink

29d00e6

Don't show cover source in link element

37fca44

Merge remote-tracking branch 'origin/dev' into coverBacklink

a4d8336

Enable cover source backlinks on search results

ab53845

xmorave2 added 2 commits March 17, 2021 17:00

Do not load cover if cover source backlink is mandatory and ajaxcover…

5ac951f

…s are off

Add ability to not show cover backlinks on search results

e33799a

Use htmlString, not string as parameter to replaceWith

743ca28

Create backlink in js instead of manipulating dom

b18879d

Fix typo.

5eb702a

demiankatz requested a review from crhallberg March 19, 2021 18:24

demiankatz reviewed Mar 19, 2021

View reviewed changes

xmorave2 and others added 3 commits March 22, 2021 08:56

Add note to config file

24bca6e

Merge branch 'coverBacklink' of https://github.com/moravianlibrary/Vu…

a817c6d

…Find-2.x into coverBacklink

Update config.ini

3f66149

crhallberg approved these changes Mar 23, 2021

View reviewed changes

xmorave2 and others added 3 commits April 7, 2021 15:45

Add class to record links with cover image

7f7512e

Refactor backlink locations determination

ebb7b8f

Merge branch 'dev' into coverBacklink

cfb0e22

Pass backlink locations to client

76aba86

Add more flexibility for backlink settings

8c3b2bd

demiankatz requested changes Apr 7, 2021

View reviewed changes

module/VuFind/src/VuFind/Content/AbstractCover.php Outdated Show resolved Hide resolved

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml Outdated Show resolved Hide resolved

themes/bootstrap3/templates/RecordTab/similaritemscarousel.phtml Show resolved Hide resolved

xmorave2 and others added 5 commits April 8, 2021 07:46

Escape html attribute

9283f9a

Fix doc comment

dd79011

Merge branch 'dev' into coverBacklink

d57f10f

Change cover link class

073b6f3

Merge branch 'coverBacklink' of https://github.com/moravianlibrary/Vu…

72f341e

…Find-2.x into coverBacklink

demiankatz requested changes Apr 15, 2021

View reviewed changes

xmorave2 added 2 commits April 16, 2021 10:19

Clarify return of getUrl

4d6e2ee

Check for array instead of empty

8134f55

demiankatz merged commit 2384443 into vufind-org:dev Apr 16, 2021

Cover backlink to vendor #1805

Cover backlink to vendor #1805

Conversation

xmorave2 commented Dec 8, 2020

demiankatz commented Dec 8, 2020

demiankatz left a comment

Choose a reason for hiding this comment

xmorave2 commented Dec 16, 2020

demiankatz commented Dec 17, 2020

demiankatz commented Jan 13, 2021

xmorave2 commented Mar 16, 2021

demiankatz commented Mar 17, 2021

xmorave2 commented Mar 17, 2021

xmorave2 commented Mar 17, 2021

lgtm-com bot commented Mar 17, 2021

demiankatz commented Mar 17, 2021

lgtm-com bot commented Mar 17, 2021

xmorave2 commented Mar 17, 2021

xmorave2 commented Mar 18, 2021

demiankatz commented Mar 19, 2021

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmorave2 Apr 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crhallberg Apr 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz commented Apr 7, 2021

xmorave2 commented Apr 7, 2021

demiankatz left a comment

Choose a reason for hiding this comment

crhallberg commented Apr 13, 2021

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmorave2 Apr 6, 2021 •

edited

crhallberg Apr 9, 2021 •

edited