Bug 12827 - NewOrder returns basketno
Summary: NewOrder returns basketno
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 12891
Blocks: 12826 12830 12953
  Show dependency treegraph
 
Reported: 2014-08-26 14:46 UTC by Jonathan Druart
Modified: 2015-06-04 23:32 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12827: NewOrder should not return basketno (18.94 KB, patch)
2014-08-27 08:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder needs more unit tests (6.44 KB, patch)
2014-08-27 08:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder should not return basketno (18.77 KB, patch)
2014-08-28 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder needs more unit tests (6.24 KB, patch)
2014-08-28 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder should not return basketno (18.77 KB, patch)
2014-09-03 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder needs more unit tests (6.24 KB, patch)
2014-09-03 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder should not return basketno (18.83 KB, patch)
2014-09-05 14:19 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12827: NewOrder needs more unit tests (6.29 KB, patch)
2014-09-05 14:19 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12827: NewOrder should not return basketno (18.85 KB, patch)
2014-09-09 17:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder needs more unit tests (6.29 KB, patch)
2014-09-09 17:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder should not return basketno (18.74 KB, patch)
2014-09-11 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12827: NewOrder needs more unit tests (6.29 KB, patch)
2014-09-11 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12827: NewOrder should not return basketno (18.78 KB, patch)
2014-09-12 14:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12827: NewOrder needs more unit tests (6.36 KB, patch)
2014-09-12 14:55 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 Jonathan Druart 2014-08-26 14:46:06 UTC
C4::Acquisition::NewOrder needs the basketno, it is useless to return it.
Comment 1 Jonathan Druart 2014-08-27 08:01:39 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-08-27 08:01:57 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-08-28 09:06:01 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-08-28 09:06:05 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-08-28 09:06:19 UTC
patches rebased.
Comment 6 Zeno Tajoli 2014-09-03 12:17:29 UTC
Testing the patch on sandbox 1 (http://pro.test1.biblibre.com/)
I receive an error when a click on Acquisition tab, link:
http://pro.test1.biblibre.com/cgi-bin/koha/acqui/acqui-home.pl

The error:

Global symbol "$rs" requires explicit package name at /home/koha/src/C4/Acquisition.pm line 1281.
Compilation failed in require at /home/koha/src/acqui/acqui-home.pl line 36.
BEGIN failed--compilation aborted at /home/koha/src/acqui/acqui-home.pl line 36.
Comment 7 Jonathan Druart 2014-09-03 12:36:15 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-09-03 12:36:19 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-09-03 12:36:55 UTC
(In reply to Zeno Tajoli from comment #6)
> The error:
> 
> Global symbol "$rs" requires explicit package name at
> /home/koha/src/C4/Acquisition.pm line 1281.
> Compilation failed in require at /home/koha/src/acqui/acqui-home.pl line 36.
> BEGIN failed--compilation aborted at /home/koha/src/acqui/acqui-home.pl line
> 36.

Sorry, I introduced this error on rebasing the patches.
Should be fixed now.
Comment 10 Zeno Tajoli 2014-09-03 14:53:17 UTC
Test with prove are OK.
prove t/db_dependent/Acquisition/NewOrder.t OK
prove t/db_dependent/Acquisition.t OK
prove t/db_dependent/ OK
prove t/ OK
prove xt/ OK
prove xt/author OK

Tommorrow (04/09/2014) test about regression on acquisition workflow.
Comment 11 sandboxes@biblibre.com 2014-09-05 14:19:26 UTC
Patch tested with a sandbox, by Zeno Tajoli <z.tajoli@cineca.it>
Comment 12 sandboxes@biblibre.com 2014-09-05 14:19:53 UTC Comment hidden (obsolete)
Comment 13 sandboxes@biblibre.com 2014-09-05 14:19:57 UTC Comment hidden (obsolete)
Comment 14 Paola Rossi 2014-09-09 16:36:30 UTC
I've tried to apply against master 3.17.00.023

There is a conflict in the tests:

Applying: Bug 12827: NewOrder should not return basketno
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Budgets.t
Auto-merging t/db_dependent/Acquisition/CancelReceipt.t
CONFLICT (content): Merge conflict in t/db_dependent/Acquisition/CancelReceipt.t
Auto-merging C4/Acquisition.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 12827: NewOrder should not return basketno

I pass the patch to "Patch doesn't apply" status.
Comment 15 Jonathan Druart 2014-09-09 17:18:59 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-09-09 17:19:05 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-09-11 09:43:46 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-09-11 09:43:52 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2014-09-12 14:54:13 UTC
Created attachment 31562 [details] [review]
[PASSED QA] Bug 12827: NewOrder should not return basketno

Since the basketno parameter is needed to insert an order, it is useless
to return it.

This patch changes the prototype for the C4::Acquisition::NewOrder
subroutine. The return value is now a scalar containing the ordernumber
created.

Test plan:
Verify there is no regression on an acquisition workflow:
1/ Create an order with several items
2/ Modify the order
3/ Receive some items
4/ Cancel the receipt
4/ Receive some items
5/ Receive all remaining items
6/ Cancel the receipt

Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2014-09-12 14:55:22 UTC
Created attachment 31563 [details] [review]
[PASSED QA] Bug 12827: NewOrder needs more unit tests

NewOrder should be more tested!

This patch moves the existing unit tests into a new file and adds some
unit tests.

Note that there is no DB field aqorders.subscription, so the test in
NewOrder can be removed.

Test plan:
  prove t/db_dependent/Acquisition/NewOrder.t
and
  prove t/db_dependent/Acquisition.t
should return green.

Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Tomás Cohen Arazi 2014-09-18 00:37:34 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 22 Jonathan Druart 2014-09-18 07:45:49 UTC
This patch missed 1 occurrence of NewOrder (introduced in master after I wrote this patch).
See bug 12953 for a fix.