Bug 34032 - Holds expirationdate left blank if waiting status is reverted
Summary: Holds expirationdate left blank if waiting status is reverted
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Emmi Takkinen
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-16 12:06 UTC by Emmi Takkinen
Modified: 2024-04-29 07:38 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 34032: Set new expirationdate if waiting status is reverted (3.78 KB, patch)
2023-06-21 11:37 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34032: Set new expirationdate if waiting status is reverted (3.82 KB, patch)
2023-06-21 17:34 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 34032: Use reserves.patron_expiration_date if set (3.58 KB, patch)
2023-11-06 12:35 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34032: Use reserves.patron_expiration_date if set (3.63 KB, patch)
2023-12-15 20:24 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 34032: Set new expirationdate if waiting status is reverted (3.87 KB, patch)
2024-04-12 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34032: Use reserves.patron_expiration_date if set (3.63 KB, patch)
2024-04-12 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34032: (QA follow-up) Tidy code (3.48 KB, patch)
2024-04-12 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2023-06-16 12:06:14 UTC
Pretty much what summary says. 

To reproduce:
1) Make sure you have DefaultHoldExpirationdate syspref enabled.
2) Find hold with status "Waiting".
3) Revert waiting status.
=> Note that hold has no expiration date set.
Comment 1 Emmi Takkinen 2023-06-21 11:37:33 UTC
Created attachment 152516 [details] [review]
Bug 34032: Set new expirationdate if waiting status is reverted

When one reverts holds waiting status holds expiration
date is not set even if DefaultHoldExpirationdate
syspref is enabled. This patch adds new param
hold_reverted to be used when RevertWaitingStatus is
used to determine if expiration date should be set again.

To test:
1) Make sure you have DefaultHoldExpirationdate syspref enabled.
2) Find hold with status "Waiting".
3) Revert waiting status.
=> Note that hold has no expiration date set.
4) Apply this patch.
5) Repeat steps 2 and 3.
=> Expiration date should now be set based on reserve
date.

Also prove t/db_dependent/Hold.t.
Comment 2 Sam Lau 2023-06-21 17:34:16 UTC
Created attachment 152535 [details] [review]
Bug 34032: Set new expirationdate if waiting status is reverted

When one reverts holds waiting status holds expiration
date is not set even if DefaultHoldExpirationdate
syspref is enabled. This patch adds new param
hold_reverted to be used when RevertWaitingStatus is
used to determine if expiration date should be set again.

To test:
1) Make sure you have DefaultHoldExpirationdate syspref enabled.
2) Find hold with status "Waiting".
3) Revert waiting status.
=> Note that hold has no expiration date set.
4) Apply this patch.
5) Repeat steps 2 and 3.
=> Expiration date should now be set based on reserve
date.

Also prove t/db_dependent/Hold.t.

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 3 Marcel de Rooy 2023-09-08 09:49:04 UTC
Wont complain about

 WARN   C4/Reserves.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 470, now: 471)

 WARN   Koha/Hold.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 126, now: 129)

 WARN   t/db_dependent/Hold.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 82, now: 89)
Comment 4 Marcel de Rooy 2023-09-08 10:05:41 UTC
Did not finish this one completely now, but feel somehow that it needs more attention still (qa intuition). Hmm.

Reserves still contains:
            expirationdate => $hold->patron_expiration_date,
Are we not respecting that one?

         my %updated_columns = $self->_result->get_dirty_columns;
         return $self->SUPER::store unless %updated_columns;
This may be obvious. But should we add $hold_reverted here? Or just trust that we have dirty columns coming from RevertWaitingStatus?

+                ( C4::Context->preference('DefaultHoldExpirationdate')
+                && ! exists $updated_columns{expirationdate} )
+                || ( C4::Context->preference('DefaultHoldExpirationdate')
+                && exists $updated_columns{expirationdate}
+                && $hold_reverted )
Still breaking my head here.

Could you please clarify in meantime a bit? Thx

Status change for feedback.
Comment 5 Marcel de Rooy 2023-09-12 14:45:04 UTC
    $hold->set(
        {
            priority    => 1,
            found       => undef,
            waitingdate => undef,
            expirationdate => $hold->patron_expiration_date,
            itemnumber  => $hold->item_level_hold ? $hold->itemnumber : undef,
        }
    )->store({ hold_reverted => 1 });

