Bug 33445 - Regression - Replacing authority via Z39.50 will not search for anything but the value from the existing authority
Summary: Regression - Replacing authority via Z39.50 will not search for anything but ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords: regression
Depends on: 29173
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-07 15:31 UTC by Phil Ringnalda
Modified: 2023-12-28 20:47 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06


Attachments
Revert "Bug 29173: Fixed pre-filling of the search form from the "Replace record with ZR39.50/SRU" (Authority page detail)" (3.56 KB, patch)
2023-04-11 21:18 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Revert "Bug 29173: Fixed pre-filling of the search form from the "Replace record with ZR39.50/SRU" (Authority page detail)" (3.60 KB, patch)
2023-04-13 19:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 33445: [alternate] Pass through form info if present and use record values otherwise (4.09 KB, patch)
2023-04-28 12:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33445: [alternate] Pass through form info if present and use record values otherwise (4.27 KB, patch)
2023-04-28 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33445: [alternate] Pass through form info if present and use record values otherwise (4.33 KB, patch)
2023-04-28 19:18 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 33445: Pass through form info if present and use record values otherwise (4.31 KB, patch)
2023-05-01 18:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33445: (follow-up) Pass form values through when clicking 'Try another search' (4.53 KB, patch)
2023-05-01 18:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search (5.56 KB, patch)
2023-05-01 18:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search (5.56 KB, patch)
2023-05-01 19:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33445: Pass through form info if present and use record values otherwise (4.36 KB, patch)
2023-05-01 19:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 33445: (follow-up) Pass form values through when clicking 'Try another search' (4.58 KB, patch)
2023-05-01 19:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search (5.60 KB, patch)
2023-05-01 19:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 33445: Pass through form info if present and use record values otherwise (4.36 KB, patch)
2023-05-01 19:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 33445: (follow-up) Pass form values through when clicking 'Try another search' (4.58 KB, patch)
2023-05-01 19:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search (5.48 KB, patch)
2023-05-01 19:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 33445: Pass through form info if present and use record values otherwise (4.46 KB, patch)
2023-05-12 09:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33445: (follow-up) Pass form values through when clicking 'Try another search' (4.67 KB, patch)
2023-05-12 09:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search (5.58 KB, patch)
2023-05-12 09:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2023-04-07 15:31:34 UTC
There are two steps to cataloguing/z3950_auth_search.pl - load it without op=do_search and it inserts the value from any params that match a form field name into the form and displays the form, or load it with op=do_search, and it searches for the values from params that match form field names and displays the results.

Bug 29173 implemented prefilling the form for the case where you are coming from an authority detail view rather than an authority editor by looking up the existing authority record, and inserting values from it into the variables holding the input params.

