Bug 17990

Summary: Code to check perl module versions is buggy
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: dcook, katrin.fischer, kyle, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17986    
Bug Blocks:    
Attachments: Bug 17990: Refactor Perl module versions check
Bug 17990: Refactor Perl module versions check
Bug 17990: Refactor Perl module versions check

Description Jonathan Druart 2017-01-25 11:41:14 UTC

    
Comment 1 Jonathan Druart 2017-01-25 11:45:06 UTC
Created attachment 59546 [details] [review]
Bug 17990: Refactor Perl module versions check

The code is duplicated, variable are not set ($_), code is hard to read,
not covered by tests and the subroutine has 2 completely different
behaviors depending on the presence of the "module" parameter.
No need more ti rewrite it.

Test plan:
- Use koha_perl_deps.pl with the different options (-u -m -a -i)
- Go on the about page, "Perl modules" tab

You should not see any differences from before and after this patch
Comment 2 David Cook 2017-01-26 23:07:47 UTC
After looking through the master code a bit, it seems like we never actually use this form in anything but tests:

$modules->version_info('module'=>"DBI");
Comment 3 David Cook 2017-01-26 23:17:54 UTC
*** Bug 17882 has been marked as a duplicate of this bug. ***
Comment 4 David Cook 2017-01-26 23:18:29 UTC
*** Bug 17887 has been marked as a duplicate of this bug. ***
Comment 5 David Cook 2017-01-26 23:20:40 UTC
Created attachment 59590 [details] [review]
Bug 17990: Refactor Perl module versions check

The code is duplicated, variable are not set ($_), code is hard to read,
not covered by tests and the subroutine has 2 completely different
behaviors depending on the presence of the "module" parameter.
No need more ti rewrite it.

Test plan:
- Use koha_perl_deps.pl with the different options (-u -m -a -i)
- Go on the about page, "Perl modules" tab

You should not see any differences from before and after this patch

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 6 David Cook 2017-01-26 23:21:36 UTC
I also ran the following tests and they all passed.

perl t/Installer_PerlModules.t
perl t/Installer_pm.t
Comment 7 Marcel de Rooy 2017-01-27 08:18:15 UTC
Created attachment 59601 [details] [review]
Bug 17990: Refactor Perl module versions check

The code is duplicated, variable are not set ($_), code is hard to read,
not covered by tests and the subroutine has 2 completely different
behaviors depending on the presence of the "module" parameter.
No need more ti rewrite it.

Test plan:
- Use koha_perl_deps.pl with the different options (-u -m -a -i)
- Go on the about page, "Perl modules" tab

You should not see any differences from before and after this patch

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-01-27 08:18:39 UTC
Good job, Jonathan.
ok( $dbi_is_missing || $dbi_is_upgrade || $dbi_is_current, 'DBI should either be missing, upgrade or current' );
So now we are safe :)
Passed QA
Comment 9 Jonathan Druart 2017-01-27 08:47:30 UTC
Heh yes this test looks a bit silly, but it was to make sure the returned structure was still correct.
To make it pass in every situation I needed to test all of them :)
Comment 10 Kyle M Hall 2017-02-17 12:14:42 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 11 Katrin Fischer 2017-02-19 20:26:05 UTC
Is this correctly classified as an enh?
Comment 12 Jonathan Druart 2017-02-20 07:53:41 UTC
(In reply to Katrin Fischer from comment #11)
> Is this correctly classified as an enh?

Yes, the buggy code was not triggered. So this patch should not fixed any bugs.
Comment 13 Katrin Fischer 2017-02-21 21:13:15 UTC
This won't get ported back to 16.11.x as it is an enhancement.