Bug 24430

Summary: Remove C4::Biblio::CountBiblioInOrders
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, kyle, liz, m.de.rooy, martin.renvoize, nick, nicolas.legrand, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35994
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36018
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 24418    
Bug Blocks: 17628, 20212, 24435, 24467, 24672    
Attachments: Bug 24430: Unit tests
Bug 24430: Add ->orders and ->orders_count to Koha::Biblio
Bug 24430: Remove CountBiblioInOrders and its traces
Bug 24430: Remove CountBiblioInOrders and its traces
Bug 24430: Unit tests
Bug 24430: Add ->orders and ->active_orders_count to Koha::Biblio
Bug 24430: Remove CountBiblioInOrders and its traces
Bug 24430: Unit tests
Bug 24430: Add ->orders and ->active_orders_count to Koha::Biblio
Bug 24430: Remove CountBiblioInOrders and its traces
Bug 24430: Unit tests
Bug 24430: Add ->orders and ->active_orders_count to Koha::Biblio
Bug 24430: Remove CountBiblioInOrders and its traces

Description Tomás Cohen Arazi 2020-01-15 19:08:47 UTC
Using the DBIC relationship with aqorders and count, we can get rid of it.
Comment 1 Tomás Cohen Arazi 2020-01-15 19:30:12 UTC
Created attachment 97414 [details] [review]
Bug 24430: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-01-15 19:30:15 UTC
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>
Comment 3 Tomás Cohen Arazi 2020-01-15 19:30:18 UTC
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>
Comment 4 Tomás Cohen Arazi 2020-01-16 14:37:52 UTC
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>
Comment 5 Tomás Cohen Arazi 2020-01-16 15:33:14 UTC
Created attachment 97446 [details] [review]
Bug 24430: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2020-01-16 15:33:17 UTC
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>
Comment 7 Tomás Cohen Arazi 2020-01-16 15:33:21 UTC
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>
Comment 8 Nick Clemens 2020-01-17 20:12:20 UTC
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>
Comment 9 Nick Clemens 2020-01-17 20:12:22 UTC
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>
Comment 10 Nick Clemens 2020-01-17 20:12:25 UTC
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>
Comment 11 Martin Renvoize 2020-01-21 11:23:14 UTC
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>
Comment 12 Martin Renvoize 2020-01-21 11:23:18 UTC
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>
Comment 13 Martin Renvoize 2020-01-21 11:23:21 UTC
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>
Comment 14 Martin Renvoize 2020-01-21 11:24:08 UTC
Trivial improvements that simplify code.. Passing QA
Comment 15 Martin Renvoize 2020-01-21 11:36:52 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 16 Joy Nelson 2020-02-06 22:29:02 UTC
enhancement not backported to 19.11.x
Comment 17 Marcel de Rooy 2024-02-06 14:43:24 UTC
A bit late, but wondering why active_orders include completed lines ? Looks a bit strange to me..
Comment 18 Marcel de Rooy 2024-02-06 14:44:01 UTC
(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?
Comment 19 Martin Renvoize 2024-02-06 14:58:32 UTC
Adding the orders relation alias is forward thinking for the API here. I agree with the approach Tomas took
Comment 20 Marcel de Rooy 2024-02-06 15:35:32 UTC
(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?
Comment 21 Martin Renvoize 2024-02-06 20:49:29 UTC
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.
Comment 22 Tomás Cohen Arazi 2024-02-07 11:33:50 UTC
(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!
Comment 23 Marcel de Rooy 2024-02-07 14:40:51 UTC
(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.
Comment 24 Marcel de Rooy 2024-02-21 13:07:04 UTC
(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.
Comment 25 Marcel de Rooy 2024-02-21 13:53:31 UTC
(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.