Currently, only this case triggers part of the condition (that could be simplified to reduce repetition) that leads to calling:
                 $self->_set_default_expirationdate;
(Unless both dates are still the same..)

If they are not, why not respect patron_expiration_date?
Comment 6 Emmi Takkinen 2023-11-03 10:21:51 UTC
Since it has been 4+ months since I worked on this, I'm utterly confused what I have been thinking while doing this. Looking into this again.
Comment 7 Emmi Takkinen 2023-11-03 12:57:19 UTC
(In reply to Marcel de Rooy from comment #4)
> Did not finish this one completely now, but feel somehow that it needs more
> attention still (qa intuition). Hmm.
> 
> Reserves still contains:
>             expirationdate => $hold->patron_expiration_date,
> Are we not respecting that one?
If hold is not in storage, then we still use patron_expiration_date. It isn't used even currently when we update hold. 

>          my %updated_columns = $self->_result->get_dirty_columns;
>          return $self->SUPER::store unless %updated_columns;
> This may be obvious. But should we add $hold_reverted here? Or just trust
> that we have dirty columns coming from RevertWaitingStatus?
It is probably save to assume that we receive dirty columns here, since reverting waiting status changes holds priority as 1. And by my logic that happens every time we revert.

> +                ( C4::Context->preference('DefaultHoldExpirationdate')
> +                && ! exists $updated_columns{expirationdate} )
> +                || ( C4::Context->preference('DefaultHoldExpirationdate')
> +                && exists $updated_columns{expirationdate}
> +                && $hold_reverted )
> Still breaking my head here.
Gosh, perltidy turns that block into a mess. But this is unnecessarily complex and a I have no idea what I was thinking when writing this. We can probably achieve same result with this:

> ( C4::Context->preference('DefaultHoldExpirationdate')
>                && ( ! exists $updated_columns{expirationdate} || $hold_reverted 
>) )
Comment 8 Emmi Takkinen 2023-11-03 13:34:38 UTC
(In reply to Marcel de Rooy from comment #5)
>     $hold->set(
>         {
>             priority    => 1,
>             found       => undef,
>             waitingdate => undef,
>             expirationdate => $hold->patron_expiration_date,
>             itemnumber  => $hold->item_level_hold ? $hold->itemnumber :
> undef,
>         }
>     )->store({ hold_reverted => 1 });
> 
> Currently, only this case triggers part of the condition (that could be
> simplified to reduce repetition) that leads to calling:
>                  $self->_set_default_expirationdate;
> (Unless both dates are still the same..)
> 
> If they are not, why not respect patron_expiration_date?
Because I just realized there is a column called patron_expiration_date. Somehow this has totally slipped from my attention. You're right, if patron_expiration_date exists we should respect it, not generate new one. However, what should we do if patron_expiration_date is in past?
Comment 9 Katrin Fischer 2023-11-04 13:06:23 UTC
(In reply to Emmi Takkinen from comment #8)
> (In reply to Marcel de Rooy from comment #5)
> >     $hold->set(
> >         {
> >             priority    => 1,
> >             found       => undef,
> >             waitingdate => undef,
> >             expirationdate => $hold->patron_expiration_date,
> >             itemnumber  => $hold->item_level_hold ? $hold->itemnumber :
> > undef,
> >         }
> >     )->store({ hold_reverted => 1 });
> > 
> > Currently, only this case triggers part of the condition (that could be
> > simplified to reduce repetition) that leads to calling:
> >                  $self->_set_default_expirationdate;
> > (Unless both dates are still the same..)
> > 
> > If they are not, why not respect patron_expiration_date?
> Because I just realized there is a column called patron_expiration_date.
> Somehow this has totally slipped from my attention. You're right, if
> patron_expiration_date exists we should respect it, not generate new one.
> However, what should we do if patron_expiration_date is in past?

It's a really good question. My thought would be to use the existing date anyway, even if in the past. Some libraries might not auto-cancel those and if they auto-cancel that will be run nightly, so there is still a moment to adjust. Maybe we could do an alert or other visual hint?
Comment 10 Emmi Takkinen 2023-11-06 08:20:18 UTC
(In reply to Katrin Fischer from comment #9)
> (In reply to Emmi Takkinen from comment #8)
> > (In reply to Marcel de Rooy from comment #5)
> > >     $hold->set(
> > >         {
> > >             priority    => 1,
> > >             found       => undef,
> > >             waitingdate => undef,
> > >             expirationdate => $hold->patron_expiration_date,
> > >             itemnumber  => $hold->item_level_hold ? $hold->itemnumber :
> > > undef,
> > >         }
> > >     )->store({ hold_reverted => 1 });
> > > 
> > > Currently, only this case triggers part of the condition (that could be
> > > simplified to reduce repetition) that leads to calling:
> > >                  $self->_set_default_expirationdate;
> > > (Unless both dates are still the same..)
> > > 
> > > If they are not, why not respect patron_expiration_date?
> > Because I just realized there is a column called patron_expiration_date.
> > Somehow this has totally slipped from my attention. You're right, if
> > patron_expiration_date exists we should respect it, not generate new one.
> > However, what should we do if patron_expiration_date is in past?
> 
> It's a really good question. My thought would be to use the existing date
> anyway, even if in the past. Some libraries might not auto-cancel those and
> if they auto-cancel that will be run nightly, so there is still a moment to
> adjust. Maybe we could do an alert or other visual hint?

Hmm, maybe displaying field as red would be enough?
Comment 11 Katrin Fischer 2023-11-06 08:24:04 UTC
Sounds good to me.
Comment 12 Emmi Takkinen 2023-11-06 12:35:02 UTC
Created attachment 158516 [details] [review]
Bug 34032: Use reserves.patron_expiration_date if set

If reserves.patron_expiration_date is set use it as holds
expiration date when waiting status is reverted.

To test:
1. Apply this patch.
2. Add hold for patron A and set expiration date manually.
3. Check in item on hold for patron A and confirm hold was
set as waiting.
4. Revert holds waiting status.
=> Hold should still have expiration date you set manually
in step 2.
5. Check that you have DefaultHoldExpirationdate and other
DefaultHold sysprefs set.
6. Add hold for patron B, but this time do not set expiration
date.
7. Check in item on hold for patron B, revert waiting status.
=> Hold should now have expiration date set based on
DefaultHold sysprefs.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Comment 13 Emmi Takkinen 2023-11-06 13:17:39 UTC
We don't probably have to do anything to past dates, since they aren't displayed at all. If I set reserves.expirationdate in past (or current date) through database, expiration date field is blank. Is this intentional?
Comment 14 Katrin Fischer 2023-11-06 15:22:05 UTC
(In reply to Emmi Takkinen from comment #13)
> We don't probably have to do anything to past dates, since they aren't
> displayed at all. If I set reserves.expirationdate in past (or current date)
> through database, expiration date field is blank. Is this intentional?

Hm, I have been wondering what we show in the GUI but didn't check.

It might all be a bit 'history' there. In the past there was only one field in the database used for both the "don't need after x" and "pick-up before". That lead to a couple of issues.

Later we divided the fields in the database (yay!), but maybe we don't show both in the GUI?
Comment 15 Emmi Takkinen 2023-11-07 06:31:58 UTC
(In reply to Katrin Fischer from comment #14)
> (In reply to Emmi Takkinen from comment #13)
> > We don't probably have to do anything to past dates, since they aren't
> > displayed at all. If I set reserves.expirationdate in past (or current date)
> > through database, expiration date field is blank. Is this intentional?
> 
> Hm, I have been wondering what we show in the GUI but didn't check.
> 
> It might all be a bit 'history' there. In the past there was only one field
> in the database used for both the "don't need after x" and "pick-up before".
> That lead to a couple of issues.
> 
> Later we divided the fields in the database (yay!), but maybe we don't show
> both in the GUI?

Technically we don't show them both, we show value in reserves.expirationdate. But if reserves.patron_expiration_date is filled, it is copied to reserves.expirationdate (not only with these patches but by default). But this doesn't seem to be the problem here. In GUI it seems that expiration field doesn't display date in past, including if value is current date. They are in database however. There's probably some sort of jQuery (flatpicker maybe) affecting this. I was wondering should we should also show dates in past and display them with visual hind or does the field behave this way on purpose? I was also wondering if we "fix" this, should it be done in another bug?
Comment 16 Katrin Fischer 2023-11-09 21:20:36 UTC
Hi Emmi, I just came across bug 35306 that seems to be related and maybe explains what we have been wondering about here.
Comment 17 Emmi Takkinen 2023-11-17 08:12:24 UTC
(In reply to Katrin Fischer from comment #16)
> Hi Emmi, I just came across bug 35306 that seems to be related and maybe
> explains what we have been wondering about here.

So it seems and expiration date is now displayed as plain text. However with these patches expiration date changes.
Comment 18 Emmi Takkinen 2023-11-17 08:44:47 UTC
(In reply to Emmi Takkinen from comment #17)
> (In reply to Katrin Fischer from comment #16)
> > Hi Emmi, I just came across bug 35306 that seems to be related and maybe
> > explains what we have been wondering about here.
> 
> So it seems and expiration date is now displayed as plain text. However with
> these patches expiration date changes.

Hmm, weird after several testes this doesn't happen anymore. Setting status back to "Needs Signoff".
Comment 19 Esther Melander 2023-12-15 19:52:55 UTC
I followed steps 1-7. At step 5 I set the DefaultHoldExpirationdate to 5 days. At the last step, when I revert the waiting status, the expiration date field is blank.

I'm not sure if there were other system preference needed. If so, they should be specified.

Also, after reverting the waiting status and refreshing the page, I got a bunch of errors:

Can't call method "biblionumber" on an undefined value at /kohadevbox/koha/C4/Reserves.pm line 2158

I can attach the error trace, if that would help.
Comment 20 ByWater Sandboxes 2023-12-15 20:24:19 UTC
Created attachment 159908 [details] [review]
Bug 34032: Use reserves.patron_expiration_date if set

If reserves.patron_expiration_date is set use it as holds
expiration date when waiting status is reverted.

To test:
1. Apply this patch.
2. Add hold for patron A and set expiration date manually.
3. Check in item on hold for patron A and confirm hold was
set as waiting.
4. Revert holds waiting status.
=> Hold should still have expiration date you set manually
in step 2.
5. Check that you have DefaultHoldExpirationdate and other
DefaultHold sysprefs set.
6. Add hold for patron B, but this time do not set expiration
date.
7. Check in item on hold for patron B, revert waiting status.
=> Hold should now have expiration date set based on
DefaultHold sysprefs.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Esther <esther@bywatersolutions.com>
Comment 21 Esther Melander 2023-12-15 20:27:13 UTC
My apologies. I forgot to hit save when changing the system preference for DefaultHoldExpirationdate. This does appear to work.
Comment 22 Kyle M Hall 2024-04-12 11:14:42 UTC
Created attachment 164823 [details] [review]
Bug 34032: Set new expirationdate if waiting status is reverted

When one reverts holds waiting status holds expiration
date is not set even if DefaultHoldExpirationdate
syspref is enabled. This patch adds new param
hold_reverted to be used when RevertWaitingStatus is
used to determine if expiration date should be set again.

To test:
1) Make sure you have DefaultHoldExpirationdate syspref enabled.
2) Find hold with status "Waiting".
3) Revert waiting status.
=> Note that hold has no expiration date set.
4) Apply this patch.
5) Repeat steps 2 and 3.
=> Expiration date should now be set based on reserve
date.

Also prove t/db_dependent/Hold.t.

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 23 Kyle M Hall 2024-04-12 11:14:54 UTC
Created attachment 164824 [details] [review]
Bug 34032: Use reserves.patron_expiration_date if set

If reserves.patron_expiration_date is set use it as holds
expiration date when waiting status is reverted.

To test:
1. Apply this patch.
2. Add hold for patron A and set expiration date manually.
3. Check in item on hold for patron A and confirm hold was
set as waiting.
4. Revert holds waiting status.
=> Hold should still have expiration date you set manually
in step 2.
5. Check that you have DefaultHoldExpirationdate and other
DefaultHold sysprefs set.
6. Add hold for patron B, but this time do not set expiration
date.
7. Check in item on hold for patron B, revert waiting status.
=> Hold should now have expiration date set based on
DefaultHold sysprefs.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Esther <esther@bywatersolutions.com>
Comment 24 Kyle M Hall 2024-04-12 11:14:56 UTC
Created attachment 164825 [details] [review]
Bug 34032: (QA follow-up) Tidy code
Comment 25 Katrin Fischer 2024-04-29 06:57:12 UTC
Kyle, you forgot to add your sign-off lines :)

Also re-tidied Holds.t for the QA script - maybe different versions of Perltidy at work here?
Comment 26 Katrin Fischer 2024-04-29 07:38:02 UTC
Pushed for 24.05!

Well done everyone, thank you!