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
Created attachment 14942 [details] [review] Proposed Patch Testing: Should not change functionality of searching invoices and displaying results. Refactoring is for readability and maintainability of code
Created attachment 14943 [details] [review] Revised patch The commit message mentioned cleaning the name space but that change wasnt included in the patch. Amended accordingly
Created attachment 14963 [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>
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
Created attachment 18354 [details] [review] Amended Patch Updated patch to apply on current master
Reset status, Patch should now apply.
Tested and confirmed that my signoff is still good. Setting status accordingly.
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.
(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
(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.
QA comment: All look good. No regression found. Marked as Passed QA.
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>
Pushed to master. Thanks, Colin!