It does that in both the first step (overwriting changes you've made in the editor) and also in the do_search step, overwriting what you put in the search form with the data from the existing stored authority record.

Steps to reproduce:

1. Find and edit any existing corporate name, personal name, subject or uniform title authority which will have results from the Z39.50 server you're going to search. For a usable ktd data and Library of Congress Names server example, use Smith, Bessie, 1894-1937.
2. In the editor, note what's in the main entry field's subfield a (Smith, Bessie)
3. Replace subfield a's contents with something different, like your own name
4. Click "Replace record via Z39.50/SRU search"
5. Note that rather than what you put in, the form field is filled with what the record originally had
6. Replace it with your name again, this time in the search form, select the server to search, and click Search
7. Note that the results are for what was originally in the record, not what you put in the form

There are two cases where that makes replace via Z39.50 unusable: when you need something different than the original, like after a change in heading or when your existing record passed through an encoding error and you need to turn Gâomez, Hâector Carlos into Gómez, Héctor Carlos, and cases where just the subfield a is not enough to distinguish, like searching for Smith, John when you want Smith, John, Jr., -1853 who appears on page 39 of the results. Oh, and any name for UNIMARC, since subfield a is only the surname.

A very good start to fixing this would be to revert bug 29173 from 22.11, 22.05, and 21.11, since this feature is now broken back to oldoldstable.
Comment 1 Phil Ringnalda 2023-04-11 21:18:14 UTC
Created attachment 149504 [details] [review]
Revert "Bug 29173: Fixed pre-filling of the search form from the "Replace record with ZR39.50/SRU" (Authority page detail)"

This reverts commit 26c26164ba78029cb99fa99662f9e0a8ce9557df.

Test plan:

1. Without the patch, edit a personal name authority record,  replacing
the value in 100 subfield a (200 subfield a for UNIMARC) with your own
name.
2. Click "Replace record via Z39.50/SRU search" and note that the
prefilled search field contains the original value, not your name.
3. Replace the Author (personal) field with your name again, choose
a search target, and click Search.
4. Note that the results of the search are for the original value, not
for your name.
5. Apply patch, repeat, noting that the search form is prefilled with
your change made in the editor, and any changes you make in the search
form are reflected in what the search results return.
Comment 2 David Nind 2023-04-13 19:42:19 UTC
Created attachment 149624 [details] [review]
Revert "Bug 29173: Fixed pre-filling of the search form from the "Replace record with ZR39.50/SRU" (Authority page detail)"

This reverts commit 26c26164ba78029cb99fa99662f9e0a8ce9557df.

Test plan:

1. Without the patch, edit a personal name authority record,  replacing
the value in 100 subfield a (200 subfield a for UNIMARC) with your own
name.
2. Click "Replace record via Z39.50/SRU search" and note that the
prefilled search field contains the original value, not your name.
3. Replace the Author (personal) field with your name again, choose
a search target, and click Search.
4. Note that the results of the search are for the original value, not
for your name.
5. Apply patch, repeat, noting that the search form is prefilled with
your change made in the editor, and any changes you make in the search
form are reflected in what the search results return.

https://bugs.koha-community.org/show_bug.cgi?id=33445
Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2023-04-13 19:45:45 UTC
Testing notes (using KTD):

1. I have signed off based on what is in the test plan working. I'm not familiar enough with authorities, and how it should work, to comment on that.

2. I think the bug title/description needs fixing - see https://wiki.koha-community.org/wiki/Commit_messages#Subject_line
Comment 4 Katrin Fischer 2023-04-16 21:51:13 UTC
Hi Phil, if you are working on a patch, please remember to make yourself Assignee of the bug. It helps to avoid doubled up work among other things.
Comment 5 Nick Clemens 2023-04-28 12:52:23 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2023-04-28 12:58:15 UTC
Created attachment 150368 [details] [review]
Bug 33445: [alternate] Pass through form info if present and use record values otherwise

This patch updates the form to pass through values formthe form when searching, and
to use record values if nothing is passed in.

TO test:
 1 - Without the patch, edit a personal name authority record
 2 - Replace 100 subfield a (200 subfield a for UNIMARC) with your own name
 3 - Click "Replace record via Z39.50/SRU search"
 4 - Note that prefilled search field contains the original value, not your name.
 5 - Replace the Author (personal) field with your name in the search form
 6 - choose a search target, and click Search.
 7 - Note that the results of the search are for the original value, not your name
 8 - From the details page of a record, click 'Edit -> Replace record via Z3950'
 9 - Note form is prefilled
10 - Change value as in 5 and search
11 - Note original term was searched
12 - Apply patch, restart all
13 - Repeat searches as before
14 - Confirm that form is prefilled with record or the updated value
15 - Confirm your entered search terms are always searched
Comment 7 Nick Clemens 2023-04-28 12:59:16 UTC
Rather than revert, I think we need to ensure we handle record values when needed, and the form values when searching. I attached an alternate
Comment 8 Phil Ringnalda 2023-04-28 19:17:44 UTC
That does still retain one behavior change, altering the meaning of the "Try another search" button on search results from the historic "Try a different search starting from scratch with an empty form" to "Try another search starting with a form prefilled from the saved record" but I can't object too much to that, since I have always avoided the another search from scratch button, preferring to use the browser's back button if I'm only a page or two forward, or just closing and reopening the popup if I'm several pages deep.

According to my concept of the meaning of "stable" we should still revert back down to oldoldstable and then land this on master, but obviously my concept of stable is very different from Koha's.
Comment 9 Phil Ringnalda 2023-04-28 19:18:36 UTC
Created attachment 150419 [details] [review]
Bug 33445: [alternate] Pass through form info if present and use record values otherwise

This patch updates the form to pass through values formthe form when searching, and
to use record values if nothing is passed in.

TO test:
 1 - Without the patch, edit a personal name authority record
 2 - Replace 100 subfield a (200 subfield a for UNIMARC) with your own name
 3 - Click "Replace record via Z39.50/SRU search"
 4 - Note that prefilled search field contains the original value, not your name.
 5 - Replace the Author (personal) field with your name in the search form
 6 - choose a search target, and click Search.
 7 - Note that the results of the search are for the original value, not your name
 8 - From the details page of a record, click 'Edit -> Replace record via Z3950'
 9 - Note form is prefilled
10 - Change value as in 5 and search
11 - Note original term was searched
12 - Apply patch, restart all
13 - Repeat searches as before
14 - Confirm that form is prefilled with record or the updated value
15 - Confirm your entered search terms are always searched

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 10 David Nind 2023-04-28 19:55:18 UTC
I was testing at the same time as Phil (using KTD):

1. Everything works as per the test plan.

2. However, after applying the patch and repeating the searches:
   - for Step 6, after getting no results using my name and clicking on "Try another search", the name in the field reverts to the original record name and not my name
   - for Step 10, the same as step 6 - which is probably correct (but I'm not sure...)
   - feel free to change the status to Failed QA if this is not the expected behavour (I would probably have failed QA if there wasn't already a sign off)

3. I tested using MARC21 and UNIMARC instances.
Comment 11 Katrin Fischer 2023-05-01 13:03:35 UTC
(In reply to David Nind from comment #10)
> I was testing at the same time as Phil (using KTD):
> 
> 1. Everything works as per the test plan.
> 
> 2. However, after applying the patch and repeating the searches:
>    - for Step 6, after getting no results using my name and clicking on "Try
> another search", the name in the field reverts to the original record name
> and not my name
>    - for Step 10, the same as step 6 - which is probably correct (but I'm
> not sure...)
>    - feel free to change the status to Failed QA if this is not the expected
> behavour (I would probably have failed QA if there wasn't already a sign off)
> 
> 3. I tested using MARC21 and UNIMARC instances.

Hi Nick, could you please have a look at David's notes here? I believe we'd always want the new name, so that would be one last glitch to fix.
Comment 12 Nick Clemens 2023-05-01 18:40:29 UTC
Created attachment 150466 [details] [review]
Bug 33445: Pass through form info if present and use record values otherwise

This patch updates the form to pass through values formthe form when searching, and
to use record values if nothing is passed in.

TO test:
 1 - Without the patch, edit a personal name authority record
 2 - Replace 100 subfield a (200 subfield a for UNIMARC) with your own name
 3 - Click "Replace record via Z39.50/SRU search"
 4 - Note that prefilled search field contains the original value, not your name.
 5 - Replace the Author (personal) field with your name in the search form
 6 - choose a search target, and click Search.
 7 - Note that the results of the search are for the original value, not your name
 8 - From the details page of a record, click 'Edit -> Replace record via Z3950'
 9 - Note form is prefilled
10 - Change value as in 5 and search
11 - Note original term was searched
12 - Apply patch, restart all
13 - Repeat searches as before
14 - Confirm that form is prefilled with record or the updated value
15 - Confirm your entered search terms are always searched

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 13 Nick Clemens 2023-05-01 18:40:32 UTC
Created attachment 150467 [details] [review]
Bug 33445: (follow-up) Pass form values through when clicking 'Try another search'

We didn't include anything except the authid id in the try again form, this
patch extends to cover all the fields
Comment 14 Nick Clemens 2023-05-01 18:40:35 UTC
Created attachment 150468 [details] [review]
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search

It seems the form didn't populate all fields form data passed in. This patch
corrects that
Comment 15 Phil Ringnalda 2023-05-01 19:04:32 UTC
Comment on attachment 150468 [details] [review]
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search

>-[% USE raw %]
>+nt[% USE raw %]

Haven't looked at whether that nt belonged somewhere else, but it doesn't look right there.
Comment 16 David Nind 2023-05-01 19:08:13 UTC
(In reply to Phil Ringnalda from comment #15)
> Comment on attachment 150468 [details] [review] [review]
> Bug 33445: (follow-up) Allow all search fields to be passed through and
> returned when trying another search
> 
> >-[% USE raw %]
> >+nt[% USE raw %]
> 
> Haven't looked at whether that nt belonged somewhere else, but it doesn't
> look right there.

Was just retesting - the nt appears at the top left of the Z3950 search window.
Comment 17 Nick Clemens 2023-05-01 19:09:49 UTC
Created attachment 150469 [details] [review]
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search

It seems the form didn't populate all fields form data passed in. This patch
corrects that
Comment 18 Nick Clemens 2023-05-01 19:10:35 UTC
(In reply to David Nind from comment #16)
> (In reply to Phil Ringnalda from comment #15)
> 
> Was just retesting - the nt appears at the top left of the Z3950 search
> window.

That was a slip, new patch attached
Comment 19 David Nind 2023-05-01 19:14:27 UTC Comment hidden (obsolete)
Comment 20 David Nind 2023-05-01 19:14:29 UTC Comment hidden (obsolete)
Comment 21 David Nind 2023-05-01 19:14:32 UTC Comment hidden (obsolete)
Comment 22 David Nind 2023-05-01 19:16:42 UTC
(In reply to Nick Clemens from comment #18)
> (In reply to David Nind from comment #16)
> > (In reply to Phil Ringnalda from comment #15)
> > 
> > Was just retesting - the nt appears at the top left of the Z3950 search
> > window.
> 
> That was a slip, new patch attached

Thanks Nick!

Now signed off.

I added some values to the keyword input field, and these were retained (including 'Try another search'.

David
Comment 23 David Nind 2023-05-01 19:32:11 UTC
Created attachment 150473 [details] [review]
Bug 33445: Pass through form info if present and use record values otherwise

This patch updates the form to pass through values formthe form when searching, and
to use record values if nothing is passed in.

TO test:
 1 - Without the patch, edit a personal name authority record
 2 - Replace 100 subfield a (200 subfield a for UNIMARC) with your own name
 3 - Click "Replace record via Z39.50/SRU search"
 4 - Note that prefilled search field contains the original value, not your name.
 5 - Replace the Author (personal) field with your name in the search form
 6 - choose a search target, and click Search.
 7 - Note that the results of the search are for the original value, not your name
 8 - From the details page of a record, click 'Edit -> Replace record via Z3950'
 9 - Note form is prefilled
10 - Change value as in 5 and search
11 - Note original term was searched
12 - Apply patch, restart all
13 - Repeat searches as before
14 - Confirm that form is prefilled with record or the updated value
15 - Confirm your entered search terms are always searched

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2023-05-01 19:32:14 UTC
Created attachment 150474 [details] [review]
Bug 33445: (follow-up) Pass form values through when clicking 'Try another search'

We didn't include anything except the authid id in the try again form, this
patch extends to cover all the fields

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2023-05-01 19:32:17 UTC
Created attachment 150475 [details] [review]
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search

It seems the form didn't populate all fields form data passed in. This patch
corrects that

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2023-05-01 19:34:39 UTC
Apologies for the noise - somehow managed to add the patch with the 'nt' when I signed off. Amended the patch, so should now be correct!
Comment 27 Marcel de Rooy 2023-05-12 09:29:52 UTC
Created attachment 151125 [details] [review]
Bug 33445: Pass through form info if present and use record values otherwise

This patch updates the form to pass through values formthe form when searching, and
to use record values if nothing is passed in.

TO test:
 1 - Without the patch, edit a personal name authority record
 2 - Replace 100 subfield a (200 subfield a for UNIMARC) with your own name
 3 - Click "Replace record via Z39.50/SRU search"
 4 - Note that prefilled search field contains the original value, not your name.
 5 - Replace the Author (personal) field with your name in the search form
 6 - choose a search target, and click Search.
 7 - Note that the results of the search are for the original value, not your name
 8 - From the details page of a record, click 'Edit -> Replace record via Z3950'
 9 - Note form is prefilled
10 - Change value as in 5 and search
11 - Note original term was searched
12 - Apply patch, restart all
13 - Repeat searches as before
14 - Confirm that form is prefilled with record or the updated value
15 - Confirm your entered search terms are always searched

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2023-05-12 09:29:55 UTC
Created attachment 151126 [details] [review]
Bug 33445: (follow-up) Pass form values through when clicking 'Try another search'

We didn't include anything except the authid id in the try again form, this
patch extends to cover all the fields

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2023-05-12 09:29:57 UTC
Created attachment 151127 [details] [review]
Bug 33445: (follow-up) Allow all search fields to be passed through and returned when trying another search

It seems the form didn't populate all fields form data passed in. This patch
corrects that

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Tomás Cohen Arazi 2023-05-12 14:21:16 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 31 Matt Blenkinsop 2023-05-15 12:42:32 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 32 Lucas Gass 2023-06-15 19:18:53 UTC
Merge conflicts in 22.05.x, no backport. Please rebase if needed.