Created attachment 123615 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Built with inspiration based on bug 11175

Nice.

But like for analyticals, please add eval() in get_marc_volumes() in case search fails, see Bug 28682

Good call, I wasn't aware of that bug.  I'll add a follow-up layer today.

Created attachment 124367 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Regarding the eval Fridolin mentions.. I'm tempted to switch the get_volumes_query routine to using build_query_compat internally instead which should yield us a more reliably constructed search query string with special characters removed. That way, with bug 28316 work progressing we will get the advantages it brings.

Created attachment 124380 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Created attachment 124381 [details] [review]
Bug 26314: (follow-up) Use clean_search_term

This patch uses the now public clean_search_term from bug 28316 to
sanitize the title string and prevent crashes in elastic search when
title contain reserved characters.

Created attachment 126448 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Created attachment 126449 [details] [review]
Bug 26314: (follow-up) Use clean_search_term

This patch uses the now public clean_search_term from bug 28316 to
sanitize the title string and prevent crashes in elastic search when
title contain reserved characters.

Created attachment 126450 [details] [review]
Bug 26314: (follow-up) Make title search explicit

This patch updates the get_volumes_query search returned when
UseControlNumber is disabled such that it matches the query produced by
XSLT sheets by replacing the ->title accessor with the more specific
->subfield('245', "a") accessor to just take subfield `a` into account.

I think we should not load this logic into XSLT module. It does not belong there imo.
Just pass the xslt_variable show_volumes_link in the detail scripts using it (probably opac-detail, catalogue detail, shelves?)
If this would result in code duplication, add a biblio method for it?

Understood.

Let's get 11175 over the finish line before we continue on this one however.  I think we should also pass the query string itself as an XSLT param and drop the logic in the XSLT.. that way we can guarantee the link search and the search used in the controllers are always in sync.

Created attachment 126952 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Created attachment 126953 [details] [review]
Bug 26314: (follow-up) Use clean_search_term

This patch uses the now public clean_search_term from bug 28316 to
sanitize the title string and prevent crashes in elastic search when
title contain reserved characters.

Created attachment 126954 [details] [review]
Bug 26314: (follow-up) Make title search explicit

This patch updates the get_volumes_query search returned when
UseControlNumber is disabled such that it matches the query produced by
XSLT sheets by replacing the ->title accessor with the more specific
->subfield('245', "a") accessor to just take subfield `a` into account.

Created attachment 126955 [details] [review]
Bug 26314: Update for changes to bug 11175 methodology

This moves the show_volumes calculation back out of C4::XSLT into the
controller scripts and refined the search query builder slightly based
on the XSLT equivilent.

I still think it might be a good idea to pass the search string from the controller instead of constructing it in two places.. keeps it dry.

