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

enrich ISBNs (Alma) #1180

Closed
hagbeck opened this issue Mar 31, 2021 · 7 comments · Fixed by #1186
Closed

enrich ISBNs (Alma) #1180

hagbeck opened this issue Mar 31, 2021 · 7 comments · Fixed by #1186
Assignees
Labels
Milestone

Comments

@hagbeck
Copy link
Collaborator

hagbeck commented Mar 31, 2021

Currently Alma exports only the ISBNs which are in the origin MARC record.

Please use morph functions <isbn to="isbn10"/> and <isbn to="isbn13"/> to ensure that both versions of ISBNs find their way in the lobid-resources.

Example: http://alma.lobid.org/resources/990030520380206441.json

@dr0i dr0i self-assigned this Apr 1, 2021
@dr0i dr0i added this to the almaMigration milestone Apr 1, 2021
dr0i added a commit that referenced this issue Apr 1, 2021
If an isbn10 exists a corresponding isbn13 is generated.
Also, an isbn is normalized by removing semnatic sugar, so that:

-  "isbn" : [ "3-642-32079-1" ],
+  "isbn" : [ "3642320791", "9783642320798" ],

See #1180.
@dr0i dr0i mentioned this issue Apr 1, 2021
@dr0i
Copy link
Member

dr0i commented Apr 1, 2021

If an isbn10 exists a corresponding isbn13 is generated.
Also, an isbn is normalized by removing semnatic sugar, so that:

-  "isbn" : [ "3-642-32079-1" ],
+  "isbn" : [ "3642320791", "9783642320798" ],

Going to deploy.

@dr0i
Copy link
Member

dr0i commented Apr 7, 2021

Deployed, see e.g. #1184.
Closing.

@dr0i dr0i closed this as completed Apr 7, 2021
@dr0i
Copy link
Member

dr0i commented Apr 7, 2021

c6e59ec introduced duplicate isbns. This cannot be seen in the test data (https://github.com/hbz/lobid-resources/blob/master/src/test/resources/alma/(DE-605)HT019246898.json) but in the new index (http://alma.lobid.org/resources/search?q=HT019246898&format=json). That's because the tests are not working atm (see #1124) and we have to check manually by doing a mvn failsafe:integration-test -Dit.test=AlmaMarc21XmlToLobidJsonTest; git diff.
Reopened.

@dr0i dr0i reopened this Apr 7, 2021
@TobiasNx
Copy link
Contributor

TobiasNx commented Apr 8, 2021

I assume that the source data already has ISBN-10 and ISBN-13. It would make sense to add the <unique />-function at the end of each transformation.
Also this seems not to be a problem of the testing but of working multiple branches at the same time. The test file you refer to (https://github.com/hbz/lobid-resources/blob/master/src/test/resources/alma/(DE-605)HT019246898.json) does not exist in the 1180-enrichIsbn branch. It was introduced in the 1171-incorrectCurrentLocation. (https://github.com/hbz/lobid-resources/commits/master/src/test/resources/alma/(DE-605)HT019246898.json)

Due to the merging of multiple PRs at the same time the test data does not reflect all changes but only those that are pushed with the specific branches. Perhaps we should update the testfile after merging the master in the feature branch before merging the PR.

@TobiasNx
Copy link
Contributor

TobiasNx commented Apr 8, 2021

I updated the morph and open a PR now.

TobiasNx added a commit that referenced this issue Apr 8, 2021
The transformed testfile was behind the updated morph process due to being introduced in a differenct branch.

#1180 (comment)
@TobiasNx TobiasNx linked a pull request Apr 8, 2021 that will close this issue
@dr0i
Copy link
Member

dr0i commented Apr 8, 2021

Indeed, the test resource HT019246898.json was added at a later stage. It proves to be a very good test case also for isbn, because this test case (a resource having both, isbn10 and isbn13) was not present before.

Due to the merging of multiple PRs at the same time the test data does not reflect all changes ... Perhaps we should update the testfile after merging the master in the feature branch before merging the PR.

Yes, in this case it's true (working tests would come in handy here ;) ).

@dr0i dr0i closed this as completed in #1186 Apr 8, 2021
@dr0i
Copy link
Member

dr0i commented Apr 8, 2021

@TobiasNx TobiasNx added the ALMA label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants