Bug 17990 - Code to check perl module versions is buggy
Summary: Code to check perl module versions is buggy
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 17882 17887 (view as bug list)
Depends on: 17986
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-25 11:41 UTC by Jonathan Druart
Modified: 2017-02-21 21:13 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17990: Refactor Perl module versions check (12.24 KB, patch)
2017-01-25 11:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17990: Refactor Perl module versions check (12.30 KB, patch)
2017-01-26 23:20 UTC, David Cook
Details | Diff | Splinter Review
Bug 17990: Refactor Perl module versions check (12.39 KB, patch)
2017-01-27 08:18 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 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.