Bug 29416

Summary: Regression: information from existing bib no longer populating on suggest for purchase
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: OPACAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: david, fridolin.somers, jonathan.druart, kyle, m.de.rooy, victor, wainuiwitikapark
Version: MainKeywords: regression, rel_20_11_candidate, rel_21_05_candidate
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29321
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29696
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
Bug Depends on: 14963, 28941    
Bug Blocks: 29311    
Attachments: Bug 29416: (bug 28941 follow-up) Restore suggestion prefill
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill
Bug 29416: (bug 28941 follow-up) Restore suggestion prefill

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