Bug 23070 - Use Koha::Hold in C4::Reserves::RevertWaitingStatus
Summary: Use Koha::Hold in C4::Reserves::RevertWaitingStatus
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: rel_20_11_target
Depends on: 21944 23185 9834
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-06 20:13 UTC by Jonathan Druart
Modified: 2020-08-03 22:54 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03


Attachments
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus (2.49 KB, patch)
2019-06-06 20:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Add tests (2.45 KB, patch)
2020-01-27 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Increment all priorities in 1 query (1.08 KB, patch)
2020-01-27 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus (1.98 KB, patch)
2020-02-18 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Add tests (2.45 KB, patch)
2020-02-18 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Increment all priorities in 1 query (1.01 KB, patch)
2020-02-18 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus (2.21 KB, patch)
2020-04-29 10:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Add tests (2.47 KB, patch)
2020-04-29 10:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Increment all priorities in 1 query (1.01 KB, patch)
2020-04-29 10:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update (1.02 KB, patch)
2020-04-29 10:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update (1.02 KB, patch)
2020-05-04 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus (2.50 KB, patch)
2020-05-04 12:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23070: Add tests (2.52 KB, patch)
2020-05-04 12:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23070: Increment all priorities in 1 query (1.07 KB, patch)
2020-05-04 12:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update (1.08 KB, patch)
2020-05-04 12:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus (2.55 KB, patch)
2020-07-05 16:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23070: Add tests (2.59 KB, patch)
2020-07-05 16:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23070: Increment all priorities in 1 query (1.12 KB, patch)
2020-07-05 16:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update (1.13 KB, patch)
2020-07-05 16:28 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-06-06 20:13:57 UTC
We are using raw SQL statements, we should use Koha::Hold instead.
Comment 1 Jonathan Druart 2019-06-06 20:15:58 UTC
Created attachment 90400 [details] [review]
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus

We are using raw SQL statements, we should use Koha::Hold instead.

