We are using raw SQL statements, we should use Koha::Hold instead.
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.
I am missing a DBIx::Class trick to make this better.
(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 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.
Catch me on IRC.. I'm not sure I understood your last comment.. might just be 'early morning brain' of course.
<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
Created attachment 97976 [details] [review] Bug 23070: Add tests
Created attachment 97977 [details] [review] Bug 23070: Increment all priorities in 1 query
(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.
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.
Created attachment 99169 [details] [review] Bug 23070: Add tests
Created attachment 99170 [details] [review] Bug 23070: Increment all priorities in 1 query
Patches rebased on top of bug 21944.
status: ASSIGNED Should it be needs signoff?
(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.
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.
Created attachment 103908 [details] [review] Bug 23070: Add tests
Created attachment 103909 [details] [review] Bug 23070: Increment all priorities in 1 query
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)
Test pass, is there anything else to test?
(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.
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
(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(
Hi didier, if you spot something wrong, please don't hesitate to set "Failed QA" as this usually alerts the dev.
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.
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)
A follow-up from bug 23185 modified the way we pass no_triggers.
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>
Created attachment 104262 [details] [review] Bug 23070: Add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 104263 [details] [review] Bug 23070: Increment all priorities in 1 query Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
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>
(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
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>
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>
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>
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>
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.03
enhancement, not backported to 19.11.x