[Bug 29906] New: When changing hold parameters over API (PUT) it forcibly gets to "suspended" state
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Bug ID: 29906 Summary: When changing hold parameters over API (PUT) it forcibly gets to "suspended" state Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: stalkernoid@gmail.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com This change: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127244&action=diff - my $suspended_until = exists $body->{suspended_until} ? $body->{suspended_until} : $hold->suspend_until; + my $suspended_until = + exists $body->{suspended_until} + ? dt_from_string( $body->{suspended_until}, 'rfc3339' ) + : dt_from_string( $hold->suspend_until, 'iso' ); Breaks passing "undef" in $hold->suspend_until as well as "undef" in $body->{suspended_until} to: my $params = { ... suspend_until => $suspended_until and always comes with date, it has "today date" instead of "udef" because: sub dt_from_string has: return DateTime->now( time_zone => $tz ) unless $date_string; so as a result when you are changing any hold location (for example) from API, it also puts that hold to be suspended. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal Depends on| |24850 Assignee|koha-bugs@lists.koha-commun |stalkernoid@gmail.com |ity.org | CC| |nugged@gmail.com Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24850 [Bug 24850] Koha::DateUtils ignores offsets in RFC3339 datetimes -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #1 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 129619 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129619&action=edit Bug 29906: fix the forcibly gets to "suspended" state problem which passes "perl false" to $suspended_until as it is, but in case of "perl trush" it calls dt_from_string -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129619|0 |1 is obsolete| | --- Comment #2 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 129621 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129621&action=edit Bug 29906: fix the forcibly gets to "suspended" state problem which passes "perl false" to $suspended_until as it is, but in case of "perl truth" it calls dt_from_string. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129621|0 |1 is obsolete| | --- Comment #3 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 129622 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129622&action=edit Bug 29906: fix the forcibly gets to "suspended" state problem which passes "perl false" to $suspended_until as it is, but in case of "perl truth" it calls dt_from_string. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129622|0 |1 is obsolete| | --- Comment #4 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 129626 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129626&action=edit Bug 29906: fix the forcibly gets to "suspended" state problem which passes "perl false" to $suspended_until as it is, but in case of "perl truth" it calls dt_from_string. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Please detail, it does not work for me. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129645 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129645&action=edit Bug 29906: Add test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Test is failing, I am getting a 400 {"errors":[{"message":"Expected string - got null.","path":"\/body\/suspended_until"}],"status":400} at /kohadevbox/koha/t/lib/Mojo.pm line 8. Looks like we are not supposed to pass undef. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #8 from Andrew Nugged <nugged@gmail.com> --- it doesn't mean undef explicitly in parameters, but it was meant those two vars to BE undef on the moment when calling: my $params = { ... suspend_until => $suspended_until as I understand this:
Breaks passing "undef" in $hold->suspend_until as well as "undef" in $body->{suspended_until}
this means when on the beginning before this code: my $suspended_until = ... $hold->suspend_until will return undef, or $body->{suspended_until} will be absent at all, so be undef in place, then this code: + my $suspended_until = + exists $body->{suspended_until} + ? dt_from_string( $body->{suspended_until}, 'rfc3339' ) + : dt_from_string( $hold->suspend_until, 'iso' ); won't allow for $suspended_until TO BECOME UNDEF, it will always have the date, and unwanted "TODAY" because when you pass undef to dt_from_string, as it is in: ... return DateTime->now( time_zone => $tz ) unless $date_string; thus that will make any call to this API endpoint to suspend hold. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #9 from Andrew Nugged <nugged@gmail.com> --- Let me update the test you provided to show how that will happen... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129626|0 |1 is obsolete| | Attachment #129645|0 |1 is obsolete| | --- Comment #10 from Andrew Nugged <nugged@gmail.com> --- Created attachment 129650 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129650&action=edit Bug 29906: fix when hold record forcibly gets unwanted "suspended" state This code allows for "Perl false" to pass through to $suspended_until if it is in $hold->suspend_until and in $body->{suspended_until} But in case of "Perl truth" it calls dt_from_string for that value. Reproduction: 1. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 2. see the test fails with something like: # Failed test 'Location change shouldn't touch suspended status' # at t/db_dependent/api/v1/holds.t line 1067. # got: '1' # expected: '0' # Failed test 'suspended_until should be undef' # at t/db_dependent/api/v1/holds.t line 1068. # got: '2022-01-20 00:00:00' # expected: undef # Looks like you failed 2 tests of 39. 3. apply the changes in this patch to Koha/REST/V1/Holds.pm 4. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 5. test will pass. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129650|0 |1 is obsolete| | --- Comment #11 from Andrew Nugged <nugged@gmail.com> --- Created attachment 129651 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129651&action=edit Bug 29906: fix when hold record forcibly gets unwanted "suspended" state This code allows for "Perl false" to pass through to $suspended_until if it is in $hold->suspend_until and in $body->{suspended_until} But in case of "Perl truth" it calls dt_from_string for that value. Reproduction: 1. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 2. see the test fails with something like: # Failed test 'Location change shouldn't touch suspended status' # at t/db_dependent/api/v1/holds.t line 1067. # got: '1' # expected: '0' # Failed test 'suspended_until should be undef' # at t/db_dependent/api/v1/holds.t line 1068. # got: '2022-01-20 00:00:00' # expected: undef # Looks like you failed 2 tests of 39. 3. apply the changes in this patch to Koha/REST/V1/Holds.pm 4. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 5. test will pass. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129651|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 129663 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129663&action=edit Bug 29906: fix when hold record forcibly gets unwanted "suspended" state This code allows for "Perl false" to pass through to $suspended_until if it is in $hold->suspend_until and in $body->{suspended_until} But in case of "Perl truth" it calls dt_from_string for that value. Reproduction: 1. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 2. see the test fails with something like: # Failed test 'Location change shouldn't touch suspended status' # at t/db_dependent/api/v1/holds.t line 1067. # got: '1' # expected: '0' # Failed test 'suspended_until should be undef' # at t/db_dependent/api/v1/holds.t line 1068. # got: '2022-01-20 00:00:00' # expected: undef # Looks like you failed 2 tests of 39. 3. apply the changes in this patch to Koha/REST/V1/Holds.pm 4. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 5. test will pass. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |martin.renvoize@ptfs-europe | |.com --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Good catch! Must admit, I'd missed entirely that this route is a partial update/patch rather than a full object replace/put. I'm still stuck on what route we should take for such routes, as there are various methods already in the codebase and we should try to work back to consistency. However, this patch certainly resolves the issue as it stands. Signing off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Oop, Jonathan and I clashed.. just taking a look at his test to compare you Andrews before I finish SO -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 129663 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129663 Bug 29906: fix when hold record forcibly gets unwanted "suspended" state Review of attachment 129663: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=29906&attachment=129663) ----------------------------------------------------------------- ::: Koha/REST/V1/Holds.pm @@ +282,4 @@
# suspended_until can also be set to undef my $suspended_until = exists $body->{suspended_until} + ? $body->{suspended_until} && dt_from_string( $body->{suspended_until}, 'rfc3339' )
I don't think we need this line changed.. just the second one. At least.. it's the second line your test is testing and as Jonathan has highlighted, it appears you can't unset the suspension by just passing 'null' to suspend_until at the moment. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This route is kinda horrible.. it's neither a proper PUT or a PATCH.. it allows for setting pickup_location and/or priority and finally allows for suspending but not unsuspending a hold by setting the suspend_until date. There are explicit routes for all those too, which are significantly clearer in my opinion. Jonathans test was indeed incorrect as compared to what the swagger spec allows to be PUT/PATCHed as suspend_until is allowed to be missing but not allowed to be null. So, finally.. I think I can mark this as signed off.. as it's consistent with how things work now. But.. I really don't like it ;P -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 129666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129666&action=edit Bug 29906: (follow-up) Clarify actual code use -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Martin Renvoize <martin.renvoize@ptfs-europe.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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |The PATCH/PUT release notes| |/api/v1/holds/{hold_id} API | |endpoint allows for partial | |updates of Holds. Priority | |and Pickup Location are | |both available to change | |(though it is preferred to | |use the routes specifically | |added for manipulating | |them). | | | |Suspend_until can | |also be added/updated to | |add or lengthen an existing | |suspension, but the field | |cannot be set to null to | |remove the suspension at | |present. | | | |This patch | |restores the suspen_until | |function to ensure | |suspensions are not | |triggered by unrelated | |pickup location or priority | |changes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #18 from Andrew Nugged <nugged@gmail.com> --- (In reply to Martin Renvoize from comment #15)
I don't think we need this line changed.. just the second one.
I agree that it's somewhat double-checking, because API specs don't allow undef to pass here, so $body->{suspended_until} don't expect ever be undef (I just added that check to restore "old code logic", but this is not important) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129663|0 |1 is obsolete| | Attachment #129666|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129898 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129898&action=edit Bug 29906: fix when hold record forcibly gets unwanted "suspended" state This code allows for "Perl false" to pass through to $suspended_until if it is in $hold->suspend_until and in $body->{suspended_until} But in case of "Perl truth" it calls dt_from_string for that value. Reproduction: 1. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 2. see the test fails with something like: # Failed test 'Location change shouldn't touch suspended status' # at t/db_dependent/api/v1/holds.t line 1067. # got: '1' # expected: '0' # Failed test 'suspended_until should be undef' # at t/db_dependent/api/v1/holds.t line 1068. # got: '2022-01-20 00:00:00' # expected: undef # Looks like you failed 2 tests of 39. 3. apply the changes in this patch to Koha/REST/V1/Holds.pm 4. run provided test in kshell: prove t/db_dependent/api/v1/holds.t 5. test will pass. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #20 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129899 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129899&action=edit Bug 29906: (follow-up) Clarify actual code use Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #21 from Fridolin Somers <fridolin.somers@biblibre.com> --- I validated unit test with Bug 29975 applied. Otherwise it fails, independently of this one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 --- Comment #22 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable CC| |kyle@bywatersolutions.com Version(s)|22.05.00 |22.05.00,21.11.03 released in| | --- Comment #23 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.11.x for 21.11.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29906 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com --- Comment #24 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Missing dependency, not backported to 21.05 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org