|
Description
Emmi Takkinen
2023-06-16 12:06:14 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. 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> 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)
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.
$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?
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. (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 >) ) (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? (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? (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? Sounds good to me. 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 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? (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? (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? Hi Emmi, I just came across bug 35306 that seems to be related and maybe explains what we have been wondering about here. (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. (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". 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. 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> My apologies. I forgot to hit save when changing the system preference for DefaultHoldExpirationdate. This does appear to work. 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> 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> Created attachment 164825 [details] [review] Bug 34032: (QA follow-up) Tidy code 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? Pushed for 24.05! Well done everyone, thank you! I prefer not to risk impact on stable branch 23.11.x |