Bug 29416 - Regression: information from existing bib no longer populating on suggest for purchase
Summary: Regression: information from existing bib no longer populating on suggest for...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: regression, rel_20_11_candidate, rel_21_05_candidate
Depends on: 14963 28941
Blocks: 29311
  Show dependency treegraph
 
Reported: 2021-11-04 13:45 UTC by Andrew Fuerste-Henry
Modified: 2022-06-06 20:31 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This restores the behaviour for purchase suggestions for an existing title, so that the suggestion form is pre-filled with the details from the existing record.
Version(s) released in:
21.11.00,21.05.05,20.11.12,20.05.18


Attachments
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill (1.44 KB, patch)
2021-11-05 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill (1.54 KB, patch)
2021-11-05 10:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill (1.55 KB, patch)
2021-11-05 10:52 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2021-11-04 13:45:08 UTC
When suggesting an existing title for purchase, we should copy bib information from the record into the suggestion. This used to happen, but has broken in master and 21.05 (I think 21.05.04 and later). Maybe due to bug 28941? That's a shot in the dark, but is the last update to opac-suggestions.pl.
Comment 1 Jonathan Druart 2021-11-05 09:14:31 UTC
Yes, caused by 
  commit 586bed1319592e05f5dc3acf64a1dba8cae69d6b
  Bug 28941: Filter suggestion inputs at the OPAC
Comment 2 Jonathan Druart 2021-11-05 09:18:34 UTC
Created attachment 127347 [details] [review]
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill

From commit 586bed1319592e05f5dc3acf64a1dba8cae69d6b
     Bug 28941: Filter suggestion inputs at the OPAC

But later in the script we send { suggestion => %$suggestion } to the
template and so erase the variable previously passed.

Test plan:
Login at the OPAC
Go to the bibliographic detail page
Suggest for purchase
=> The form must be prefilled!
Comment 3 Marcel de Rooy 2021-11-05 10:23:46 UTC
QA: Looking here
Comment 4 Marcel de Rooy 2021-11-05 10:38:59 UTC
Out of scope, just seeing this:

{
    last unless ($op eq 'add');
# ETC
}

Normally we only use last in a loop (for, while). Perl allows this and says:
"Note that a block by itself is semantically identical to a loop that executes once. Thus last can be used to effect an early exit out of such a block."

But surely this should just have been:

if( $op eq 'add' ) {
# ETC
}
Comment 5 Marcel de Rooy 2021-11-05 10:46:12 UTC
Created attachment 127354 [details] [review]
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill

From commit 586bed1319592e05f5dc3acf64a1dba8cae69d6b
     Bug 28941: Filter suggestion inputs at the OPAC

But later in the script we send { suggestion => %$suggestion } to the
template and so erase the variable previously passed.

Test plan:
Login at the OPAC
Go to the bibliographic detail page
Suggest for purchase
=> The form must be prefilled!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2021-11-05 10:47:59 UTC
Tested, seems to work ;)
Can be backported as a minimal fix now. But surely this code should get more attention in terms of quality. Advanced hacking.
Comment 7 Nick Clemens 2021-11-05 10:52:04 UTC
Created attachment 127356 [details] [review]
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill

From commit 586bed1319592e05f5dc3acf64a1dba8cae69d6b
     Bug 28941: Filter suggestion inputs at the OPAC

But later in the script we send { suggestion => %$suggestion } to the
template and so erase the variable previously passed.

Test plan:
Login at the OPAC
Go to the bibliographic detail page
Suggest for purchase
=> The form must be prefilled!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Jonathan Druart 2021-11-05 11:05:27 UTC
(In reply to Marcel de Rooy from comment #4)
> Out of scope, just seeing this:
> 
> {
>     last unless ($op eq 'add');
> # ETC
> }
> 
> Normally we only use last in a loop (for, while). Perl allows this and says:
> "Note that a block by itself is semantically identical to a loop that
> executes once. Thus last can be used to effect an early exit out of such a
> block."
> 
> But surely this should just have been:
> 
> if( $op eq 'add' ) {
> # ETC
> }

Please add a bug report for that.
Comment 9 Jonathan Druart 2021-11-05 11:15:18 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 10 Kyle M Hall 2021-11-11 12:33:53 UTC
Pushed to 21.05.x for 21.05.05
Comment 11 Fridolin Somers 2021-11-12 23:17:58 UTC
Pushed to 20.11.x for 20.11.12
Comment 12 Victor Grousset/tuxayo 2021-11-16 22:33:47 UTC
Backported: Pushed to 20.05.x branch for 20.05.18
Comment 13 wainuiwitikapark 2021-11-23 00:22:02 UTC
Not backporting to 19.11.x - LTS version will just backport security patches