Bug 15242 - Missing subroutine in overdue_notices.pl
Summary: Missing subroutine in overdue_notices.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 12933
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-23 15:41 UTC by Nick Clemens
Modified: 2016-06-21 21:40 UTC (History)
6 users (show)

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


Attachments
Bug 15242 - Missing subroutine in overdue_notices.pl (1.12 KB, patch)
2015-11-23 15:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15242 - Missing subroutine in overdue_notices.pl (1.25 KB, patch)
2015-11-24 07:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15242 - Missing subroutine in overdue_notices.pl (982 bytes, patch)
2015-11-24 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 15242 - Missing subroutine in overdue_notices.pl (1.03 KB, patch)
2015-11-25 13:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15242 - Missing subroutine in overdue_notices.pl (1.09 KB, patch)
2015-11-25 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!