Bug 15242

Summary: Missing subroutine in overdue_notices.pl
Product: Koha Reporter: Nick Clemens <nick>
Component: Command-line UtilitiesAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, martin.renvoize, robin, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12933    
Bug Blocks:    
Attachments: Bug 15242 - Missing subroutine in overdue_notices.pl
Bug 15242 - Missing subroutine in overdue_notices.pl
Bug 15242 - Missing subroutine in overdue_notices.pl
[SIGNED OFF] Bug 15242 - Missing subroutine in overdue_notices.pl
Bug 15242 - Missing subroutine in overdue_notices.pl

Description Nick Clemens 2015-11-23 15:41:24 UTC
When parse_letter was renamed to parse_overdue_letter on bug 12933 an error was introduced so that overdue_notices.pl cannot find the subroutine
Comment 1 Nick Clemens 2015-11-23 15:44:44 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-11-24 07:19:25 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize 2015-11-24 09:24:37 UTC
QA Style note, we tend to import routines over adding full namespaces.. 

I'd prefer to stick to that standard unless you can come up with a good/valid reason against doing so?
Comment 4 Katrin Fischer 2015-11-24 09:32:30 UTC
We have this line at the top of the script:
  43 use C4::Overdues qw(GetFine GetOverdueMessageTransportTypes);
I think adding the new routine the same way would be more consistent.

Can you please provide a new patch?
Comment 5 Marc Véron 2015-11-24 09:58:07 UTC
(In reply to Martin Renvoize from comment #3)
> QA Style note, we tend to import routines over adding full namespaces.. 

(...)

There is one more full name space in line 323:
my @overduebranches    = C4::Overdues::GetBranchcodesWithOverdueRules();

I think this should be fixed as well to be consistent.
Ah, is it necessary that the subs are exported in C4/Overdues.pm?
Line 47 following: 
GetBranchcodesWithOverdueRules is not listed
Comment 6 Martin Renvoize 2015-11-24 10:28:19 UTC
Oh man we're inconsistent in this script :(.

Well spotted Marc, reading C4::Overdues I note it's using EXPORT instead of EXPORT_OK too :(.

Personally, I believe if a routine is intended to be used in another module it should appears in the EXPORT_OK block not the EXPORT block, and that it should be explicitly imported into the using module with a use statement.

So.. if I were being picky I'd say.. convert the EXPORT to EXPORT_OK (but this may break modules that expect to have access to the routines without explicitly importing them :( ), add the routine your found to the EXPORT_OK block also, and then clean up here to match. 

Perhaps split the work into two bugs.. one clean up and one just fixing the initial bug.. thoughts?
Comment 7 Martin Renvoize 2015-11-24 10:39:42 UTC
s/this script/these scripts/
Comment 8 Katrin Fischer 2015-11-24 11:31:35 UTC
I think best to fix the one problem identified here ... and do general cleanup separately :)
Comment 9 Marc Véron 2015-11-24 12:36:40 UTC
(In reply to Katrin Fischer from comment #8)
> I think best to fix the one problem identified here ... and do general
> cleanup separately :)

+1  :-)
Comment 10 Nick Clemens 2015-11-24 19:25:15 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2015-11-25 13:51:44 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-11-25 15:27:43 UTC
Created attachment 45144 [details] [review]
Bug 15242 - Missing subroutine in overdue_notices.pl

This patch changes the call to parse_overdues_letter to use the full
namespace to ensure the subroutine can be found.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2015-11-25 15:30:14 UTC
(In reply to Martin Renvoize from comment #3)
> QA Style note, we tend to import routines over adding full namespaces.. 
> 
> I'd prefer to stick to that standard unless you can come up with a
> good/valid reason against doing so?

And I prefer the other way :)
I personally think it adds readability and eases refactoring.
Comment 14 Tomás Cohen Arazi 2015-11-25 15:32:37 UTC
Patch pushed to master.

Thanks Nick!