Bug 29906 - When changing hold parameters over API (PUT) it forcibly gets to "suspended" state
Summary: When changing hold parameters over API (PUT) it forcibly gets to "suspended" ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Peter Vashchuk
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 24850
Blocks:
  Show dependency treegraph
 
Reported: 2022-01-19 15:55 UTC by Peter Vashchuk
Modified: 2022-06-06 20:31 UTC (History)
7 users (show)

See Also:
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


Attachments
Bug 29906: fix the forcibly gets to "suspended" state problem (1.10 KB, patch)
2022-01-19 16:19 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29906: fix the forcibly gets to "suspended" state problem (1.10 KB, patch)
2022-01-19 16:22 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29906: fix the forcibly gets to "suspended" state problem (1.10 KB, patch)
2022-01-19 16:45 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29906: fix the forcibly gets to "suspended" state problem (1.10 KB, patch)
2022-01-19 17:35 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29906: Add test (1.22 KB, patch)
2022-01-20 11:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29906: fix when hold record forcibly gets unwanted "suspended" state (3.04 KB, patch)
2022-01-20 13:05 UTC, Andrii Nugged
Details | Diff | Splinter Review
Bug 29906: fix when hold record forcibly gets unwanted "suspended" state (3.02 KB, patch)
2022-01-20 13:24 UTC, Andrii Nugged
Details | Diff | Splinter Review
Bug 29906: fix when hold record forcibly gets unwanted "suspended" state (3.07 KB, patch)
2022-01-20 14:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29906: (follow-up) Clarify actual code use (910 bytes, patch)
2022-01-20 15:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29906: fix when hold record forcibly gets unwanted "suspended" state (3.15 KB, patch)
2022-01-27 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29906: (follow-up) Clarify actual code use (985 bytes, patch)
2022-01-27 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2022-01-19 15:55:46 UTC
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.
Comment 1 Peter Vashchuk 2022-01-19 16:19:00 UTC Comment hidden (obsolete)
Comment 2 Peter Vashchuk 2022-01-19 16:22:55 UTC Comment hidden (obsolete)
Comment 3 Peter Vashchuk 2022-01-19 16:45:11 UTC Comment hidden (obsolete)
Comment 4 Peter Vashchuk 2022-01-19 17:35:05 UTC
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.
Comment 5 Jonathan Druart 2022-01-20 11:33:43 UTC
Please detail, it does not work for me.
Comment 6 Jonathan Druart 2022-01-20 11:34:44 UTC
Created attachment 129645 [details] [review]
Bug 29906: Add test
Comment 7 Jonathan Druart 2022-01-20 11:35:13 UTC
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.
Comment 8 Andrii Nugged 2022-01-20 13:04:36 UTC
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.
Comment 9 Andrii Nugged 2022-01-20 13:04:46 UTC
Let me update the test you provided to show how that will happen...
Comment 10 Andrii Nugged 2022-01-20 13:05:30 UTC Comment hidden (obsolete)
Comment 11 Andrii Nugged 2022-01-20 13:24:25 UTC
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.
Comment 12 Martin Renvoize 2022-01-20 14:38:02 UTC
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>
Comment 13 Martin Renvoize 2022-01-20 14:39:52 UTC
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.
Comment 14 Martin Renvoize 2022-01-20 14:44:06 UTC
Oop, Jonathan and I clashed.. just taking a look at his test to compare you Andrews before I finish SO
Comment 15 Martin Renvoize 2022-01-20 15:08:05 UTC
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.
Comment 16 Martin Renvoize 2022-01-20 15:14:01 UTC
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
Comment 17 Martin Renvoize 2022-01-20 15:15:49 UTC
Created attachment 129666 [details] [review]
Bug 29906: (follow-up) Clarify actual code use
Comment 18 Andrii Nugged 2022-01-21 09:32:26 UTC
(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)
Comment 19 Jonathan Druart 2022-01-27 15:28:49 UTC
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>
Comment 20 Jonathan Druart 2022-01-27 15:28:55 UTC
Created attachment 129899 [details] [review]
Bug 29906: (follow-up) Clarify actual code use

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Fridolin Somers 2022-02-01 01:28:22 UTC
I validated unit test with Bug 29975 applied.
Otherwise it fails, independently of this one.
Comment 22 Fridolin Somers 2022-02-01 07:57:27 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 23 Kyle M Hall 2022-02-07 16:46:19 UTC
Pushed to 21.11.x for 21.11.03
Comment 24 Andrew Fuerste-Henry 2022-02-21 15:42:07 UTC
Missing dependency, not backported to 21.05