Bug 12830 - Move the order-related code into its own module
Summary: Move the order-related code into its own module
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 12827 12844 12953
Blocks: 12826 12896 13319
  Show dependency treegraph
 
Reported: 2014-08-27 10:42 UTC by Jonathan Druart
Modified: 2015-06-04 23:33 UTC (History)
4 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 12830: Move the order-related code into Koha::Acquisition::Order (33.22 KB, patch)
2014-08-27 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12826: use find instead of search (1.04 KB, patch)
2014-08-27 12:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: use find instead of search (1014 bytes, patch)
2014-08-28 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: Move the order-related code into Koha::Acquisition::Order (33.46 KB, patch)
2014-09-10 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: use find instead of search (1010 bytes, patch)
2014-09-10 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: Move the order-related code into Koha::Acquisition::Order (33.72 KB, patch)
2014-09-11 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: use find instead of search (1010 bytes, patch)
2014-09-11 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: Move the order-related code into Koha::Acquisition::Order (34.77 KB, patch)
2014-09-12 09:45 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 12830: use find instead of search (1.08 KB, patch)
2014-09-12 09:45 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 12830: Move the order-related code into Koha::Acquisition::Order (33.76 KB, patch)
2014-09-18 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: use find instead of search (1.04 KB, patch)
2014-09-18 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: Move the order-related code into Koha::Acquisition::Order (33.70 KB, patch)
2014-09-23 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12830: use find instead of search (1.04 KB, patch)
2014-09-23 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12830: Move the order-related code into Koha::Acquisition::Order (33.77 KB, patch)
2014-10-27 21:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12830: use find instead of search (1.21 KB, patch)
2014-10-27 21:28 UTC, Katrin Fischer
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-27 10:42:21 UTC
The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code.

I propose to create a new Koha::Acquisition::Order module and to put in it the code related to the order management.
Comment 1 Jonathan Druart 2014-08-27 10:54:54 UTC Comment hidden (obsolete)
Comment 2 Colin Campbell 2014-08-27 12:11:30 UTC
Good stuff breaking down C4::acquisition into more manageable parts is long overdue. 
When fetching the order ... ordernumber is the primary key so why not simply call 
$schema->resultset('Aqorder')->find($ordernumber);?
Comment 3 Jonathan Druart 2014-08-27 12:18:13 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-08-27 12:20:20 UTC
(In reply to Colin Campbell from comment #2)
> When fetching the order ... ordernumber is the primary key so why not simply
> call $schema->resultset('Aqorder')->find($ordernumber);?

There is no reason :)
I added a followup for that. Thanks!
Comment 5 Jonathan Druart 2014-08-28 09:07:18 UTC Comment hidden (obsolete)
Comment 6 Paola Rossi 2014-09-09 15:18:29 UTC
I've tried to apply the patch against master 3.17.00.023

Applying: Bug 12830: Move the order-related code into Koha::Acquisition::Order
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12830: Move the order-related code into Koha::Acquisition::Order

So I pass the patch to "Patch doesn't apply" status.
Comment 7 Jonathan Druart 2014-09-09 15:39:40 UTC
Paola, did you apply dependent patches?
Comment 8 Paola Rossi 2014-09-09 16:06:20 UTC
(In reply to Jonathan Druart from comment #7)
> Paola, did you apply dependent patches?

No, so I pass the patch back to "Needs Signoff"!
Comment 9 Kyle M Hall 2014-09-09 16:14:09 UTC
Just a note, DBIC's find doesn't need a key/value pair when finding on a table with a single column primary key. You can just pass in the value. That being said, what you have will work just fine.
Comment 10 Paola Rossi 2014-09-10 09:48:27 UTC
I've tried to apply the patch against 3.17.00.023.
Firstly I applied the 12827 patch.

I unfortunately see the same conflict as yesterday:

Applying: Bug 12830: Move the order-related code into Koha::Acquisition::Order
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12830: Move the order-related code into Koha::Acquisition::Order

So I pass the patch to "Patch doesn't apply" status.
Comment 11 Jonathan Druart 2014-09-10 10:53:42 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-09-10 10:53:49 UTC Comment hidden (obsolete)
Comment 13 Paola Rossi 2014-09-10 11:54:01 UTC
I've applied the patches against master 3.17.00.023

I was trying to create an order. After selecting "Save", this error appeared:

Software error:

DBIx::Class::Relationship::CascadeActions::update(): Can't update Koha::Schema::Result::Aqorder=HASH(0x5c577e8): row not found at /var/root-koha/bug-XXXX/Koha/Acquisition/Order.pm line 50

[nonetheless the order has been created]

So I pass the patch to "Failed QA" status.
Comment 14 Paola Rossi 2014-09-10 13:18:48 UTC
To complete the content of the Comment 13:

1) the software error DBIx.... occurs either testing on a DB without orders, or testing on a DB with former orders.

