Bug 29906

Summary: When changing hold parameters over API (PUT) it forcibly gets to "suspended" state
Product: Koha Reporter: Peter Vashchuk <stalkernoid>
Component: Hold requestsAssignee: 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
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