Bug 29869

Summary: Add Koha::Hold->fill
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, jonathan.druart, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on:    
Bug Blocks: 17728, 29891, 29868, 29874, 33791    
Attachments: Bug 29869: Unit tests
Bug 29869: Add Koha::Hold->fill
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill
Bug 29869: Remove C4::Reserves::ModReserveFill
Bug 29869: Teach log viewer about FILL
Bug 29869: Unit tests
Bug 29869: Add Koha::Hold->fill
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill
Bug 29869: Remove C4::Reserves::ModReserveFill
Bug 29869: Teach log viewer about FILL
Bug 29869: Unit tests
Bug 29869: Add Koha::Hold->fill
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill
Bug 29869: Remove C4::Reserves::ModReserveFill
Bug 29869: Teach log viewer about FILL
Bug 29869: Unit tests
Bug 29869: Add Koha::Hold->fill
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill
Bug 29869: Remove C4::Reserves::ModReserveFill
Bug 29869: Teach log viewer about FILL
Bug 29869: (QA follow-up) import t::lib::Mocks for Hold.t for mock_preference

Description Tomás Cohen Arazi 2022-01-12 20:43:16 UTC
This method will overtake C4::Reserves::ModReserveFill.

It will store 'FULFILL' in the action_logs table as well (to solve the FIXME that cna be found in the old method).
Comment 1 Tomás Cohen Arazi 2022-01-13 11:33:18 UTC
->fill, I meant ->fill :-D.
Comment 2 Tomás Cohen Arazi 2022-01-13 12:53:11 UTC
Created attachment 129395 [details] [review]
Bug 29869: Unit tests
Comment 3 Tomás Cohen Arazi 2022-01-13 12:53:16 UTC
Created attachment 129396 [details] [review]
Bug 29869: Add Koha::Hold->fill

This patch introduces a new method for marking a hold as filled. The
code is the result of tracking the following methods in C4::Reserves:
- ModReserveFill
- GetReserveFee
- ChargeReserveFee

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 4 Tomás Cohen Arazi 2022-01-13 12:53:20 UTC
Created attachment 129397 [details] [review]
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill

Before diving into removing ModReserveFill, I propose this:

