Bug 19256

Summary: Koha::Acquisition::Order should use Koha::Object
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13726
Change sponsored?: --- Patch complexity: Medium patch
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Attachments: Bug 19256: Make Koha::Acq::Order using Koha::Object
Bug 19256: Make Koha::Acq::Order using Koha::Object
Bug 19256 - Remove warn from unit test
Bug 19256: Make Koha::Acq::Order using Koha::Object
Bug 19256: (QA followup) Remove warn from unit test
Bug 19256: (QA followup) Add missing POD

Description Jonathan Druart 2017-09-05 16:06:29 UTC

    
Comment 1 Jonathan Druart 2017-09-06 13:45:31 UTC
Created attachment 66872 [details] [review]
Bug 19256: Make Koha::Acq::Order using Koha::Object

At the moment we have 2 different modules for acquisition orders:
Koha::Tmp::Order[s] and Koha::Acquisition::Order
The later has been added before the creation of Koha::Object.
Koha::Tmp::Order[s] has been created to make the TT syntax for notices
works with acquisition order data.

This patch removes the temporary packages Koha::Tmp::Order[s] and adapt
the code of Koha::Acquisition::Order[s] to be based on Koha::Object[s].

It also overloads Koha::Object->new to add the trick that was done in
Koha::Acquisition::Order->insert. This is needed because acqui/addorder.pl
is called from several places and CGI->Vars is used to retrieved order's
attributes (and so much more). To avoid regression, the easiest (but not
cleanest) way to do is to filter on aqorders column's names.
This is *not* a pattern to follow!

Test plan:
Create basket and add orders from different ways, then continue a whole
acquisition process
Comment 2 Kyle M Hall 2017-09-06 14:16:18 UTC
Created attachment 66874 [details] [review]
Bug 19256: Make Koha::Acq::Order using Koha::Object

At the moment we have 2 different modules for acquisition orders:
Koha::Tmp::Order[s] and Koha::Acquisition::Order
The later has been added before the creation of Koha::Object.
Koha::Tmp::Order[s] has been created to make the TT syntax for notices
works with acquisition order data.

This patch removes the temporary packages Koha::Tmp::Order[s] and adapt
the code of Koha::Acquisition::Order[s] to be based on Koha::Object[s].

It also overloads Koha::Object->new to add the trick that was done in
Koha::Acquisition::Order->insert. This is needed because acqui/addorder.pl
is called from several places and CGI->Vars is used to retrieved order's
attributes (and so much more). To avoid regression, the easiest (but not
cleanest) way to do is to filter on aqorders column's names.
This is *not* a pattern to follow!

Test plan:
Create basket and add orders from different ways, then continue a whole
acquisition process

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Kyle M Hall 2017-09-06 14:16:24 UTC
Created attachment 66875 [details] [review]
Bug 19256 - Remove warn from unit test

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Tomás Cohen Arazi 2017-09-06 18:48:06 UTC
Created attachment 66903 [details] [review]
Bug 19256: Make Koha::Acq::Order using Koha::Object

At the moment we have 2 different modules for acquisition orders:
Koha::Tmp::Order[s] and Koha::Acquisition::Order
The later has been added before the creation of Koha::Object.
Koha::Tmp::Order[s] has been created to make the TT syntax for notices
works with acquisition order data.

This patch removes the temporary packages Koha::Tmp::Order[s] and adapt
the code of Koha::Acquisition::Order[s] to be based on Koha::Object[s].

It also overloads Koha::Object->new to add the trick that was done in
Koha::Acquisition::Order->insert. This is needed because acqui/addorder.pl
is called from several places and CGI->Vars is used to retrieved order's
attributes (and so much more). To avoid regression, the easiest (but not
cleanest) way to do is to filter on aqorders column's names.
This is *not* a pattern to follow!

Test plan:
Create basket and add orders from different ways, then continue a whole
acquisition process

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2017-09-06 18:48:16 UTC
Created attachment 66904 [details] [review]
Bug 19256: (QA followup) Remove warn from unit test

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2017-09-06 18:48:23 UTC
Created attachment 66905 [details] [review]
Bug 19256: (QA followup) Add missing POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2017-09-06 18:49:31 UTC
The code looks correct (while it needs re-working at some point), the tests pass,
QA scripts are happy, and I noticed no regressions on the UI.
Comment 8 Jonathan Druart 2017-09-07 18:08:12 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 9 Fridolin SOMERS 2017-10-10 10:03:55 UTC
Enhancement not pushed to 17.05.x