Bug 12493 - The module C4::Acquisition.pm contains subroutines which belong to C4::Contract.pm
Summary: The module C4::Acquisition.pm contains subroutines which belong to C4::Contra...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Yohann Dufour
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks: 12487 12508
  Show dependency treegraph
 
Reported: 2014-06-27 09:03 UTC by Yohann Dufour
Modified: 2015-06-04 23:33 UTC (History)
3 users (show)

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


Attachments
Bug 12493: moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm (13.96 KB, patch)
2014-06-27 14:04 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 12493: unit tests for the module C4::Contract.pm (7.53 KB, patch)
2014-06-30 08:05 UTC, Yohann Dufour
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12493: moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm (14.18 KB, patch)
2014-07-03 15:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12493: unit tests for the module C4::Contract.pm (7.63 KB, patch)
2014-07-03 15:56 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[FOLLOW UP] Bug 12493 : moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm (1.26 KB, patch)
2014-07-08 07:23 UTC, Yohann Dufour
Details | Diff | Splinter Review
[PASSED QA] Bug 12493: moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm (14.24 KB, patch)
2014-07-18 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12493: unit tests for the module C4::Contract.pm (7.70 KB, patch)
2014-07-18 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12493 : moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm (1.32 KB, patch)
2014-07-18 13:27 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Yohann Dufour 2014-06-27 09:03:26 UTC
The module C4::Acquisition.pm has two subroutines : GetContract and GetContracts which normally belong to C4::Contract.pm.
Moreover, the module C4::Contract.pm needs unit tests.
Comment 1 Yohann Dufour 2014-06-27 14:04:36 UTC Comment hidden (obsolete)
Comment 2 Yohann Dufour 2014-06-30 08:05:33 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2014-07-03 15:55:55 UTC Comment hidden (obsolete)
Comment 4 Bernardo Gonzalez Kriegel 2014-07-03 15:56:02 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2014-07-06 19:34:00 UTC
Comment on attachment 29494 [details] [review]
[SIGNED-OFF] Bug 12493: moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm

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

::: C4/Acquisition.pm
@@ +363,5 @@
>      my @rows;
>      for my $basket (@$baskets) {
>          my @orders     = GetOrders( $$basket{basketno} );
> +        my $contract   = GetContract({
> +            contractnumber => $$basket{contractnumber}

I know this line existed before, but see here for future reference:
http://wiki.koha-community.org/wiki/Coding_Guidelines#PERL5:_Disagreements

And maybe fix in a follow up?

::: C4/Contract.pm
@@ +21,1 @@
>  use strict;

When using Modern::Perl you can delete the use strict; and use warnings; lines here.
Comment 6 Katrin Fischer 2014-07-06 19:38:26 UTC
Creating a new file like Contract.pm is a good time to perltidy it - there is some mixup between tabs and spaces right at the beginning.
Comment 7 Katrin Fischer 2014-07-06 19:45:33 UTC
I started fixing this, but don't want to touch too many lines, so maybe it's better you do a follow up?
Comment 8 Yohann Dufour 2014-07-08 07:23:29 UTC Comment hidden (obsolete)
Comment 9 Yohann Dufour 2014-07-08 07:25:57 UTC
Katrin, I've tested my commit with the tools koha-qa.pl but it doesn't detect tabs or spaces, and I've not found the mixup you said.
Comment 10 Katrin Fischer 2014-07-08 07:38:41 UTC
Hi Yohann, you are right, I thought Contract.pm was new, but it isn't - sorry for that and thx for your follow-up!
Comment 11 Kyle M Hall 2014-07-18 13:25:56 UTC
Created attachment 29831 [details] [review]
[PASSED QA] Bug 12493: moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm

This patch includes:
- the subroutines GetContract and GetContracts has been moved from C4::Acquisition.pm to C4::Contract.pm and adapted for a general use
- adaptation of acqui/basket.pl, acqui/basketheader.pl, acqui/neworderempty.pl, acqui/supplier.pl and admin/aqcontract.pl
- the unit tests for the module C4::Contract.pm

Test plan:
1) Apply the patch
2) Execute the unit tests by launching:
prove t/db_dependent/Contract.t t/Acquisition/ t/db_dependent/Acquisition/ t/db_dependent/Acquisition.t
3) The command has to be a success :
t/db_dependent/Contract.t ................................. ok
t/Acquisition/CanUserManageBasket.t ....................... ok
t/Acquisition/Invoice.t ................................... ok
t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t .. ok
t/db_dependent/Acquisition/GetOrdersByBiblionumber.t ...... ok
t/db_dependent/Acquisition/Invoices.t ..................... ok
t/db_dependent/Acquisition/OrderFromSubscription.t ........ ok
t/db_dependent/Acquisition/TransferOrder.t ................ 1/11 # Transfering order to basket2
t/db_dependent/Acquisition/TransferOrder.t ................ ok
t/db_dependent/Acquisition/close_reopen_basket.t .......... ok
t/db_dependent/Acquisition.t .............................. ok
All tests successful.
Files=10, Tests=284, 15 wallclock secs ( 0.11 usr  0.02 sys + 12.88 cusr  0.77 csys = 13.78 CPU)
Result: PASS

4) Log on with a superlibrarian permission
5) Go on the page acqui/supplier.pl (Acquisitions > Button "New vendor")
6) Record a vendor with a nonzero "name"
7) Go on the page admin/aqcontract.pl (click on the "Contracts" item in the menu)
8) Click on the button "New" > "Contract" and record a new one
9) Verify the displayed data are correct about the contract
10) "Edit" the contract with different values and verify the data are updated
11) Click on "Delete" in order to delete the contract, verify the displayed data are correct but cancel the operation
12) Click on "New" > "Basket" and verify there is the created contract in field "Contract", then record a basket by selectioning the created contract
13) Verify the contract name displayed is correct
14) Record an active budget and a fund linked to this budget
15) Go on the new basket (Home > Acquisitions > Search the created vendor)
16) Click on "Add to basket" then "From a new (empty) record" and verify the displayed contract name is correct

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested with both patches applied.
Works as described following test plan, all points (I did 14 first)
All test pass
No koha-qa errors

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2014-07-18 13:26:14 UTC
Created attachment 29832 [details] [review]
[PASSED QA] Bug 12493: unit tests for the module C4::Contract.pm

This patch are the unit tests of the previous patch

Test plan :
Same as the previous patch

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comments on previous patch

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2014-07-18 13:27:07 UTC
Created attachment 29833 [details] [review]
[PASSED QA] Bug 12493 : moving the subroutines GetContract and GetContracts from C4::Acquisition.pm to C4::Contract.pm

Fix in order to respect the coding guidelines

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Unit tests pass but *ONLY* with but 12487 applied on top. These two bugs must be pushed simultaneously.
Comment 14 Tomás Cohen Arazi 2014-07-30 13:52:56 UTC
Patches pushed to master.

Thanks Yohann!