Summary: | When changing hold parameters over API (PUT) it forcibly gets to "suspended" state | ||
---|---|---|---|
Product: | Koha | Reporter: | Peter Vashchuk <stalkernoid> |
Component: | Hold requests | Assignee: | Peter Vashchuk <stalkernoid> |
Status: | CLOSED FIXED | QA Contact: | Tomás Cohen Arazi <tomascohen> |
Severity: | major | ||
Priority: | P5 - low | CC: | andrewfh, fridolin.somers, gmcharlt, jonathan.druart, kyle, martin.renvoize, nugged |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
The PATCH/PUT /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.
|
Version(s) released in: |
22.05.00,21.11.03
|
Bug Depends on: | 24850 | ||
Bug Blocks: | |||
Attachments: |
Bug 29906: fix the forcibly gets to "suspended" state problem
Bug 29906: fix the forcibly gets to "suspended" state problem Bug 29906: fix the forcibly gets to "suspended" state problem Bug 29906: fix the forcibly gets to "suspended" state problem Bug 29906: Add test Bug 29906: fix when hold record forcibly gets unwanted "suspended" state Bug 29906: fix when hold record forcibly gets unwanted "suspended" state Bug 29906: fix when hold record forcibly gets unwanted "suspended" state Bug 29906: (follow-up) Clarify actual code use Bug 29906: fix when hold record forcibly gets unwanted "suspended" state Bug 29906: (follow-up) Clarify actual code use |
Description
Peter Vashchuk
2022-01-19 15:55:46 UTC
Created attachment 129619 [details] [review] 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 Created attachment 129621 [details] [review] 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. Created attachment 129622 [details] [review] 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. Created attachment 129626 [details] [review] 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. Please detail, it does not work for me. Created attachment 129645 [details] [review] Bug 29906: Add test 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. 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.
Let me update the test you provided to show how that will happen... Created attachment 129650 [details] [review] 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. Created attachment 129651 [details] [review] 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. Created attachment 129663 [details] [review] 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> 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. Oop, Jonathan and I clashed.. just taking a look at his test to compare you Andrews before I finish SO Comment on attachment 129663 [details] [review] Bug 29906: fix when hold record forcibly gets unwanted "suspended" state Review of attachment 129663 [details] [review]: ----------------------------------------------------------------- ::: 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. 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 Created attachment 129666 [details] [review] Bug 29906: (follow-up) Clarify actual code use (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) Created attachment 129898 [details] [review] 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> Created attachment 129899 [details] [review] Bug 29906: (follow-up) Clarify actual code use Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> I validated unit test with Bug 29975 applied. Otherwise it fails, independently of this one. Pushed to master for 22.05, thanks to everybody involved 🦄 Pushed to 21.11.x for 21.11.03 Missing dependency, not backported to 21.05 |