2) prove t/db_dependent/Acquisition/CancelReceipt.t
t/db_dependent/Acquisition/CancelReceipt.t .. 1/4 Open of share file /tmp/sharefile-koha-koha failed: Permesso negato at /usr/lib/perl5/Cache/FastMmap.pm line 640.
# Looks like you planned 4 tests but ran 1.
# Looks like your test exited with 13 just after 1.
t/db_dependent/Acquisition/CancelReceipt.t .. Dubious, test returned 13 (wstat 3328, 0xd00)
Failed 3/4 subtests 
 
Test Summary Report
-------------------
t/db_dependent/Acquisition/CancelReceipt.t (Wstat: 3328 Tests: 1 Failed: 0)
  Non-zero exit status: 13
  Parse errors: Bad plan.  You planned 4 tests but ran 1.
Files=1, Tests=1,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.91 cusr  0.08 csys =  1.01 CPU)
Result: FAIL
Comment 15 Jonathan Druart 2014-09-10 13:56:55 UTC
(In reply to Paola Rossi from comment #13)
> I've applied the patches against master 3.17.00.023
> 
> I was trying to create an order. After selecting "Save", this error appeared:
> 
> Software error:
> 
> DBIx::Class::Relationship::CascadeActions::update(): Can't update
> Koha::Schema::Result::Aqorder=HASH(0x5c577e8): row not found at
> /var/root-koha/bug-XXXX/Koha/Acquisition/Order.pm line 50

Hi Paola,
Yes this bug is introduced by bug 12626 (pushed 1 day after I submitted these patches :-/).
I proposed a patch on bug 12891, but it needs QA.
I cannot continue to rebase the patch until it is not pushed.
Will do as soon as it is pushed!

(In reply to Paola Rossi from comment #14)
> To complete the content of the Comment 13:
> 
> 1) the software error DBIx.... occurs either testing on a DB without orders,
> or testing on a DB with former orders.
> 
> 2) prove t/db_dependent/Acquisition/CancelReceipt.t
> t/db_dependent/Acquisition/CancelReceipt.t .. 1/4 Open of share file
> /tmp/sharefile-koha-koha failed: Permesso negato at
> /usr/lib/perl5/Cache/FastMmap.pm line 640.

It's not introduced by this patch, you should remove the file /tmp/sharefile-koha-koha or remove the fastmap package (libcache-fastmmap-perl).
Comment 16 Jonathan Druart 2014-09-11 09:55:25 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-09-11 09:55:29 UTC Comment hidden (obsolete)
Comment 18 Paola Rossi 2014-09-12 09:45:04 UTC Comment hidden (obsolete)
Comment 19 Paola Rossi 2014-09-12 09:45:47 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-09-18 13:12:38 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-09-18 13:12:46 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-09-23 09:51:29 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-09-23 09:51:38 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2014-10-27 21:27:59 UTC
Created attachment 32810 [details] [review]
[PASSED QA] Bug 12830: Move the order-related code into Koha::Acquisition::Order

The C4::Acquisition module should be exploded in order to add
readability and maintainability to this part of the code.

This patch is a POC, it introduces a new Koha::Acquisition::Order module and put in
it the code from NewOrder and NewOrderItem.

Test plan:
1/ Create an order, modify it, receive it, cancel the receipt.
2/ Launch the prove command on all unit tests modified by this patch and
verify that all pass.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2014-10-27 21:28:22 UTC
Created attachment 32811 [details] [review]
[PASSED QA] Bug 12830: use find instead of search

Since ordernumber is the pk, it is stupid to use search.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
All acquisition related tests and the QA script pass.
I can't find any regressions with these patches.
Comment 26 Tomás Cohen Arazi 2014-10-28 14:14:45 UTC
Patches pushed to master.

Keep the good work Jonathan!