Bug 13726 - Koha::Acquisition::Bookseller should use Koha::Object
Summary: Koha::Acquisition::Bookseller 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: 12896 13019 13323 13719 17562 17563 17564 17668
Blocks: 15449 16017 17691 18013 18662 19130
  Show dependency treegraph
 
Reported: 2015-02-18 13:15 UTC by Jonathan Druart
Modified: 2018-06-04 20:10 UTC (History)
6 users (show)

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


Attachments
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (96.11 KB, patch)
2015-02-18 13:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (100.33 KB, patch)
2015-03-18 11:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (97.98 KB, patch)
2016-11-07 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (100.01 KB, patch)
2016-11-07 14:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (105.27 KB, patch)
2016-11-22 15:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (110.28 KB, patch)
2016-11-28 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (112.86 KB, patch)
2016-11-28 12:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13726: Fix for serials/acqui-search-result.pl (1.26 KB, patch)
2016-11-29 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (112.93 KB, patch)
2016-11-29 09:35 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13726: Fix for serials/acqui-search-result.pl (1.32 KB, patch)
2016-11-29 09:35 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object (112.73 KB, patch)
2016-12-28 15:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13726: Fix for serials/acqui-search-result.pl (1.37 KB, patch)
2016-12-28 15:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13726 - (QA followup) Fix vendor retrieval in invoices.pl (1.04 KB, patch)
2016-12-28 15:15 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-02-18 13:15:21 UTC
Koha::Acquisition::Bookseller has been introduced by bug 12896 but no consensus on the DBIx::Class implementation was reach.
Now that we have Koha::Object (cf bug 13019), it should use it.
Comment 1 Jonathan Druart 2015-02-18 13:16:48 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-02-18 13:18:37 UTC
I did not test this patch deeply and there are certainly some bugs, but I would like to get feedback on it.
I am waiting for confirmation before continuing with Koha::Acquisition::Order and Koha::Acquisition::Contract.
Comment 3 Jonathan Druart 2015-02-18 13:21:39 UTC
Note also that a lot of code could be removed (we should pass the object directly to the template instead of all values), but I would prefer to do that in another patch.
Comment 4 Brendan Gallagher 2015-03-06 10:04:48 UTC
Auto-merging acqui/invoice.pl
CONFLICT (content): Merge conflict in acqui/invoice.pl
Auto-merging acqui/basket.pl
CONFLICT (content): Merge conflict in acqui/basket.pl
Auto-merging C4/Acquisition.pm
CONFLICT (content): Merge conflict in C4/Acquisition.pm


After applying 13719...
Comment 5 Jonathan Druart 2015-03-18 11:04:53 UTC
Created attachment 36977 [details] [review]
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller's contracts data.
Comment 6 Josef Moravec 2015-12-16 14:10:41 UTC
Could you please rebase?
Comment 7 Jonathan Druart 2015-12-17 10:37:20 UTC
Not yet :)
This one depends on bug 13323.
Comment 8 Josef Moravec 2016-11-03 11:44:39 UTC
Bug 13323 is in master, could you rebase now?
Comment 9 Jonathan Druart 2016-11-07 11:28:43 UTC
Created attachment 57253 [details] [review]
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller's contracts data.
Comment 10 Jonathan Druart 2016-11-07 11:29:40 UTC
That was an awful rebase, I hope I did not loose too much things...
Comment 11 Jonathan Druart 2016-11-07 14:39:34 UTC
Created attachment 57263 [details] [review]
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller's contracts data.
Comment 12 Josef Moravec 2016-11-08 12:10:49 UTC
Thanks for rebasing!

I started to test and here are first comments

When receiving shipment (script parcel.pl) I got this error:
"The method listinct is not covered by tests!"

In tests:
t/db_dependent/Bookseller.t fails with Can't locate object method "new" via package "Koha::Acquisition::Bookseller::Contact" (perhaps you forgot to load "Koha::Acquisition::Bookseller::Contact"?) at t/db_dependent/Bookseller.t line 686.

t/db_dependent/Contract.t fails with Can't locate object method "new" via package "Koha::Acquisition::Bookseller" (perhaps you forgot to load "Koha::Acquisition::Bookseller"?) at t/db_dependent/Contract.t line 42.

