Bug 17631

Summary: Koha::Biblio - Remove GetHolds
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: josef.moravec, katrin.fischer, kyle, m.de.rooy
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17630    
Bug Blocks: 17628, 19596    
Attachments: Bug 17631: Koha::Biblio - Remove GetHolds
[SIGNED-OFF] Bug 17631: Koha::Biblio - Remove GetHolds
Bug 17631: Koha::Biblio - Remove GetHolds

Description Jonathan Druart 2016-11-15 13:56:20 UTC
C4::Biblio::GetHolds can be replaced with Koha::Biblio->holds->count
Comment 1 Jonathan Druart 2016-11-15 14:06:14 UTC
Created attachment 57492 [details] [review]
Bug 17631: Koha::Biblio - Remove GetHolds

C4::Biblio::GetHolds can be replaced with Koha::Biblio->holds->count

Test plan:
Create an order and place a hold on the biblio you have ordered.
On the basket view, you should not be able to Cancel the order and/or
delete the record
Receive the order, on the parcel page you should get the same behavior.
Comment 2 Josef Moravec 2016-11-18 13:34:10 UTC
Created attachment 57648 [details] [review]
[SIGNED-OFF] Bug 17631: Koha::Biblio - Remove GetHolds

C4::Biblio::GetHolds can be replaced with Koha::Biblio->holds->count

Test plan:
Create an order and place a hold on the biblio you have ordered.
On the basket view, you should not be able to Cancel the order and/or
delete the record
Receive the order, on the parcel page you should get the same behavior.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 3 Josef Moravec 2016-11-18 13:34:37 UTC
As I read through this patch, I have an idea - maybe silly idea ;)

But, does it make sense to you to have some method like Koha::Biblio->can_be_deleted?

Probably useful on other places like catalogue/detail for example.
Comment 4 Jonathan Druart 2016-11-18 13:58:09 UTC
(In reply to Josef Moravec from comment #3)
> As I read through this patch, I have an idea - maybe silly idea ;)
> 
> But, does it make sense to you to have some method like
> Koha::Biblio->can_be_deleted?
> 
> Probably useful on other places like catalogue/detail for example.

Yes and no, the can_be_deleted method will only check the permission of a user, I think we should only have only 1 method for this kind of simple permissions.
But have a look at Koha::Virtualshelf and search for "sub can*".
Comment 5 Marcel de Rooy 2016-12-02 08:41:19 UTC
Created attachment 57900 [details] [review]
Bug 17631: Koha::Biblio - Remove GetHolds

C4::Biblio::GetHolds can be replaced with Koha::Biblio->holds->count

Test plan:
Create an order and place a hold on the biblio you have ordered.
On the basket view, you should not be able to Cancel the order and/or
delete the record
Receive the order, on the parcel page you should get the same behavior.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2016-12-02 08:44:24 UTC
C4  0.0187
Koha::Obj  0.1387

100 calls to GetHolds versus $biblio->holds->count (with one Biblios->find).
C4 is 7 times faster.
Comment 7 Marcel de Rooy 2016-12-02 08:47:19 UTC
(In reply to Marcel de Rooy from comment #6)
> C4  0.0187
> Koha::Obj  0.1387
> 
> 100 calls to GetHolds versus $biblio->holds->count (with one Biblios->find).
> C4 is 7 times faster.

(10 holds on the same biblio record)
Comment 8 Jonathan Druart 2016-12-02 08:55:22 UTC
(In reply to Marcel de Rooy from comment #6)
> C4  0.0187
> Koha::Obj  0.1387
> 
> 100 calls to GetHolds versus $biblio->holds->count (with one Biblios->find).
> C4 is 7 times faster.

Using an ORM is obviously heavier, this will be true for all the refactoring changes. However, you need to calculate everything in a context, ie. under plack and using memcached (keeping in mind we will try to cache more things).
Comment 9 Marcel de Rooy 2016-12-02 09:14:28 UTC
(In reply to Jonathan Druart from comment #8)
> Using an ORM is obviously heavier, this will be true for all the refactoring
> changes. However, you need to calculate everything in a context, ie. under
> plack and using memcached (keeping in mind we will try to cache more things).

Sure. Just adding this for the record. Especially since you are not caching things here. (Which would probably useless in this specific case.)
Comment 10 Kyle M Hall 2016-12-16 14:54:48 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 11 Katrin Fischer 2016-12-18 20:54:41 UTC
This won't get ported back to 16.11.x as it is an enhancement.