Using the DBIC relationship with aqorders and count, we can get rid of it.
Created attachment 97414 [details] [review] Bug 24430: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97415 [details] [review] Bug 24430: Add ->orders and ->orders_count to Koha::Biblio This patch introduces an accessor to the related orders, and a method that returns the orders count. The target usage for the count is the API. In the rest of the codebase, we would just call: $biblio->orders->count To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97416 [details] [review] Bug 24430: Remove CountBiblioInOrders and its traces This patch replaces the only uses of CountBiblioInOrders and makes that code use $biblio->orders->count instead. Test nothing breaks in basket.pl and parcel.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97443 [details] [review] Bug 24430: Remove CountBiblioInOrders and its traces This patch replaces the only uses of CountBiblioInOrders and makes that code use $biblio->orders->count instead. Test nothing breaks in basket.pl and parcel.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97446 [details] [review] Bug 24430: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97447 [details] [review] Bug 24430: Add ->orders and ->active_orders_count to Koha::Biblio This patch introduces an accessor to the related orders, and a method that returns the active orders count. The target usage for the count is the API. In the rest of the codebase, we would just call: $biblio->orders->count To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97448 [details] [review] Bug 24430: Remove CountBiblioInOrders and its traces This patch replaces the only uses of CountBiblioInOrders and makes that code use $biblio->orders->count instead. Test nothing breaks in basket.pl and parcel.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 97563 [details] [review] Bug 24430: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 97564 [details] [review] Bug 24430: Add ->orders and ->active_orders_count to Koha::Biblio This patch introduces an accessor to the related orders, and a method that returns the active orders count. The target usage for the count is the API. In the rest of the codebase, we would just call: $biblio->orders->count To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 97565 [details] [review] Bug 24430: Remove CountBiblioInOrders and its traces This patch replaces the only uses of CountBiblioInOrders and makes that code use $biblio->orders->count instead. Test nothing breaks in basket.pl and parcel.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 97652 [details] [review] Bug 24430: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 97653 [details] [review] Bug 24430: Add ->orders and ->active_orders_count to Koha::Biblio This patch introduces an accessor to the related orders, and a method that returns the active orders count. The target usage for the count is the API. In the rest of the codebase, we would just call: $biblio->orders->count To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 97654 [details] [review] Bug 24430: Remove CountBiblioInOrders and its traces This patch replaces the only uses of CountBiblioInOrders and makes that code use $biblio->orders->count instead. Test nothing breaks in basket.pl and parcel.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Trivial improvements that simplify code.. Passing QA
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x
A bit late, but wondering why active_orders include completed lines ? Looks a bit strange to me..
(In reply to Tomás Cohen Arazi from comment #0) > Using the DBIC relationship with aqorders and count, we can get rid of it. You could have used aqorders but added orders?
Adding the orders relation alias is forward thinking for the API here. I agree with the approach Tomas took
(In reply to Martin Renvoize from comment #19) > Adding the orders relation alias is forward thinking for the API here. I > agree with the approach Tomas took Yeah no problem. But couldnt you add an ->orders method and use the aqorders relation inside. Preventing custom relations in DBIx schema?
Not with how the API embeds get translated. The real fix is to fix our database field names and database table names to generally be consistency.. but that would upset everyone with reports.
(In reply to Marcel de Rooy from comment #17) > A bit late, but wondering why active_orders include completed lines ? Looks > a bit strange to me.. Looking at Koha::Acquisition::Orders it feels like I might have mixed 'current' vs. 'active'. So a terminology problem. And 'filter_by_active' seems to have a wrong POD (should be 'filtering OUT orders that are not active'). We could have a problem with this embed in 'biblio.active_orders+count' in /acquisitions/orders. Please file a bug! (In reply to Marcel de Rooy from comment #20) > Yeah no problem. But couldnt you add an ->orders method and use the aqorders > relation inside. Preventing custom relations in DBIx schema? We could even do it the other way around: not having any automatic relationship generated, and only define what we are gonna use!
(In reply to Tomás Cohen Arazi from comment #22) > Looking at Koha::Acquisition::Orders it feels like I might have mixed > 'current' vs. 'active'. So a terminology problem. And 'filter_by_active' > seems to have a wrong POD (should be 'filtering OUT orders that are not > active'). We could have a problem with this embed in > 'biblio.active_orders+count' in /acquisitions/orders. > > Please file a bug! Thanks for your feedback. I came here from bug 35994 but I may open yet another bug to address the 'intermediate concerns' as you mentioned.
(In reply to Tomás Cohen Arazi from comment #22) > Looking at Koha::Acquisition::Orders it feels like I might have mixed > 'current' vs. 'active'. So a terminology problem. And 'filter_by_active' > seems to have a wrong POD (should be 'filtering OUT orders that are not > active'). We could have a problem with this embed in > 'biblio.active_orders+count' in /acquisitions/orders. > > Please file a bug! This is bug 36018. Renaming the _current method there. I checked on the use of active_orders_count and it is only used in parcel.tt. This all looks okay. Do not allow order cancellation with deletion of biblio if there are multiple active orders.
(In reply to Marcel de Rooy from comment #24) > I checked on the use of active_orders_count and it is only used in > parcel.tt. This all looks okay. Do not allow order cancellation with > deletion of biblio if there are multiple active orders. Hmm. Scratch that. Continued on 36018.