(In reply to Martin Renvoize from comment #18)
> I still think it might be a good idea to pass the search string from the
> controller instead of constructing it in two places.. keeps it dry.

Not sure what you mean here.
Constructing the query for components and volumes kind of the same? The shared logic could be moved?

(In reply to Marcel de Rooy from comment #19)
> (In reply to Martin Renvoize from comment #18)
> > I still think it might be a good idea to pass the search string from the
> > controller instead of constructing it in two places.. keeps it dry.
> 
> Not sure what you mean here.
> Constructing the query for components and volumes kind of the same? The
> shared logic could be moved?

Constructing the queries for 'components parts' and 'volumes' is similar, but not the same.. it makes sense to have methods for the two distinctly.

What I'm talking about is that the XSLT's also construct the same search query strings.. it would be cleaner to pass the constructed query string to the xslt as a parameter so we're re-using the code that builds the query.

+1

I tried to rebase, but I get stuck at the very last patch with a 'fake ancestor'. 
Martin, could you have another look maybe? I think I got you some testers :)

Created attachment 155437 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Created attachment 155438 [details] [review]
Bug 26314: (follow-up) Use clean_search_term

This patch uses the now public clean_search_term from bug 28316 to
sanitize the title string and prevent crashes in elastic search when
title contain reserved characters.

Created attachment 155439 [details] [review]
Bug 26314: (follow-up) Make title search explicit

This patch updates the get_volumes_query search returned when
UseControlNumber is disabled such that it matches the query produced by
XSLT sheets by replacing the ->title accessor with the more specific
->subfield('245', "a") accessor to just take subfield `a` into account.

Created attachment 155440 [details] [review]
Bug 26314: Update for changes to bug 11175 methodology

This moves the show_volumes calculation back out of C4::XSLT into the
controller scripts and refined the search query builder slightly based
on the XSLT equivilent.

Created attachment 155441 [details] [review]
Bug 26314: (QA follow-up) Resolve new QA script failures

Rebased as requested.. I've made sure the QA scripts are happy and that the tests are all passing.. but I've not actually confirmed the functionality is as it was originally intended.  (Honestly, it's long enough ago, I can't totally remember how it's meant to work :( )

I look forward to feedback though, so please test :)

(In reply to Martin Renvoize from comment #28)
> Rebased as requested.. I've made sure the QA scripts are happy and that the
> tests are all passing.. but I've not actually confirmed the functionality is
> as it was originally intended.  (Honestly, it's long enough ago, I can't
> totally remember how it's meant to work :( )
> 
> I look forward to feedback though, so please test :)

Thanks Martin! I start testing in a few minutes

Created attachment 155446 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155447 [details] [review]
Bug 26314: (follow-up) Use clean_search_term

This patch uses the now public clean_search_term from bug 28316 to
sanitize the title string and prevent crashes in elastic search when
title contain reserved characters.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155448 [details] [review]
Bug 26314: (follow-up) Make title search explicit

This patch updates the get_volumes_query search returned when
UseControlNumber is disabled such that it matches the query produced by
XSLT sheets by replacing the ->title accessor with the more specific
->subfield('245', "a") accessor to just take subfield `a` into account.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155449 [details] [review]
Bug 26314: Update for changes to bug 11175 methodology

This moves the show_volumes calculation back out of C4::XSLT into the
controller scripts and refined the search query builder slightly based
on the XSLT equivilent.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155450 [details] [review]
Bug 26314: (QA follow-up) Resolve new QA script failures

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Works as expected :-)

If you would like to have a look on my tests please check Sandbox:
https://staff-b26314.sandboxes.biblibre.eu/



If you search for 

index-term-genre:Monografische Reihe

you will find 3 series

only the second has the 'Show volumes' link

The first and the third don't have this link because there are no volumes.

P(In reply to Michaela Sieber from comment #35)
> 
> Works as expected :-)
> 
> If you would like to have a look on my tests please check Sandbox:
> https://staff-b26314.sandboxes.biblibre.eu/


I forgot to mention that System preference UseControlNumber is set to ‘Use’

"Use record control number ($w subfields) and control number (001) for linking of bibliographic records. "

Awesome, thanks for testing Michaela ﷐[U+1F601]﷑

Comment on attachment 155446 [details] [review]
Bug 26314: Only display volumes link when required

Review of attachment 155446 [details] [review]:
-----------------------------------------------------------------

Note to self, do we still need to 008 and other check we have in the XSLT here after the penultimate patch?

> 
> Note to self, do we still need to 008 and other check we have in the XSLT
> here after the penultimate patch?

The POD in this patch is not completely right as it only talks about 773, we could correct that, but I am not sure if it's worth changing up things now.

There are two ways to express a Set > volumes relationship:

Case 1: 773
Set:
001 		xxx
003 		DE-627

Volume with dependent title:
773		
_a volume title
_w xxx


Set:
001 		1020782544
003 		DE-627

Case 2: 8xx/449 pairs
Volume with independent title:
830 		0 	
_a Advances in analysis and geometry ;
_v Volume 5
_w 1020782544
_x 2511-0438 


And actually Michaela tested the second case. So we know it works with both cases, with and without the (003) prefix $w. (I verified)

The problem I see is the rcn index is broad. It indexes all $w subfields from various linking fields.

That's why the link includes NOT (bib-level:a OR bib-level:b) - so we can differentiate between analytical records and monographic ones.

... and that's why I thikn we better keep the checks on 008 and LDR.

Created attachment 155670 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155671 [details] [review]
Bug 26314: (follow-up) Use clean_search_term

This patch uses the now public clean_search_term from bug 28316 to
sanitize the title string and prevent crashes in elastic search when
title contain reserved characters.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155672 [details] [review]
Bug 26314: (follow-up) Make title search explicit

This patch updates the get_volumes_query search returned when
UseControlNumber is disabled such that it matches the query produced by
XSLT sheets by replacing the ->title accessor with the more specific
->subfield('245', "a") accessor to just take subfield `a` into account.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155673 [details] [review]
Bug 26314: Update for changes to bug 11175 methodology

This moves the show_volumes calculation back out of C4::XSLT into the
controller scripts and refined the search query builder slightly based
on the XSLT equivilent.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 155674 [details] [review]
Bug 26314: (QA follow-up) Resolve new QA script failures

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155675 [details] [review]
Bug 26314: (QA follow-up) Improve POD and fix links for UseControlNumber off

When UseControlNumber is off, we previously did a search on the title index,
but this was not actually correct. It might work when we have a dependent
title and the title also appears on 245$p, but there is a better way:

The title of the set record will be found in:
* 773$3 for a dependent title
* 800/810/811/830 for an independent title

The search indices to use are host-item and title-series.

As we are not only looking for 773, POD was adjusted to reflect that.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155676 [details] [review]
Bug 26314: Only display volumes link when required

This patch makes C4::XSLT query for volumes the same way it would do
with the generated link (i.e. based on UseControlNumber) and passes
a flag to the XSLT so it displays (or not) the 'Show volumes' link.

To test:
1. Apply the first patch
2. Have a known record without volumes
3. Open the record in the OPAC
=> FAIL: It shows the 'Show volumes' link
4. Have a record known to have volumes
5. Open the record in the OPAC
=> SUCCESS: It shows the 'Show volumes' link
6. Apply this patch and restart_all
7. Reload the above records
=> SUCCESS: It shows the link where it has to, and hides it where it
   shouldn't be displayed.
8. Repeat for Intranet
9. Sign off :-D

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155677 [details] [review]
Bug 26314: (follow-up) Use clean_search_term

This patch uses the now public clean_search_term from bug 28316 to
sanitize the title string and prevent crashes in elastic search when
title contain reserved characters.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155678 [details] [review]
Bug 26314: (follow-up) Make title search explicit

This patch updates the get_volumes_query search returned when
UseControlNumber is disabled such that it matches the query produced by
XSLT sheets by replacing the ->title accessor with the more specific
->subfield('245', "a") accessor to just take subfield `a` into account.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155679 [details] [review]
Bug 26314: Update for changes to bug 11175 methodology

This moves the show_volumes calculation back out of C4::XSLT into the
controller scripts and refined the search query builder slightly based
on the XSLT equivilent.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155680 [details] [review]
Bug 26314: (QA follow-up) Resolve new QA script failures

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155681 [details] [review]
Bug 26314: (QA follow-up) Improve POD and fix links for UseControlNumber off

When UseControlNumber is off, we previously did a search on the title index,
but this was not actually correct. It might work when we have a dependent
title and the title also appears on 245$p, but there is a better way:

The title of the set record will be found in:
* 773$3 for a dependent title
* 800/810/811/830 for an independent title

The search indices to use are host-item and title-series.

As we are not only looking for 773, POD was adjusted to reflect that.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Just a note on the 008: Yes, it still makes sense:

For continuing resources:
008, Pos. 21 = m - Monographic series

As the 008 fields change depending on LDR 6 and 7, there are additional requirements:

LDR, pos. 6 = Language material
AND
LDR, pos. 7 = 
 b - Serial component part
 i - integrating resource
 s - serial

We check LDR pos. 6 but omit pos=7. This was there previously and I think it does no harm for now.

Comment on attachment 155678 [details] [review]
Bug 26314: (follow-up) Make title search explicit


>-        my $cleaned_title = $marc->title;
>+        my $cleaned_title = $marc->subfield('245', "a");

Is this correct if we think of UNIMARC?

(In reply to Tomás Cohen Arazi from comment #53)
> Comment on attachment 155678 [details] [review] [review]
> Bug 26314: (follow-up) Make title search explicit
> 
> 
> >-        my $cleaned_title = $marc->title;
> >+        my $cleaned_title = $marc->subfield('245', "a");
> 
> Is this correct if we think of UNIMARC?

No, it wouldn't be, but this is currently marked as a MARC21 only feature.

Pushed to master for 23.11.

Nice work everyone, thanks!

Enhancement not pushed to 23.05.x

Hi, we are 4-5 people in the docs team looking at this and none of us understand how to get the "Show volumes" link to appear.

Can someone either specify exactly what is needed in the record for this to appear or provide example records where this works?

Thanks!

Hi!

I'm working on it--I'll try to create records where this works & attach them to this bug!

--h2 "Real Cataloger!":)

Hey Heather,

maybe this helps:

https://katalog.bibliothek.kit.edu/cgi-bin/koha/opac-search.pl?idx=&q=1748615742&weight_search=1


Series record  ( 001 = 1748615742 ) :

https://katalog.bibliothek.kit.edu/cgi-bin/koha/opac-detail.pl?biblionumber=1224415


Volume record with Link to Series in MARC field 830 $w

830 
_a Computational and applied mathematics ;
_v Volume 1
_9 1
_w 1748615742.

https://katalog.bibliothek.kit.edu/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=1224415


Regards
Michaela

(In reply to Michaela Sieber from comment #59)
> Hey Heather,
> 
> maybe this helps:
> 
> https://katalog.bibliothek.kit.edu/cgi-bin/koha/opac-search.

Hi, Michaela!

Thank you!  The MARC21 tagging looks correct.  I have no idea why this search:
rcn:1748615742
Retrieves the monographic title:
Eigenwertaufgaben in Hilbertschen Räumen
With an 001 of:
1751061221

I exported the "Computational..." bib & examined it, and it appears to have "encoding issues...?"
--h2

Created attachment 162163 [details]
Serial record for Mythlore imported & exported from https://demo-admin.catalyst-koha.com.au/

This is a MARC21 serial record that I imported from the Library of Congress for the title Mythlore, into https://demo-admin.catalyst-koha.com.au/ and then created an analytic record for a pretend article in it that I'll also attach.

--h2

Created attachment 162164 [details]
A pretend analytic MARC21 record with Mythlore as its host record

In the catalog https://demo-admin.catalyst-koha.com.au/ this analytic record for a pretend article in the serial Mythlore correctly links to Mythlore as its host record via data in its 773 field.
--h2

Looking at the last comments, I wonder if there is a confusion about volumes vs. analytics. While a volume can be linked to a set record using 773, it's a little bit different. 

Maybe these notes will help a little:

Case 1: Volume with dependent title (773)

##Set:
LDR, pos. 19 = a - Set
001 		xxx
003 		OrgCode

##Volume:
LDR, pos. 7 = m - Monograph/Item
LDR, pos. 19 = c - Part with dependent title
773		
_a volume title
_w xxx


Case 2: Volume with independent title (490/8xx)

##Set:
LDR, pos. 19 = a - Set
001 		yyy
003 		OrgCode

##Volume:
LDR, pos. 7 = m - Monograph/Item
LDR, pos. 19 = b - Part with independent title

490 1 _ 
_a Volume title
_v Vol 5
830 		0 	
_a Volume title
_v Vol 5
_w yyy

Thank you, Katrin!

Yes--I was confused, and understand much better now--thank you!  I think one reason is that OCLC libraries (like ours) don't have LDR position 19 accessible or displayed (maybe not even used) in their records at all--the Connexion Client software doesn't seem to allow for display or coding of this field.

I know it is common for academic libraries to have a bib record on which they order & receive the volumes of a monographic series, and then catalog the volumes of the series on their own bibs, but the relationships in the bib records would be expressed with 7XX linking entry fields.  The serial record used for acquisitions is sometimes even suppressed from public display.

In the US, typically a record with 490/8XX fields will not have its 8XX field linking to another bib but to an authority record for the series.  According to CONSER (Library of Congress) the 830 is an authorized access point (i.e., that must be authority controlled) so points to an authority record, not a bib record.

So I'm probably not the best person to try to understand, explain, and help craft documentation for this feature--it sounds like it's more for a non-US cataloging workflow where a volume in a monographic series would have an 8XX pointing to a bib record (rather than carrying a 7XX linking entry field pointing to a bib record).

I don't have access to a cataloging system that can show/verify the LDR position 19, so I can't verify that MARC21 coding is correct for this feature to work.  I wish I could be more help!
--h2

Would it be possible to port this feature to unimarc please ?

(In reply to Mathieu Saby from comment #65)
> Would it be possible to port this feature to unimarc please ?

Not easily as we don't know about the how multi-part records work in UNIMARC, indexing and XSLT might also need to be adjusted. A first step would be to file a new bug for UNIMARC linking it to this one and to start gathering the necessary information.

Thank you, I will do that