Bug 17631 - Koha::Biblio - Remove GetHolds
Summary: Koha::Biblio - Remove GetHolds
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)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 17630
Blocks: 17628 19596
  Show dependency treegraph
 
Reported: 2016-11-15 13:56 UTC by Jonathan Druart
Modified: 2018-06-04 20:10 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17631: Koha::Biblio - Remove GetHolds (6.52 KB, patch)
2016-11-15 14:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17631: Koha::Biblio - Remove GetHolds (6.59 KB, patch)
2016-11-18 13:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17631: Koha::Biblio - Remove GetHolds (6.67 KB, patch)
2016-12-02 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.