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

Libkey integration - requested changes #147

Merged

Conversation

Chloe070196
Copy link

@Chloe070196 Chloe070196 commented Dec 3, 2024

Changes requested after code review, most of them performance-related, namely:


  • check if a url contains a DOI

  • check for libKey setting
    Only ever use the LibKey integration if a LibKey
    setting is associated with the active library.
    This aims to ensure that the LibKey integration
    code only runs if LibKey has been set up for the
    relevant library.

  • do not proceed if no setting found

    Removes an unecessary fetch() call as find(true)
    already executes a fetch(), and changes the
    control flow so that getLibKeyLink() returns
    null early if no setting is found (only move on
    to creating an instance of CurlWrapper if the
    setting was found).

  • remove duplicated lines

    These were likely involuntarily duplicated during
    a merge conflict resolution. Remove the redundant
    lines.

  • remove unecessary module

    It was established that there is no need for the
    LibKey integration to be a module. This removes
    the SQL that would have added it to the modules
    table in the Aspen database.


To test:

  • check functionality for eContent with DOI urls (test plans at 3956f87 and 4a1d87a )
  • check functionality for eContent with urls that do not contain DOIs - must be preserved after applying the patch

It was established that there is no need for the
LibKey integration to be a module. This removes
the SQL that would have added it to the modules
table in the Aspen database.
These were likely involuntarily duplicated during
a merge conflict resolution. Remove the redundant
lines.
Removes an unecessary fetch() call as find(true)
already executes a fetch(), and changes the
control flow so that getLibKeyLink() returns
null early if no setting is found (only move on
to creating an instance of CurlWrapper if the
setting was found).
Only ever use the LibKey integration if a LibKey
setting is associated with the active library.

This aims to ensure that the LibKey integration
code only runs if LibKey has been set up for the
relevant library.
@Chloe070196
Copy link
Author

Two additional changes may still be required - awaiting feedback on whether they will need to be implemented or not

@Chloe070196 Chloe070196 changed the title Libkey integration Libkey integration - requested changes Dec 3, 2024
@AlexanderBlanchardAC
Copy link

AlexanderBlanchardAC commented Dec 4, 2024

Retested after changes made. The issues found during testing, error message for items with no doi and missing libkey link in the links section have both been resolved and all appears to be working as expected. The next commit addresses the url returned when it is not libkey (to ensure it defaults to the 856 u instead of returning an empty string).

If no LibKey link is found, use the related url
instead.
@Chloe070196 Chloe070196 merged commit 62e9642 into PTFS-Europe:libkey_integration Dec 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants