Bug 16519

Summary: Do not use global variables in [opac-]addbybiblionumbers.pl
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: frederic, kyle, m.de.rooy, veron
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16517, 16518    
Bug Blocks:    
Attachments: Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl
Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl
Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl
[PASSED QA] Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl

Description Jonathan Druart 2016-05-13 19:57:38 UTC
To avoid bug like bug 16518 and to ease the readability/maintainability of these scripts, it would be great to see them rewritten not to use global variables.
Comment 1 Jonathan Druart 2016-05-13 20:02:23 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-05-25 14:08:42 UTC
Sorry, QA tools complain:
 FAIL	opac/opac-addbybiblionumber.pl
   FAIL	  valid
		Global symbol "$bib" requires explicit package name 
		opac/opac-addbybiblionumber.pl had compilation errors.
		Found = in conditional, should be == 
		Can't modify scalar in scalar assignment
Comment 3 Jonathan Druart 2016-05-25 16:10:55 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-05-25 16:11:48 UTC
Shame on me, sorry about that.
I have decided to rename the $bib var after my tests, but I have missed one!
Comment 5 Marc Véron 2016-05-28 19:12:09 UTC
Created attachment 51903 [details] [review]
Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl

To avoid bug like bug 16518 and to ease the readability/maintainability
of these scripts, this patch replaces the use of 'our' with 'my' to
avoid the use of global variables.

Basically the code has been moved from subroutines to the appropriate places.

Test plan:
At the intranet and OPAC sides
1/ Add items to a list
2/ Add items to a list using an existing name
3/ Add items to a list you don't have right on it (by modifying the
biblionumber in the url)
4/ At the OPAC, use the opac-addbybiblionumber.pl without being logged
in to add items to a list

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 6 Marcel de Rooy 2016-08-19 06:19:22 UTC
Hm Not sure if you are improving the code here. Wasn't it possible to move a lot of this code to some module shared by both scripts?
Comment 7 Jonathan Druart 2016-08-19 07:38:09 UTC
(In reply to Marcel de Rooy from comment #6)
> Hm Not sure if you are improving the code here. Wasn't it possible to move a
> lot of this code to some module shared by both scripts?

With this code this script will now follow the same pattern as other scripts.
So yes, I think it improves the code.
Everything is possible :) I did not want to add too much changes just for a var scope problem.
Comment 8 Katrin Fischer 2016-09-18 20:04:18 UTC
Created attachment 55662 [details] [review]
[PASSED QA] Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl

To avoid bug like bug 16518 and to ease the readability/maintainability
of these scripts, this patch replaces the use of 'our' with 'my' to
avoid the use of global variables.

Basically the code has been moved from subroutines to the appropriate places.

Test plan:
At the intranet and OPAC sides
1/ Add items to a list
2/ Add items to a list using an existing name
3/ Add items to a list you don't have right on it (by modifying the
biblionumber in the url)
4/ At the OPAC, use the opac-addbybiblionumber.pl without being logged
in to add items to a list

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 9 Kyle M Hall 2016-09-27 13:06:06 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 10 Frédéric Demians 2016-09-30 09:27:29 UTC
Pushed in 16.05. Will be in 16.05.05.