[Bug 23185] New: Koha::Objects supports passing through 'update' which means we can side step 'set' + 'store'
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Bug ID: 23185 Summary: Koha::Objects supports passing through 'update' which means we can side step 'set' + 'store' Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: critical Priority: P1 - high Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: dcook@prosentient.com.au, jonathan.druart@bugs.koha-community.org, koha-bugs@lists.koha-community.org, kyle@bywatersolutions.com, m.de.rooy@rijksmuseum.nl, martin.renvoize@ptfs-europe.com, testopia@bugs.koha-community.org, tomascohen@gmail.com Depends on: 21761 Target Milestone: --- +++ This bug was initially created as a clone of Bug #21761 +++ If we're adding triggers at the code level all over the place by overloading the store method we need to never ever let dbic handle it.. by allowing the 'update' method through we are exposing ourselves to places in the codebase skipping the store triggers. This would in effect bind us to the 'update_all' style of update call from dbic, which results in a loop over the affected rows so that in code triggers can be called per row rather than allowing any form of 'update all these rows to X' in one query. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 [Bug 21761] Koha::Object supports passing through 'update' which means we can side step 'set' + 'store' -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23187 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23187 [Bug 23187] Koha::Objects supports find_or_create without passing through the Koha::Object store method -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Note the 's' in the bug title.. this is not the same as 'Koha::Object' -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23070 -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- See further discussion in bug 23070 -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- My personal feeling is that perhaps we should allow this one through but add a QA script warning to check that 'store' for the object in question doesn't have any triggers in place for the fields of the set being updated in any changes or additions to the update call in question. -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I also think it wouldn't be a bad idea to have an 'update_with_triggers' or something along those lines method which in effect did a dbic update_all call(https://metacpan.org/pod/DBIx::Class::ResultSet#update_all), i.e. calling set + store one each Koha::Object in a loop from the Koha::Objects set. -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|critical |major -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I almost added similar comments on 21761 until I spotted this one. Obviously, implementing update on a larger resultset by looping and storing individually will be a performance thing. Wouldnt it be beneficial to pass to DBIx if there is no specific store method in the corresponding Koha object ? -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_20_05_target, | |RM_priority -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Bug 23185 depends on bug 21761, which changed state. Bug 21761 Summary: Koha::Object supports passing through 'update' which means we can side step 'set' + 'store' https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25303 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- More and more I feel like this needs to be caught at the QA level.. whenever we see the 'store' method being changed/created in a Koha::Thing class we should warn the QA person via the qa scripts to check for a corresponding 'update' method in the Koha::Things which just loops over the resultset calling the store on each result. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |jonathan.druart@bugs.koha-c |ity.org |ommunity.org Status|NEW |ASSIGNED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |25303 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 [Bug 25303] Koha::Objects->delete should not skip overridden object class ->delete -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Agreed with Jonathan that we should default Koha::Objects->update to loop through the resultset and call Koha::Object->store.. However, we should also allow for a direct_update option either by adding a direct_update method to Koha::Objects or by checking for a parameter passed to Koha::Objects->update. Examples: Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => { '>' => 0 } })->update({ priority => \'priority + 1' }, { skip_triggers => 1}); OR Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => { '>' => 0 } })->direct_update({ priority => \'priority + 1' }); -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23070 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23070 [Bug 23070] Use Koha::Hold in C4::Reserves::RevertWaitingStatus -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Is adding direct_update not adding the side step that you wanted to catch ? -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Marcel de Rooy from comment #8)
Is adding direct_update not adding the sidestep that you wanted to catch ?
It's making the side step very explicit.. there are certainly times when you want to have a direct access call.. as per the example above ;) where we're sure that we are only updating one field which otherwise has no triggers in store. -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103867 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103867&action=edit Bug 23185: Add tests -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103868 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103868&action=edit Bug 23185: Make Koha::Objects->update loop on the object set if needed -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Does this make sense? -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Needs Signoff |Failed QA --- Comment #13 from Victor Grousset/tuxayo <victor@tuxayo.net> --- I suppose the test plan is to run the automated tests right? It's stuck since 32 min I had to kill the process. root@e0f73671e924:koha((5724c66959...))$ kshell kohadev-koha@e0f73671e924:/kohadevbox/koha$ time prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Objects.t .. 18/22 It's infinite looping on something. PID USER PRI NI VIRT RES SHR S CPU% MEM% TIME+ Command 2549 kohadev-k 20 0 899M 687M 14736 R 75.4 5.9 23:38.44 /usr/bin/perl t/db_dependent/Koha/Objects.t I reverted to master and ran the test again and it worked. kohadev-koha@e0f73671e924:/kohadevbox/koha$ time prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Objects.t .. ok All tests successful. Files=1, Tests=21, 21 wallclock secs ( 0.06 usr 0.01 sys + 15.54 cusr 4.21 csys = 19.82 CPU) Result: PASS real 0m21.111s -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I passes for me, in ~30sec -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- It passes for me, in ~30sec -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bgkriegel@gmail.com --- Comment #16 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- (In reply to Jonathan Druart from comment #15)
It passes for me, in ~30sec
Same problem as Victor, need to kill it... :( $ time prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Objects.t .. 18/22 ^C real 16m31,000s user 0m0,208s sys 0m0,026s -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Ho right, that's the tests from bug 25303. Something weird is going on here. -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #17)
Ho right, that's the tests from bug 25303. Something weird is going on here.
Fixed with a follow-up on bug 25303. -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103867|0 |1 is obsolete| | --- Comment #19 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Created attachment 103946 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103946&action=edit Bug 23185: Add tests Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Tested on top of 25303 w/new follow-up Test pass in ~45s, no errors -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103868|0 |1 is obsolete| | --- Comment #20 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Created attachment 103947 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103947&action=edit Bug 23185: Make Koha::Objects->update loop on the object set if needed Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 103947 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103947 Bug 23185: Make Koha::Objects->update loop on the object set if needed Review of attachment 103947: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=23185&attachment=103947) ----------------------------------------------------------------- I'm open to opinions on the below comment but I feel it's clearer to keep the two things distinct. ::: Koha/Objects.pm @@ +199,5 @@
+ +sub update { + my ($self, $params) = @_; + + my $no_triggers = delete $params->{no_triggers};
Personally, I think I'd rather keep method options distinct from the row data.. so I would have prefered my ($self, $params, $options) = @_; my $no_triggers = $options->{no_triggers}; -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Generally.. I really like this however :) -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103946|0 |1 is obsolete| | --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 104087 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104087&action=edit Bug 23185: Add tests Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103947|0 |1 is obsolete| | --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 104088 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104088&action=edit Bug 23185: Make Koha::Objects->update loop on the object set if needed Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 104089 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104089&action=edit Bug 23185: (QA follow-up) Semantics, split fields and options This patch improves the semantics of the update routine to more clearly separate the 'fields' we're modifying from the 'options' we wish to apply to the modification. -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23187 -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|23187 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23187 [Bug 23187] Koha::Objects supports find_or_create without passing through the Koha::Object store method -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|25303 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 [Bug 25303] Koha::Objects->delete should not skip overridden object class ->delete -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104087|0 |1 is obsolete| | --- Comment #26 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 104232 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104232&action=edit Bug 23185: Add tests Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104088|0 |1 is obsolete| | --- Comment #27 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 104233 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104233&action=edit Bug 23185: Make Koha::Objects->update loop on the object set if needed Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104089|0 |1 is obsolete| | --- Comment #28 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 104234 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104234&action=edit Bug 23185: (QA follow-up) Semantics, split fields and options This patch improves the semantics of the update routine to more clearly separate the 'fields' we're modifying from the 'options' we wish to apply to the modification. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #29 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Works! Signed off. Assuming the test plan was only to run the tests. -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Victor Grousset/tuxayo from comment #29)
Works! Signed off. Assuming the test plan was only to run the tests.
Thanks Victor. When it's a "structural change", and especially if there is no test plan or test plan is simply "run the tests", then it means it's always better to have a look at the code, understand it, and confirm you agree with it. -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #31 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Jonathan Druart from comment #30)
When it's a "structural change", and especially if there is no test plan or test plan is simply "run the tests", then it means it's always better to have a look at the code, understand it, and confirm you agree with it.
Thanks, noted. I can't competently agree with much changes in general but experience will change that :) -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #32 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I've been following this and the outcome is really good. Everything looks correct including the QA scripts. -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104232|0 |1 is obsolete| | -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104233|0 |1 is obsolete| | -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104234|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #33 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104271 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104271&action=edit Bug 23185: Add tests Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #34 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104272 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104272&action=edit Bug 23185: Make Koha::Objects->update loop on the object set if needed Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #35 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104273 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104273&action=edit Bug 23185: (QA follow-up) Semantics, split fields and options This patch improves the semantics of the update routine to more clearly separate the 'fields' we're modifying from the 'options' we wish to apply to the modification. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #36 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104276 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104276&action=edit Bug 23185: (QA follow-up) Add POD for Koha::Objects->update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_05_candidate, | |RM_priority | -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #38 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104537 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104537&action=edit Bug 23185: ->update can only be called on instantiated classes Since our use ok Koha::Objects, there's been an implicit instantiation happening in ->search that has somehow hidden the fact that classes need instantiation... This change in ->delete and ->update made it clear because of the failures/hangs on the tests. This patch instantiates Koha::Patron::Categories before attempting to call ->update. The original patches for this bug are correct and the only issue here is that we need to document better than for some methods, instantiation is not implicit and is required first. This is awkward and I would prefer to force everyone to call ->new first. But I understand it would be inconvenient. To test: 1. Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Categories.t => FAIL: Notice it hangs, Ctrl+c to abort 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests dong hang, and in fact pass 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #39 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104538 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104538&action=edit Bug 23185: Realistic POD for Koha::Objects->search Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #40 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104541 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104541&action=edit Bug 23185: (follow-up) K:C:Register->make_default should use no_triggers Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #41 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I leave the last patch for you to review, Martin and Jonathan. I took the liberty to push the others as they were breaking Jenkins. -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #42 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Tomas, I feel like we should explode if Koha::Objects->update is called: test ref($self) at the beginning. What do you think? -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #43 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #42)
Tomas, I feel like we should explode if Koha::Objects->update is called: test ref($self) at the beginning. What do you think?
I agree. -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #44 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #41)
I leave the last patch for you to review, Martin and Jonathan. I took the liberty to push the others as they were breaking Jenkins.
Do we agree on that patch? -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |25423 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25423 [Bug 25423] Methods update and empty from Koha::Objects are not class methods -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 --- Comment #45 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Yes, pushed to master for 20.05 -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com --- Comment #46 from Joy Nelson <joy@bywatersolutions.com> --- missing dependencies not backported to 19.11.x -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are on the CC list for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes. You are on the CC list for the bug.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org