Description
Jonathan Druart
2015-02-18 13:15:21 UTC
Created attachment 35992 [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. 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. 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. 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... 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. Could you please rebase? 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. That was an awful rebase, I hope I did not loose too much things... 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. 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! 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 Hi Josef, Yes it seems that the Contact code need a lot of work to be adapted. I give up for now. 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. 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. 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. 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. 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. Created attachment 57803 [details] [review] Bug 13726: Fix for serials/acqui-search-result.pl 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> 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> 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> 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> Created attachment 58475 [details] [review] Bug 13726 - (QA followup) Fix vendor retrieval in invoices.pl Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Pushed to master for 17.05, thanks Jonathan, Nick! 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. |