Bug 19256 - Koha::Acquisition::Order should use Koha::Object
Summary: Koha::Acquisition::Order should use Koha::Object
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:
Blocks:
 
Reported: 2017-09-05 16:06 UTC by Jonathan Druart
Modified: 2018-06-04 20:10 UTC (History)
2 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 19256: Make Koha::Acq::Order using Koha::Object (43.64 KB, patch)
2017-09-06 13:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19256: Make Koha::Acq::Order using Koha::Object (43.55 KB, patch)
2017-09-06 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19256 - Remove warn from unit test (923 bytes, patch)
2017-09-06 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19256: Make Koha::Acq::Order using Koha::Object (43.75 KB, patch)
2017-09-06 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19256: (QA followup) Remove warn from unit test (997 bytes, patch)
2017-09-06 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19256: (QA followup) Add missing POD (1.64 KB, patch)
2017-09-06 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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