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

Reuse nwbib fix for #1992, #1951 and #1829 #2067

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Reuse nwbib fix for #1992, #1951 and #1829 #2067

merged 3 commits into from
Sep 11, 2024

Conversation

TobiasNx
Copy link
Contributor

@TobiasNx TobiasNx commented Sep 2, 2024

Resolves #1951 and #1829 Wrong ISBN and ISSN

and also fixes #1992 missing linking to Bände and Enthält

hbz/nwbib#636

@TobiasNx TobiasNx requested a review from fsteeg September 2, 2024 16:32
@TobiasNx TobiasNx changed the title Reuse nwbib fix for #1951 and #1829 Reuse nwbib fix for #1992, #1951 and #1829 Sep 3, 2024
@fsteeg
Copy link
Member

fsteeg commented Sep 4, 2024

Build in GitHub Action fails, the change does not compile. You should always test locally (sbt run in web/) before pushing, and check the GitHub Action before opening a PR.

@fsteeg fsteeg removed their request for review September 4, 2024 06:36
@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Sep 4, 2024
@TobiasNx TobiasNx marked this pull request as draft September 4, 2024 07:29
@TobiasNx
Copy link
Contributor Author

TobiasNx commented Sep 4, 2024

@fsteeg you are right. It does not compile and the solution is not as easy as just copying the lines from the nwbib project. I assume that the failed compiling is due to nwbib still using JAVA 8 and lobid JAVA 11

@fsteeg
Copy link
Member

fsteeg commented Sep 4, 2024

I assume that the failed compiling is due to nwbib still using JAVA 8 [...]

You can see the error in the build log of the GitHub Action:

https://github.com/hbz/lobid-resources/actions/runs/10678801716/job/29596615852?pr=2067#step:10:900

The problem seems to be the actual code change.

the solution is not as easy as just copying the lines from the nwbib project

Yes, but if you have it running locally I'm sure we can set it up together.

@TobiasNx TobiasNx force-pushed the 1951-ISSNISBN branch 3 times, most recently from 3e3f270 to 5796c76 Compare September 5, 2024 10:11
@TobiasNx TobiasNx assigned fsteeg and unassigned TobiasNx Sep 6, 2024
@TobiasNx TobiasNx requested a review from fsteeg September 6, 2024 07:24
@TobiasNx TobiasNx marked this pull request as ready for review September 6, 2024 07:24
@TobiasNx
Copy link
Contributor Author

TobiasNx commented Sep 6, 2024

@fsteeg now it should work.

@fsteeg
Copy link
Member

fsteeg commented Sep 10, 2024

I've deployed your change to test: https://test.lobid.org/resources

now it should work.

Bände / Enthält seems to work now, but the ISBN / ISSN fields now show the full URI ID. In general, I think it would be better to have separate PRs for these separate issues (we've had that topic in the last Metafacture meeting too). Also, it would really be more efficient if you get your setup running locally, so you can test if it actually works before pushing and requesting a review.

@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Sep 10, 2024
@TobiasNx
Copy link
Contributor Author

Had a look at Table row values that is called for ISBN and ISSN, NwBib seems to differ a lot from lobid. I am not sure how to solve this any further.

@fsteeg
Copy link
Member

fsteeg commented Sep 11, 2024

I've replaced the jsonVal block with the nwbib version: fcf01f4

Seems good now, e.g.: https://test.lobid.org/resources/990366384450206441

@TobiasNx TobiasNx merged commit b828d09 into master Sep 11, 2024
1 check passed
@TobiasNx TobiasNx deleted the 1951-ISSNISBN branch September 11, 2024 11:53
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.

Linked "Bände" and "Enthält" are not in GUI ALMA ISBN of related resources in GUI
2 participants