In order to perform real-life testing of the new Koha::Hold->fill
method, this patch makes the ModReserveFill method, just call the new
one.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/Reserves* \
           t/db_dependent/Circulation* \
           t/db_dependent/SIP/*
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 5 Tomás Cohen Arazi 2022-01-13 12:53:24 UTC
Created attachment 129398 [details] [review]
Bug 29869: Remove C4::Reserves::ModReserveFill
Comment 6 Tomás Cohen Arazi 2022-01-13 13:04:20 UTC
Created attachment 129400 [details] [review]
Bug 29869: Teach log viewer about FILL
Comment 7 Martin Renvoize 2022-01-13 13:37:35 UTC
All looks solid to me, and I like that you specify the log action more clearly.

Unfortunately, the unit tests patch wont apply for me.. sha1 issues.

Otherwise I'm happy to SO
Comment 8 Tomás Cohen Arazi 2022-01-13 14:51:04 UTC
Created attachment 129405 [details] [review]
Bug 29869: Unit tests
Comment 9 Tomás Cohen Arazi 2022-01-13 14:51:09 UTC
Created attachment 129406 [details] [review]
Bug 29869: Add Koha::Hold->fill

This patch introduces a new method for marking a hold as filled. The
code is the result of tracking the following methods in C4::Reserves:
- ModReserveFill
- GetReserveFee
- ChargeReserveFee

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 10 Tomás Cohen Arazi 2022-01-13 14:51:14 UTC
Created attachment 129407 [details] [review]
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill

Before diving into removing ModReserveFill, I propose this:

In order to perform real-life testing of the new Koha::Hold->fill
method, this patch makes the ModReserveFill method, just call the new
one.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/Reserves* \
           t/db_dependent/Circulation* \
           t/db_dependent/SIP/*
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 11 Tomás Cohen Arazi 2022-01-13 14:51:20 UTC
Created attachment 129408 [details] [review]
Bug 29869: Remove C4::Reserves::ModReserveFill
Comment 12 Tomás Cohen Arazi 2022-01-13 14:51:24 UTC
Created attachment 129409 [details] [review]
Bug 29869: Teach log viewer about FILL
Comment 13 Tomás Cohen Arazi 2022-01-13 14:58:47 UTC
(In reply to Martin Renvoize from comment #7)
> All looks solid to me, and I like that you specify the log action more
> clearly.
> 
> Unfortunately, the unit tests patch wont apply for me.. sha1 issues.
> 
> Otherwise I'm happy to SO

Reordered the whole tree. Sorry for that, and thanks for looking at this!
Comment 14 Martin Renvoize 2022-01-13 16:17:52 UTC
Created attachment 129417 [details] [review]
Bug 29869: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-01-13 16:17:56 UTC
Created attachment 129418 [details] [review]
Bug 29869: Add Koha::Hold->fill

This patch introduces a new method for marking a hold as filled. The
code is the result of tracking the following methods in C4::Reserves:
- ModReserveFill
- GetReserveFee
- ChargeReserveFee

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-01-13 16:18:00 UTC
Created attachment 129419 [details] [review]
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill

Before diving into removing ModReserveFill, I propose this:

In order to perform real-life testing of the new Koha::Hold->fill
method, this patch makes the ModReserveFill method, just call the new
one.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/Reserves* \
           t/db_dependent/Circulation* \
           t/db_dependent/SIP/*
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-01-13 16:18:04 UTC
Created attachment 129420 [details] [review]
Bug 29869: Remove C4::Reserves::ModReserveFill

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-01-13 16:18:08 UTC
Created attachment 129421 [details] [review]
Bug 29869: Teach log viewer about FILL

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2022-01-13 16:22:47 UTC
Great to see unit tests, I like the method you've used, adding tests, replacing the internals of the original method, then finally removing the original method.. it meant I could run tests at each point before applying subsequent patches.

All works well at every stage.. feeling confident

Signing off.
Comment 20 Tomás Cohen Arazi 2022-01-14 11:41:59 UTC
It applies cleanly on latest master.
Comment 21 Kyle M Hall 2022-01-14 17:12:15 UTC
Created attachment 129502 [details] [review]
Bug 29869: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2022-01-14 17:13:03 UTC
Created attachment 129503 [details] [review]
Bug 29869: Add Koha::Hold->fill

This patch introduces a new method for marking a hold as filled. The
code is the result of tracking the following methods in C4::Reserves:
- ModReserveFill
- GetReserveFee
- ChargeReserveFee

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2022-01-14 17:13:07 UTC
Created attachment 129504 [details] [review]
Bug 29869: Make ModReserveFill a (temporary) wrapper for Koha::Hold->fill

Before diving into removing ModReserveFill, I propose this:

In order to perform real-life testing of the new Koha::Hold->fill
method, this patch makes the ModReserveFill method, just call the new
one.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/Reserves* \
           t/db_dependent/Circulation* \
           t/db_dependent/SIP/*
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2022-01-14 17:13:10 UTC
Created attachment 129505 [details] [review]
Bug 29869: Remove C4::Reserves::ModReserveFill

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2022-01-14 17:13:13 UTC
Created attachment 129506 [details] [review]
Bug 29869: Teach log viewer about FILL

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2022-01-14 17:13:17 UTC
Created attachment 129507 [details] [review]
Bug 29869: (QA follow-up) import t::lib::Mocks for Hold.t for mock_preference

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Jonathan Druart 2022-01-15 05:32:09 UTC
> C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber });
	
 
Calling a C4 method from Koha is bad. Calling a private method from outside the module is very bad. Calling a private C4 method from Koha is very very bad...
Comment 28 Tomás Cohen Arazi 2022-01-15 11:38:21 UTC
(In reply to Jonathan Druart from comment #27)
> > C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber });
> 	
>  
> Calling a C4 method from Koha is bad. Calling a private method from outside
> the module is very bad. Calling a private C4 method from Koha is very very
> bad...

I copied what was done in ->cancel. Moving _FixPriority to Koha::* is a good thing to do.
Comment 29 Martin Renvoize 2022-01-31 12:53:12 UTC
I'm with Tomas here, whilst it would be nice to move _FixPriority, I think it's well tested and we should focus on modernising and moving the functions that use it first.. and then move/modernise that last as it's own step.

It's a big block of code and I feel that perhaps some of it can/should be updated to be a trigger on the Koha::Hold->store method eventually.. but I think splitting it like that at this point prior to the rest of our methods moving is actually more of a risk.

Lets march forward and keep the momentum going on this.
Comment 30 Tomás Cohen Arazi 2022-01-31 14:27:43 UTC
(In reply to Martin Renvoize from comment #29)
> I'm with Tomas here, whilst it would be nice to move _FixPriority, I think
> it's well tested and we should focus on modernising and moving the functions
> that use it first.. and then move/modernise that last as it's own step.
> 
> It's a big block of code and I feel that perhaps some of it can/should be
> updated to be a trigger on the Koha::Hold->store method eventually.. but I
> think splitting it like that at this point prior to the rest of our methods
> moving is actually more of a risk.
> 
> Lets march forward and keep the momentum going on this.

I'm already looking at _FixPriorities. It looks like it does too much (cancelling a hold?) and can be simplified a lot. My perception on writing Koha::Hold->fill, is that we will eventually remove things from controllers and, for example, we could end-up passing the Koha::Item to this new method.

I'm pondering (for bug 29891):

- Koha::Biblio->fix_holds_priorities, that would call $self->holds->unfilled->fix_priorities
- Koha::Holds->fix_priorities (that acts on the resultset)
Comment 31 Fridolin Somers 2022-02-11 00:38:13 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