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).
->fill, I meant ->fill :-D.
Created attachment 129395 [details] [review] Bug 29869: Unit tests
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
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
Created attachment 129398 [details] [review] Bug 29869: Remove C4::Reserves::ModReserveFill
Created attachment 129400 [details] [review] Bug 29869: Teach log viewer about FILL
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
Created attachment 129405 [details] [review] Bug 29869: Unit tests
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
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
Created attachment 129408 [details] [review] Bug 29869: Remove C4::Reserves::ModReserveFill
Created attachment 129409 [details] [review] Bug 29869: Teach log viewer about FILL
(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!
Created attachment 129417 [details] [review] Bug 29869: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
Created attachment 129420 [details] [review] Bug 29869: Remove C4::Reserves::ModReserveFill Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129421 [details] [review] Bug 29869: Teach log viewer about FILL Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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.
It applies cleanly on latest master.
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>
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>
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>
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>
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>
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>
> 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...
(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.
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.
(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)
Pushed to master for 22.05, thanks to everybody involved [U+1F984]