Change related.issn to array and delete - #1828

[TobiasNx]

Resolves #1828

ISSN is defined as array in the context therefore I changed related.issn to an array.

lobid-resources/web/conf/context.jsonld

Lines 89 to 92 in 3b710e2

	"issn" : {
	"@id" : "http://purl.org/ontology/bibo/issn",
	"@container" : "@set"
	},

Also it has a - which should be deleted.

[dr0i]

checks failed, reassigning

[TobiasNx]

[error] Test tests.IndexIntegrationTest.testResultCount[(83) {Q=q.all:07206763}] failed: expected:<[0]> but was:<[1]>, took 0.076 sec
...
[error] Test tests.IndexIntegrationTest.testResultCount[(84) {Q=q.all:0720\-6763}] failed: expected:<[1]> but was:<[0]>, took 0.091 sec

@blackwinter These are your tests. I have deleted the - in related.issn and turned issn into an array as defined in the context.
Could you have a look?

[TobiasNx]

@dr0i could it be that with the change from string to array related.issn is not indexed anymore: #1267

[blackwinter]

These tests illustrate that there's no normalization happening for ISSNs (in contrast to ISBNs). So if you changed the data, just adjust the tests accordingly (the new behaviour is the expected behaviour).

[TobiasNx]

I updated one test. It seems that the hyphen normalization is not supported for related.issn

[dr0i]

You didn't update:

[error] Test tests.IndexIntegrationTest.testResultCount[(84) {Q=q.all:0720-6763}] failed: expected:<[1]> but was:<[0]>

i.e. 0720-6763 cannot be queried. Idk if this is something that you want to work?

[TobiasNx]

Idk if this is something that you want to work?

ISSNs are usually with - as ISBNs therefore ISSN with and without - should work.

[blackwinter]

Also it has a - which should be deleted.

Why is that?

ISSNs are usually with - as ISBNs therefore ISSN with and without - should work.

So add both. It's an array now after all.

[TobiasNx]

Also it has a - which should be deleted.

Why is that?

ISSNs are usually with - as ISBNs therefore ISSN with and without - should work.

So add both. It's an array now after all.

This is the same situation with ISBN isn't it? We only use the ISBN without -. The same is the "historical" situation with ISSN.

lobid-resources/src/main/resources/morph-hbz01-to-lobid.xml

Lines 7557 to 7559 in 7f982c1

	<data source="542[-ab][-1].[a]" name="$[ns-bibo]issn">
	<replace pattern="-| " with=""/>
	</data>

[blackwinter]

Okay, I understand. But we don't want ISSN normalization in q.all, this is what got us in trouble with hbz IDs (see 2f45f28).

[TobiasNx]

This can wait until after your holiday.
d167733

this seems to be the commit. also we we already normalize the issn for the record:

lobid-resources/src/main/resources/alma/fix/identifiers.fix

Line 78 in 7c2be26

replace_all("issn[].*", "-","")

Also I do not see how this is any different to ISBN or why this is handled differently to ISBN:

lobid-resources/web/test/tests/IndexIntegrationTest.java

Lines 117 to 120 in 27e0915

	{ "q.all:0702075558", /*->*/ 1 },
	{ "q.all:07\\-0207\\-555\\-8", /*->*/ 1 },
	{ "q.all:07206763", /*->*/ 0 },
	{ "q.all:0720\\-6763", /*->*/ 1 },

There we also do not keep the hyphen.

[blackwinter]

Sorry, that's not what I meant with "normalization". I was talking about the standardnumber filter.

As I said before:

just adjust the tests accordingly (the new behaviour is the expected behaviour)

[TobiasNx]

Ah, I see what the reason was. related.isbn and realted.issn were not configured in the same way in the index-config,json as issn and isbn. I commited a change for the index config.

[dr0i]

@TobiasNx will adopt the test to reflect a related.issn query. Besides this the PR is good.

[TobiasNx]

@dr0i could you have another look?

[dr0i]

Okay, I understand. But we don't want ISSN normalization in q.all, this is what got us in trouble with hbz IDs (see 2f45f28).

Do you ACK @TobiasNx ? We won't be able to query an ISSN with a hyphen.

@blackwinter but the same is OK for ISBN? (Look at 2f45f28 and test if q.all=HT0702075558 hits - it does!)

[blackwinter]

We won't be able to query an ISSN with a hyphen.

You're not using the query field q.all, are you?

but the same is OK for ISBN? (Look at 2f45f28 and test if q.all=HT0702075558 hits - it does!)

HT0702075558 is not a valid hbz ID, these only have 9 digits (so far).

[TobiasNx]

I am confused what is blocking the PR:
Original intention of this PR was:

1. To change the related.issn-element to an array as the top-level issn-element already is (as well as isbn and related.isbn are). (3b710e2)
2. Also to delete the hyphen as we already do with the issn-element (as well as with isbn and related.isbn). (3b710e2)
3. To ensure that one can search for records with an issn with and without hyphen I changed the index config (18b55a7) and added a new test (d391be8).

This should be reflected in the 3 commits (not counting the merge).

Anything wrong with this?

PS: I see that the test still fails for the issn with hyphen using q.all:
[error] Test tests.IndexIntegrationTest.testResultCount[(84) {Q=q.all:0720\-6763}] failed: expected:<[1]> but was:<[0]>, took 0.078 sec

But with the isbn it works:

lobid-resources/web/test/tests/IndexIntegrationTest.java

Lines 117 to 121 in 6e22f91

	{ "q.all:0702075558", /*->*/ 1 },
	{ "q.all:07\\-0207\\-555\\-8", /*->*/ 1 },
	{ "q.all:07206763", /*->*/ 1 },
	{ "q.all:0720\\-6763", /*->*/ 1 },
	{ "q.all:HT072067630", /*->*/ 0 },

[blackwinter]

I see that the test still fails for the issn with hyphen using q.all:

As I said before, you have to adjust the tests to match your change in the data. Basically, you have to switch the results for q.all:07206763 and q.all:0720-6763. You only did the first half so far.

These tests show that it's a behaviour change, but DigiBib (union catalogue) has to live with it I suppose.

[blackwinter]

But with the isbn it works:

ISBNs receive special treatment via the digibib_standardnumber filter.

[TobiasNx]

Talked to @blackwinter off board.

Queries with q.all are not the general lobid search but a specific DigiBib-Search.
I added issn and related.issn tests.

Now the test should work.

[blackwinter]

Now the test should work.

There are still some failures. Will review when everything passes.

[TobiasNx]

@dr0i can you have a look. It seems the indexIntegration tests work locally but the tests still seem to fail.

[dr0i]

@blackwinter you can have a look now

[dr0i]

Will be deployed next Monday.