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

autodetect author ASIN, Youtube, storygraph ids #8203

Merged
merged 5 commits into from
Sep 6, 2023

Conversation

davidscotson
Copy link
Collaborator

Closes #8202

Autodects ASIN, Youtube and storygraph identifiers on the Author edit page.

Technical

Testing

Pasting in (or typing) one of the above ids should automatically set the dropdown to the appropriate id type. Wikidata and ISNI types should continue to be detected.

One note for testing, if the contents doesn't change, the code isn't triggered, so if you paste in a storygraph id, manually change the type of id in the selector to something else, then paste in the exact same storygraph id then it'll not do anything. Not an issue for real users but something that testers might do and wonder if it is a bug.

Screenshot

Stakeholders

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

Merging #8203 (e4163f8) into master (7e98b57) will increase coverage by 0.69%.
Report is 94 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8203      +/-   ##
==========================================
+ Coverage   16.27%   16.97%   +0.69%     
==========================================
  Files          69       72       +3     
  Lines        3816     3789      -27     
  Branches      668      654      -14     
==========================================
+ Hits          621      643      +22     
+ Misses       2765     2728      -37     
+ Partials      430      418      -12     
Files Changed Coverage Δ
openlibrary/plugins/openlibrary/js/index.js 0.00% <0.00%> (ø)
...enlibrary/js/merge-request-table/LibrarianQueue.js 0.00% <0.00%> (ø)
.../merge-request-table/LibrarianQueue/TableHeader.js 0.00% <0.00%> (ø)
.../js/merge-request-table/LibrarianQueue/TableRow.js 0.00% <0.00%> (ø)
...lugins/openlibrary/js/merge-request-table/index.js 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

@cdrini cdrini added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Aug 28, 2023
@cdrini cdrini assigned jimchamp and unassigned cdrini Aug 28, 2023
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, @davidscotson! With the exception of the Youtube identifier pattern, everything is working as expected.

The conditionals that determine the currently selected identifier type are becoming unwieldy, and will become worse as new identifiers are added in the future. In order to simplify things a bit, could you make the following changes?

  1. Create an object that maps detectable selectedIdentifier values to their patterns. This can be declared on the line following the opening <script> tag.
  2. Replace the conditionals with a for...in loop that iterates over the object's keys. While iterating, if this.inputValue is a match, store the current key as the selected identifier and break out of the for loop.

While you work on this, I'll open a separate PR which updates our pg_dump file to include the author configs for Youtube, Storygraph, and Amazon identifiers.

const wikiDatas = this.inputValue.match(/^Q[1-9]\d*$/i);
const isnis = this.inputValue.match(/^\d{15}[\dX]$/i);
const storygraphs = this.inputValue.match(/^[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i);
const asins = this.inputValue.match(/^B[0-9A-Za-z]{9}$/);
const youtubes = this.inputValue.match(/^UC[-_0-9A-Za-z]{21}[AQgw]$/);
Copy link
Collaborator

@jimchamp jimchamp Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using handle URLs for our Youtube links, not the ID-based channel URLs, so this pattern will need to be updated. A Youtube identifier begins with an @ symbol, followed by the Youtuber's handle.

Handle naming guidelines can be found here.
Additional information about Youtube's many URL types can be found here.

@davidscotson
Copy link
Collaborator Author

I'll make those changes.

A couple of things I've noticed while using the feature which I might look at too if they don't become tricky:

  1. Pasting in an INSI with spaces doesn't trigger the detector.
  2. The type selector doesn't reset so its kind of easy to accidentally set a viaf over the top of whatever you just added.

@jimchamp
Copy link
Collaborator

Thanks! To your points:

  1. I noticed this while testing, but didn't mention it. I think that this pattern should be changed to include optional whitespaces, but I'll let you decide if that's in the scope of this PR.
  2. Do you mean after an identifier has been set by clicking the "set" button? It's probably a good idea to reset the selector when this happens.

@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Sep 1, 2023
@davidscotson
Copy link
Collaborator Author

Pushed a rebased branch that makes the requested change to for .. in seperately, then adds new items, including Youtube handles, also fixes ISNI with spaces.

@jimchamp jimchamp added Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Sep 5, 2023
@jimchamp jimchamp changed the title autodetect author ASIN, Youtube, storygraph ids fixes #8202 autodetect author ASIN, Youtube, storygraph ids Sep 6, 2023
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @davidscotson! This looks good to me.

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that the object name was in snake_case. Changing it to identifierPatterns and merging.

openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved
openlibrary/components/AuthorIdentifiers.vue Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-detect ASINs & StoryGraph identifiers in the author page editor
4 participants