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

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");

*** Bug 17882 has been marked as a duplicate of this bug. ***

*** Bug 17887 has been marked as a duplicate of this bug. ***

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>

I also ran the following tests and they all passed.

perl t/Installer_PerlModules.t
perl t/Installer_pm.t

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>

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

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 :)

Pushed to master for 17.05, thanks Jonathan!

Is this correctly classified as an enh?

(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.

This won't get ported back to 16.11.x as it is an enhancement.