t/db_dependent/Letters.t fails with 
"my" variable $bookseller masks earlier declaration in same scope at t/db_dependent/Letters.t line 418.
and
Can't locate object method "new" via package "Koha::Acquisition::Bookseller::Contact" (perhaps you forgot to load "Koha::Acquisition::Bookseller::Contact"?) at t/db_dependent/Letters.t line 379.

All other Acquisition test are passing - I am testing on top of 17562, 17563 and 17564

I tried all related (I hope) tests, and in ui I tested Vendor, Contracts, Baskets adding to order, and so on... I will continue after the error in parcel.pl will be fixed, thanks!
Comment 13 Josef Moravec 2016-11-08 12:21:51 UTC
koha-qa comments:

 FAIL	Koha/Acquisition/Bookseller/Contact.pm
   FAIL	  forbidden patterns
		forbidden pattern: Warning: The 'sub type' may be wrong is declared in a Koha::* package (see bug 15446) (line 9)

 FAIL	Koha/Acquisition/Bookseller/Contacts.pm
   FAIL	  forbidden patterns
		forbidden pattern: Warning: The 'sub type' may be wrong is declared in a Koha::* package (see bug 15446) (line 9)

 FAIL	t/db_dependent/Letters.t
   FAIL	  valid
		"my" variable $bookseller masks earlier declaration in same scope
Comment 14 Jonathan Druart 2016-11-08 14:33:14 UTC
Hi Josef, Yes it seems that the Contact code need a lot of work to be adapted.
I give up for now.
Comment 15 Jonathan Druart 2016-11-22 15:36:04 UTC
Created attachment 57714 [details] [review]
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller' contacts data.
Comment 16 Jonathan Druart 2016-11-28 09:24:18 UTC
Created attachment 57785 [details] [review]
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller' contacts data.
Comment 17 Josef Moravec 2016-11-28 11:40:29 UTC
I faced some ISE during testing (plack on):

Supplier.pl:
Can't locate object method "search" via package "Koha::Subscriptions" (perhaps you forgot to load "Koha::Subscriptions"?) at /home/vagrant/kohaclone/Koha/Acquisition/Bookseller.pm line 25.


Invoices.pl op=do_search:
Can't use string ("Koha::Acquisition::Bookseller") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/Koha/Object.pm line 219.
Comment 18 Jonathan Druart 2016-11-28 12:11:16 UTC
Created attachment 57792 [details] [review]
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller' contacts data.
Comment 19 Josef Moravec 2016-11-29 08:44:23 UTC
Very close now I think.

Just one issue I discovered now - when adding a serial subscription, I tried to search for a vendor: search is working, but no vendor name is shown in results table and vendor id is not transferred to new subscription form when clicking on Choose button.
Comment 20 Jonathan Druart 2016-11-29 09:17:24 UTC
Created attachment 57803 [details] [review]
Bug 13726: Fix for serials/acqui-search-result.pl
Comment 21 Josef Moravec 2016-11-29 09:35:26 UTC
Created attachment 57804 [details] [review]
[SIGNED-OFF] Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller' contacts data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 22 Josef Moravec 2016-11-29 09:35:32 UTC
Created attachment 57805 [details] [review]
[SIGNED-OFF] Bug 13726: Fix for serials/acqui-search-result.pl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 23 Nick Clemens 2016-12-28 15:15:27 UTC
Created attachment 58473 [details] [review]
Bug 13726: Make Koha::Acq::Bookseller using Koha::Object

This patch create a Koha::Acquisition::Booksellers module and
Koha::Acquisition::Bookseller::Contract[s] modules.

All code in the acquisition module is adapted to use the CRUD methods of
Koha::Object[s].
The former C4 routines are removed.

Test plan:
Since a lot of files are impacted by this patch, try a complete
acquisition workflow and try to catch errors.
Be focused on bookseller and bookseller' contacts data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2016-12-28 15:15:35 UTC
Created attachment 58474 [details] [review]
Bug 13726: Fix for serials/acqui-search-result.pl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2016-12-28 15:15:42 UTC
Created attachment 58475 [details] [review]
Bug 13726 - (QA followup) Fix vendor retrieval in invoices.pl

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Kyle M Hall 2016-12-30 11:54:43 UTC
Pushed to master for 17.05, thanks Jonathan, Nick!
Comment 27 Katrin Fischer 2017-01-04 21:58:39 UTC
This appears to dangerous for stable being part of the API rewrite - changing it to enh. As enh this won't be ported back to 16.11.