Bug 9505

Summary: acqui/invoices.pl contains an unnecessary loop
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: AcquisitionsAssignee: Colin Campbell <colin.campbell>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 10794    
Attachments: Proposed Patch
Revised patch
bug 9505 refactor loops in invoices.pl
Amended Patch
bug 9505 refactor loops in invoices.pl

Description Colin Campbell 2013-01-30 10:52:58 UTC
Script retrieves a list of invoices it loops through the list to produce a new bu identical list. While the execution time wasted is trivial it obscures the purpose of the code
Comment 1 Colin Campbell 2013-01-30 11:00:05 UTC Comment hidden (obsolete)
Comment 2 Colin Campbell 2013-01-30 11:10:11 UTC Comment hidden (obsolete)
Comment 3 Galen Charlton 2013-01-30 19:15:19 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-02-28 09:23:30 UTC
Patch does not apply:
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
CONFLICT (content): Merge conflict in acqui/invoices.pl
Comment 5 Colin Campbell 2013-05-23 16:02:33 UTC Comment hidden (obsolete)
Comment 6 Colin Campbell 2013-05-23 16:04:00 UTC
Reset status, Patch should now apply.
Comment 7 Galen Charlton 2013-05-23 16:11:19 UTC
Tested and confirmed that my signoff is still good.  Setting status accordingly.
Comment 8 Jonathan Druart 2013-05-27 13:43:00 UTC
Comment on attachment 18354 [details] [review]
Amended Patch

Review of attachment 18354 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
@@ +92,4 @@
>                [% IF ( invoicenumber ) %]
>                  <li>Invoice no.: [% invoicenumber %]</li>
>                [% END %]
> +              [% IF booksellerid %]

I think this test can be removed. I don't find anywhere supplier was passed.
Or perhaps should we check the supplierid variable. It is passed when we come from the acqui/supplier.pl page.
Comment 9 Colin Campbell 2013-05-30 09:10:33 UTC
(In reply to comment #8)
> Comment on attachment 18354 [details] [review] [review]
> Amended Patch
> 
> Review of attachment 18354 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
> @@ +92,4 @@
> >                [% IF ( invoicenumber ) %]
> >                  <li>Invoice no.: [% invoicenumber %]</li>
> >                [% END %]
> > +              [% IF booksellerid %]
> 
> I think this test can be removed. I don't find anywhere supplier was passed.
> Or perhaps should we check the supplierid variable. It is passed when we
> come from the acqui/supplier.pl page.

It gets passed from invoices.pl
Comment 10 Jonathan Druart 2013-05-30 09:34:12 UTC
(In reply to comment #9)
> > I think this test can be removed. I don't find anywhere supplier was passed.
> > Or perhaps should we check the supplierid variable. It is passed when we
> > come from the acqui/supplier.pl page.
> 
> It gets passed from invoices.pl

Humm... yes :)
It looks like I forgot to read the code with the context.
Comment 11 Jonathan Druart 2013-05-30 09:44:58 UTC
QA comment:
All look good. No regression found.
Marked as Passed QA.
Comment 12 Jonathan Druart 2013-05-30 09:45:13 UTC
Created attachment 18505 [details] [review]
bug 9505 refactor loops in invoices.pl

- Remove an unnecessary loop where output just
  recreated input.
- Remove unnecessary temp variables that obscure code purpose.
- Call the variable containing invoices, invoices
  rather than anonymous and ambiguous results
  reflect namechange in template.
- Lists are passed to template as array refs;
  declare them as scalars as that is how we use them.
- No need to introduce the whole namespace of some C4
  modules for 1 routine.

Test plan:

Note that this patch should not change any visible behavior.

[1] Open the invoice search page.
[2] Verify that the list of suppliers in the drop-down
    on the search form is complete.
[3] Verify that the list of libraries in the drop-down
    on the saerch form is complete.
[4] Perform a search.  Verify that the list of invoices
    is correct.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 13 Galen Charlton 2013-05-30 14:13:56 UTC
Pushed to master.  Thanks, Colin!