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

Use IdentifiersInput Vue component for work identifier UI #10032

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

schu96
Copy link
Contributor

@schu96 schu96 commented Nov 14, 2024

Partially addresses #3430

Feature

Technical

DRY implementation of work identifiers but currently unable to save the changes made by users into Solr.

The work identifiers currently displayed in this component are based off of this list: https://openlibrary.org/config/work

Testing

  1. Navigate to a work/book and click on the edit button
  2. Switch from the edition edit tab to the Work Details edit tab
    image
  3. Scroll to Work Identifiers section and add/remove work identifiers.
    • Duplicates are currently not allowed for work identifiers. Adding a duplicate will warn the user that an identifier already exists.
    • Work Identifiers should be saved as a list, allowing more than one ID per identifier. Duplicate ID numbers across different identifiers is not allowed i.e BookBrainz ID:123456 vs Amazon ID:123456.

image

  1. After adding new work identifiers, save the changes and return to the editing page -> Work Details tab to check that the newly added work identifiers are still present.

Stakeholders

@cdrini

@schu96 schu96 force-pushed the 3430/feature/add-work-ids-ui branch from 05db8f5 to 807342f Compare December 2, 2024 22:55
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.44%. Comparing base (347bff9) to head (c1d245b).
Report is 151 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10032      +/-   ##
==========================================
+ Coverage   17.12%   17.44%   +0.31%     
==========================================
  Files          89       89              
  Lines        4752     4792      +40     
  Branches      831      848      +17     
==========================================
+ Hits          814      836      +22     
- Misses       3428     3436       +8     
- Partials      510      520      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@schu96 schu96 marked this pull request as ready for review December 4, 2024 23:13
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

This looks great @schu96 !! Great work stitching together the data from all the different places. Librarians have been asking for this feature for quite a while 😊

One question on if we can DRY the python method, one small fix which I'll apply through github; otherwise looks great!

get_work_config().identifiers, names, self.identifiers
)

def _process_identifiers(self, config_, names, values):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this method the same for editions? If so I think we might want to create a helper method at the bottom of this file that both classes call, to avoid duplicating the code, if possible!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def get_identifier_config(identifier: Literal['work', 'edition', 'author']) -> Storage:
return _get_identifier_config(identifier)
@web.memoize
def _get_identifier_config(identifier: Literal['work', 'edition', 'author']) -> Storage:
"""
Returns the identifier config.
The results are cached on the first invocation. Any changes to /config/{identifier} page require restarting the app.
This is cached because fetching and creating the Thing object was taking about 20ms of time for each book request.
"""
with open(
f'openlibrary/plugins/openlibrary/config/{identifier}/identifiers.yml'
) as in_file:
id_config = yaml.safe_load(in_file)
identifiers = [
Storage(id) for id in id_config.get('identifiers', []) if 'name' in id
]
if identifier == 'edition':
thing = web.ctx.site.get('/config/edition')
classifications = [
Storage(t.dict()) for t in thing.classifications if 'name in t'
]
roles = thing.roles
return Storage(
classifications=classifications, identifiers=identifiers, roles=roles
)
return Storage(identifiers=identifiers)

Is this something close to what you had in mind? I noticed that the author identifier config method followed a similar pattern to the work config method as well

openlibrary/templates/books/edit.html Outdated Show resolved Hide resolved
@cdrini cdrini added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Dec 10, 2024
@schu96 schu96 force-pushed the 3430/feature/add-work-ids-ui branch from e6cb911 to 3ba4710 Compare December 14, 2024 02:24
@schu96 schu96 force-pushed the 3430/feature/add-work-ids-ui branch from 7a4fec1 to 7276ac1 Compare December 14, 2024 06:26
@schu96 schu96 force-pushed the 3430/feature/add-work-ids-ui branch from 3e06667 to a13e95d Compare December 14, 2024 07:46
@schu96 schu96 force-pushed the 3430/feature/add-work-ids-ui branch from c4ce674 to c1d245b Compare December 16, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants