Bug 9505 - acqui/invoices.pl contains an unnecessary loop
Summary: acqui/invoices.pl contains an unnecessary loop
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Colin Campbell
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 10794
  Show dependency treegraph
 
Reported: 2013-01-30 10:52 UTC by Colin Campbell
Modified: 2014-05-26 21:04 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (10.02 KB, patch)
2013-01-30 11:00 UTC, Colin Campbell
Details | Diff | Splinter Review
Revised patch (10.14 KB, patch)
2013-01-30 11:10 UTC, Colin Campbell
Details | Diff | Splinter Review
bug 9505 refactor loops in invoices.pl (10.56 KB, patch)
2013-01-30 19:15 UTC, Galen Charlton
Details | Diff | Splinter Review
Amended Patch (11.47 KB, patch)
2013-05-23 16:02 UTC, Colin Campbell
Details | Diff | Splinter Review
bug 9505 refactor loops in invoices.pl (11.52 KB, patch)
2013-05-30 09:45 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 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!