This patch does not seem optimal, we would like to increment priority in
only 1 statement and without the need to fetch and loop all holds.
Comment 2 Jonathan Druart 2019-06-06 20:16:51 UTC
I am missing a DBIx::Class trick to make this better.
Comment 3 Martin Renvoize 2019-06-13 16:24:43 UTC
(In reply to Jonathan Druart from comment #2)
> I am missing a DBIx::Class trick to make this better.

If we were not getting rid of being able to call dbic's 'update' routine there might be.. though even then I think you'd need to pass some raw sql via a scalar ref.. something along the lines

`Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => { '>' => 0 } })->update({ priority => \'priority + 1' });`

However, as we're keen to ensure we always go via our 'store' codepath to catch triggers then that is the equivalent to having to call ->update_all in DBIx::Class::ResultSet world.. which as that triggers a loop internally and a db hit per row rather than a db hit for the set then what you've got here seems the best we can do.

We could possibly get clever when we work on a solution for our version of update and check whether there are triggers in the local store routing and either call dbics underlying update directly or manipulate it into a loop which calls our store routines much like dbic does for update_all.
Comment 4 Jonathan Druart 2019-06-13 16:35:07 UTC
I do not think we should force developers to use store. For instance here it's not needed, we do not want to change the value for priority. The problem appears especially when we add or update from the interface.
Comment 5 Martin Renvoize 2019-06-14 07:52:18 UTC
Catch me on IRC.. I'm not sure I understood your last comment.. might just be 'early morning brain' of course.
Comment 6 Jonathan Druart 2019-06-20 01:31:52 UTC
<Joubu> About 23070, not sure I will manage to rephrase it :)                                                                                                                                       
<Joubu> ->store will check the default values (triggering Koha::Object->store), and also do some stuffs (like logging, etc.)
<Joubu> But in some cases (like the one we have on 23070), we know that it won't be necessary. The +1 query will not need the default values check or anything else, we just want to increment a value in DB.
<Joubu> That a good example to let people use ->update directly, even if it should considered as a not recommended method
<Joubu> And QA will have to be careful about that
Comment 7 Jonathan Druart 2020-01-27 13:56:21 UTC
Created attachment 97976 [details] [review]
Bug 23070: Add tests
Comment 8 Jonathan Druart 2020-01-27 13:56:25 UTC
Created attachment 97977 [details] [review]
Bug 23070: Increment all priorities in 1 query
Comment 9 Tomás Cohen Arazi 2020-01-28 02:11:26 UTC
(In reply to Martin Renvoize from comment #3)
> (In reply to Jonathan Druart from comment #2)
> > I am missing a DBIx::Class trick to make this better.
> 
> If we were not getting rid of being able to call dbic's 'update' routine
> there might be.. though even then I think you'd need to pass some raw sql
> via a scalar ref.. something along the lines
> 
> `Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => {
> '>' => 0 } })->update({ priority => \'priority + 1' });`
> 
> However, as we're keen to ensure we always go via our 'store' codepath to
> catch triggers then that is the equivalent to having to call ->update_all in
> DBIx::Class::ResultSet world.. which as that triggers a loop internally and
> a db hit per row rather than a db hit for the set then what you've got here
> seems the best we can do.
> 
> We could possibly get clever when we work on a solution for our version of
> update and check whether there are triggers in the local store routing and
> either call dbics underlying update directly or manipulate it into a loop
> which calls our store routines much like dbic does for update_all.

I'm leaning towards thinking we shouldn't use a catch-all ->store method, but an ->insert and an ->update methods... There's a reasoning behind how DBIC does things... We've gone too far with our use of ->update_or_insert and we end up having a big IF clause on the more interesting Koha::Object-derived classes, to detect if it is an update or an insert...

And I also belive many business stuffs should be on the controllers instead. We are inserting too many things in the DAO (not exactly the pattern, I know) only to be backwards compatible.

That said, this patches look correct the way our codebase is going forward.
Comment 10 Jonathan Druart 2020-02-18 10:40:39 UTC
Created attachment 99168 [details] [review]
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus

We are using raw SQL statements, we should use Koha::Hold instead.

This patch does not seem optimal, we would like to increment priority in
only 1 statement and without the need to fetch and loop all holds.
Comment 11 Jonathan Druart 2020-02-18 10:40:42 UTC
Created attachment 99169 [details] [review]
Bug 23070: Add tests
Comment 12 Jonathan Druart 2020-02-18 10:40:46 UTC
Created attachment 99170 [details] [review]
Bug 23070: Increment all priorities in 1 query
Comment 13 Jonathan Druart 2020-02-18 10:41:03 UTC
Patches rebased on top of bug 21944.
Comment 14 Victor Grousset/tuxayo 2020-04-29 04:50:54 UTC
status: ASSIGNED

Should it be needs signoff?
Comment 15 Jonathan Druart 2020-04-29 08:49:42 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> status: ASSIGNED
> 
> Should it be needs signoff?

No, if it is ASSIGNED, it's not ready yet.
Comment 16 Jonathan Druart 2020-04-29 10:22:46 UTC
Created attachment 103907 [details] [review]
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus

We are using raw SQL statements, we should use Koha::Hold instead.

This patch does not seem optimal, we would like to increment priority in
only 1 statement and without the need to fetch and loop all holds.
Comment 17 Jonathan Druart 2020-04-29 10:22:50 UTC
Created attachment 103908 [details] [review]
Bug 23070: Add tests
Comment 18 Jonathan Druart 2020-04-29 10:22:53 UTC
Created attachment 103909 [details] [review]
Bug 23070: Increment all priorities in 1 query
Comment 19 Jonathan Druart 2020-04-29 10:22:57 UTC
Created attachment 103910 [details] [review]
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update

To make sure we will update all the objects in one go (and no trigger
the ->set->store from Koha::Object->update)
Comment 20 Bernardo Gonzalez Kriegel 2020-04-29 14:17:09 UTC
Test pass, is there anything else to test?
Comment 21 Jonathan Druart 2020-04-30 13:45:32 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #20)
> Test pass, is there anything else to test?

Yes, place some holds on the same record, mark on as waiting then revert the waiting status and confirm that the priorities are recalculated correctly.
Comment 22 Victor Grousset/tuxayo 2020-05-01 14:39:05 UTC
I got an error on the tests, is there any thing else to do before running them?


#### Error:
kohadev-koha@661a25cf6198:/kohadevbox/koha$ time prove t/db_dependent/Reserves.t
t/db_dependent/Reserves.t .. 8/63 Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 558.
Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 558.
t/db_dependent/Reserves.t .. 60/63         # Looks like you planned 2 tests but ran 1.

    #   Failed test 'item level hold'
    #   at t/db_dependent/Reserves.t line 961.
    # Looks like you planned 2 tests but ran 1.
    # Looks like you failed 1 test of 1 run.

#   Failed test 'reserves.item_level_hold'
#   at t/db_dependent/Reserves.t line 992.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'no_triggers' in 'field list' [for Statement "UPDATE `reserves` SET `no_triggers` = ?, `priority` = priority + 1 WHERE ( ( `biblionumber` = ? AND `priority` > ? ) )" with ParamValues: 0=1, 1=458, 2=0] at /kohadevbox/koha/Koha/Objects.pm line 222
# Looks like your test exited with 11 just after 61.
t/db_dependent/Reserves.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 3/63 subtests 

Test Summary Report
-------------------
t/db_dependent/Reserves.t (Wstat: 2816 Tests: 61 Failed: 1)
  Failed test:  61
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 63 tests but ran 61.
Files=1, Tests=61,  6 wallclock secs ( 0.04 usr  0.01 sys +  4.78 cusr  0.72 csys =  5.55 CPU)
Result: FAIL

real	0m6.765s
user	0m4.890s
sys	0m0.749s
Comment 23 didier 2020-05-02 15:36:18 UTC
(In reply to Victor Grousset/tuxayo from comment #22)
> I got an error on the tests, is there any thing else to do before running
> them?
Look like theres's a bug should be

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 38db7d6e0e..397c33f0bd 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -1994,7 +1994,7 @@ sub RevertWaitingStatus {
     ## Increment the priority of all other non-waiting
     ## reserves for this bib record
     my $holds = Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => { '>' => 0 } })
-                           ->update({ priority => \'priority + 1', no_triggers => 1 });
+                           ->update({ priority => \'priority + 1'}, {no_triggers => 1 });
 
     ## Fix up the currently waiting reserve
     $hold->set(
Comment 24 Katrin Fischer 2020-05-03 10:31:57 UTC
Hi didier, if you spot something wrong, please don't hesitate to set "Failed QA" as this usually alerts the dev.
Comment 25 Victor Grousset/tuxayo 2020-05-03 22:07:41 UTC
Thanks Didier, I wasn't sure if I could set to "Failed QA" because my setup or way of testing could be wrong. However, you definitely found something fishy.
Comment 26 Jonathan Druart 2020-05-04 09:56:38 UTC
Created attachment 104253 [details] [review]
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update

To make sure we will update all the objects in one go (and no trigger
the ->set->store from Koha::Object->update)
Comment 27 Jonathan Druart 2020-05-04 09:57:26 UTC
A follow-up from bug 23185 modified the way we pass no_triggers.
Comment 28 Victor Grousset/tuxayo 2020-05-04 12:11:01 UTC
Created attachment 104261 [details] [review]
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus

We are using raw SQL statements, we should use Koha::Hold instead.

This patch does not seem optimal, we would like to increment priority in
only 1 statement and without the need to fetch and loop all holds.

== Test plan ==
- apply patch
- place some holds on the same record
- check that the priorities look good
- mark one hold as waiting by doing a check-in
- revert the waiting status
- confirm that the priorities are recalculated correctly

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 29 Victor Grousset/tuxayo 2020-05-04 12:11:07 UTC
Created attachment 104262 [details] [review]
Bug 23070: Add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 30 Victor Grousset/tuxayo 2020-05-04 12:11:13 UTC
Created attachment 104263 [details] [review]
Bug 23070: Increment all priorities in 1 query

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 31 Victor Grousset/tuxayo 2020-05-04 12:11:19 UTC
Created attachment 104264 [details] [review]
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update

To make sure we will update all the objects in one go (and no trigger
the ->set->store from Koha::Object->update)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 32 Victor Grousset/tuxayo 2020-05-04 12:13:20 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Bernardo Gonzalez Kriegel from comment #20)
> > Test pass, is there anything else to test?
> 
> Yes, place some holds on the same record, mark on as waiting then revert the
> waiting status and confirm that the priorities are recalculated correctly.

Test plan added to the patch based on the above.

Details of what I tried:
the holds:
patron 1 prio 1
patron 2 prio 2
patron 3 prio 3
patron 4 prio 4
patron 5 prio 5
patron 6 prio 6

check in

patron 1 waiting
patron 2 prio 1
patron 3 prio 2
patron 4 prio 3
patron 5 prio 4
patron 6 prio 5

revert

prio same a before :D

automated tests passes :D
Comment 33 Katrin Fischer 2020-07-05 16:28:37 UTC
Created attachment 106556 [details] [review]
Bug 23070: Use Koha::Hold in C4::Reserves::RevertWaitingStatus

We are using raw SQL statements, we should use Koha::Hold instead.

This patch does not seem optimal, we would like to increment priority in
only 1 statement and without the need to fetch and loop all holds.

== Test plan ==
- apply patch
- place some holds on the same record
- check that the priorities look good
- mark one hold as waiting by doing a check-in
- revert the waiting status
- confirm that the priorities are recalculated correctly

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2020-07-05 16:28:42 UTC
Created attachment 106557 [details] [review]
Bug 23070: Add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2020-07-05 16:28:47 UTC
Created attachment 106558 [details] [review]
Bug 23070: Increment all priorities in 1 query

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2020-07-05 16:28:51 UTC
Created attachment 106559 [details] [review]
Bug 23070: Pass no_triggers => 1 to Koha::Objects->update

To make sure we will update all the objects in one go (and no trigger
the ->set->store from Koha::Object->update)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Jonathan Druart 2020-07-20 15:48:00 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 38 Lucas Gass 2020-07-27 15:51:11 UTC
backported to 20.05.x for 20.05.03
Comment 39 Aleisha Amohia 2020-08-03 22:54:12 UTC
enhancement, not backported to 19.11.x