Bug 29074 - DefaultHoldExpirationdatePeriod blank value interpreted as zero
Summary: DefaultHoldExpirationdatePeriod blank value interpreted as zero
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on: 26498
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-21 15:39 UTC by Andrew Fuerste-Henry
Modified: 2023-11-06 22:06 UTC (History)
7 users (show)

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


Attachments
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.16 KB, patch)
2022-12-14 11:42 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.21 KB, patch)
2023-08-17 07:22 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.27 KB, patch)
2023-08-25 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.28 KB, patch)
2023-10-13 11:52 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29074: Prevent undef warning when DefaultHoldExpirationdate is read (1.28 KB, patch)
2023-10-13 11:53 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.33 KB, patch)
2023-10-13 16:47 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29074: Prevent undef warning when DefaultHoldExpirationdate is read (1.33 KB, patch)
2023-10-13 16:47 UTC, Lucas Gass
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2021-09-21 15:39:24 UTC
If DefaultHoldExpirationdate is on and DefaultHoldExpirationdatePeriod is blank, the blank is treated as a zero and all holds are created with an expiration date of today. I'm not sure what the best answer is. We could:
- add some text to the syspref to make clear that a blank is treated as zero
- make it always save and display a zero when you try to save a blank
or
- make it treat a blank as disabled (and then add some text to the syspref to tell people that)
Comment 1 Emmi Takkinen 2022-11-11 10:58:50 UTC
(In reply to Andrew Fuerste-Henry from comment #0)
> If DefaultHoldExpirationdate is on and DefaultHoldExpirationdatePeriod is
> blank, the blank is treated as a zero and all holds are created with an
> expiration date of today. I'm not sure what the best answer is. We could:
> - add some text to the syspref to make clear that a blank is treated as zero
> - make it always save and display a zero when you try to save a blank
> or
> - make it treat a blank as disabled (and then add some text to the syspref
> to tell people that)

Hmm, I'd say either of two last options could be the way to go.
Comment 2 Emmi Takkinen 2022-12-14 11:42:12 UTC
Created attachment 144572 [details] [review]
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank

If DefaultHoldExpirationdate is on and DefaultHoldExpirationdatePeriod
is blank, the blank is treated as a zero and all holds are created
with an expiration date of today.

This patch changes expiration dates storing so that if
DefaultHoldExpirationdatePeriod is left blank, expiration date is not
set. Users are informed about this with a note in syspref.

To test:
1. Set DefaultHoldExpirationdate on but leave
DefaultHoldExpirationdatePeriod blank.
2. Add hold for patron.
=> Note that expiration date is set as today.
3. Apply this patch
=> Confirm following note is now added after sysprefs:
"NOTE: If DefaultHoldExpirationdatePeriod is left blank
default expiration date is not set."
4. Add new hold for patron.
=> Note that expiration date is not set.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Comment 3 Marius 2023-01-16 20:03:44 UTC
If I don't put the packet, with "DefaultHoldExpirationdate" on the set and with DefaultHoldExpirationdatePeriod blank, the reservation expiration date remains blank.
Comment 4 Emmi Takkinen 2023-06-27 08:14:15 UTC
(In reply to Marius from comment #3)
> If I don't put the packet, with "DefaultHoldExpirationdate" on the set and
> with DefaultHoldExpirationdatePeriod blank, the reservation expiration date
> remains blank.

This is odd, without using this patch if DefaultHoldExpirationdatePeriod is blank, expiration date is set in database but it's not displayed in Koha.

And even odder, this happens only if syspref dateformat is set as any other than yyyy-mm-dd. This seems like an whole other bug to me.
Comment 5 Emmi Takkinen 2023-06-27 10:35:37 UTC
(In reply to Emmi Takkinen from comment #4)
> (In reply to Marius from comment #3)
> > If I don't put the packet, with "DefaultHoldExpirationdate" on the set and
> > with DefaultHoldExpirationdatePeriod blank, the reservation expiration date
> > remains blank.
> 
> This is odd, without using this patch if DefaultHoldExpirationdatePeriod is
> blank, expiration date is set in database but it's not displayed in Koha.
> 
> And even odder, this happens only if syspref dateformat is set as any other
> than yyyy-mm-dd. This seems like an whole other bug to me.

This happanes because expiration date input has attribute 'data-flatpickr-futuredate="true"'. When this is set, expiration dates set as today or in the past aren't displayed. Which can now happen if "DefaultHoldExpirationdate" is enabled and "DefaultHoldExpirationdatePeriod" is blank. Setting this bug back to "Needs Signoff".
Comment 6 ByWater Sandboxes 2023-08-17 07:22:46 UTC
Created attachment 154484 [details] [review]
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank

If DefaultHoldExpirationdate is on and DefaultHoldExpirationdatePeriod
is blank, the blank is treated as a zero and all holds are created
with an expiration date of today.

This patch changes expiration dates storing so that if
DefaultHoldExpirationdatePeriod is left blank, expiration date is not
set. Users are informed about this with a note in syspref.

To test:
1. Set DefaultHoldExpirationdate on but leave
DefaultHoldExpirationdatePeriod blank.
2. Add hold for patron.
=> Note that expiration date is set as today.
3. Apply this patch
=> Confirm following note is now added after sysprefs:
"NOTE: If DefaultHoldExpirationdatePeriod is left blank
default expiration date is not set."
4. Add new hold for patron.
=> Note that expiration date is not set.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: stina <stina.hallin@ub.lu.se>
Comment 7 Katrin Fischer 2023-08-25 14:00:31 UTC
Created attachment 154813 [details] [review]
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank

If DefaultHoldExpirationdate is on and DefaultHoldExpirationdatePeriod
is blank, the blank is treated as a zero and all holds are created
with an expiration date of today.

This patch changes expiration dates storing so that if
DefaultHoldExpirationdatePeriod is left blank, expiration date is not
set. Users are informed about this with a note in syspref.

To test:
1. Set DefaultHoldExpirationdate on but leave
DefaultHoldExpirationdatePeriod blank.
2. Add hold for patron.
=> Note that expiration date is set as today.
3. Apply this patch
=> Confirm following note is now added after sysprefs:
"NOTE: If DefaultHoldExpirationdatePeriod is left blank
default expiration date is not set."
4. Add new hold for patron.
=> Note that expiration date is not set.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: stina <stina.hallin@ub.lu.se>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Tomás Cohen Arazi 2023-08-30 13:11:24 UTC
Should the condition be:

&& defined C4::Context->preference('DefaultHoldExpirationdatePeriod')
&& C4::Context->preference('DefaultHoldExpirationdatePeriod') ne ''

otherwise it will log a warning if it is undef, each time the syspref is read.
Comment 9 Emmi Takkinen 2023-10-13 11:52:47 UTC
Created attachment 157068 [details] [review]
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank

If DefaultHoldExpirationdate is on and DefaultHoldExpirationdatePeriod
is blank, the blank is treated as a zero and all holds are created
with an expiration date of today.

This patch changes expiration dates storing so that if
DefaultHoldExpirationdatePeriod is left blank, expiration date is not
set. Users are informed about this with a note in syspref.

To test:
1. Set DefaultHoldExpirationdate on but leave
DefaultHoldExpirationdatePeriod blank.
2. Add hold for patron.
=> Note that expiration date is set as today.
3. Apply this patch
=> Confirm following note is now added after sysprefs:
"NOTE: If DefaultHoldExpirationdatePeriod is left blank
default expiration date is not set."
4. Add new hold for patron.
=> Note that expiration date is not set.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: stina <stina.hallin@ub.lu.se>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Emmi Takkinen 2023-10-13 11:53:25 UTC
Created attachment 157069 [details] [review]
Bug 29074: Prevent undef warning when DefaultHoldExpirationdate is read

Test plan:
Repeat test plan from first patch.
Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Comment 11 Lucas Gass 2023-10-13 16:47:29 UTC
Created attachment 157128 [details] [review]
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank

If DefaultHoldExpirationdate is on and DefaultHoldExpirationdatePeriod
is blank, the blank is treated as a zero and all holds are created
with an expiration date of today.

This patch changes expiration dates storing so that if
DefaultHoldExpirationdatePeriod is left blank, expiration date is not
set. Users are informed about this with a note in syspref.

To test:
1. Set DefaultHoldExpirationdate on but leave
DefaultHoldExpirationdatePeriod blank.
2. Add hold for patron.
=> Note that expiration date is set as today.
3. Apply this patch
=> Confirm following note is now added after sysprefs:
"NOTE: If DefaultHoldExpirationdatePeriod is left blank
default expiration date is not set."
4. Add new hold for patron.
=> Note that expiration date is not set.

Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: stina <stina.hallin@ub.lu.se>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 12 Lucas Gass 2023-10-13 16:47:32 UTC
Created attachment 157129 [details] [review]
Bug 29074: Prevent undef warning when DefaultHoldExpirationdate is read

Test plan:
Repeat test plan from first patch.
Also prove t/db_dependent/Hold.t.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 13 Katrin Fischer 2023-10-14 16:21:19 UTC
1) The changes make sense, but I wondered if it wouldn't be safer to change

sub _set_default_expirationdate {
    my $self = shift;

    my $period = C4::Context->preference('DefaultHoldExpirationdatePeriod') || 0;
    my $timeunit =

?

This would mean one change in one spot instead of changing 2.

We could just "do onthing" if DefaultHoldExpirationdatePeriod is undefined/empty.

2) For the note I suggest a tiny rephrase:

 If <a href="/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=DefaultHoldExpirationdatePeriod">DefaultHoldExpirationdatePeriod</a> is left blank default expiration date is not set.</br>

..., the default expiration date is not set.
..., no default expiration date is set.
Comment 14 Emmi Takkinen 2023-11-03 10:03:11 UTC
(In reply to Katrin Fischer from comment #13)
> 1) The changes make sense, but I wondered if it wouldn't be safer to change
> 
> sub _set_default_expirationdate {
>     my $self = shift;
> 
>     my $period = C4::Context->preference('DefaultHoldExpirationdatePeriod')
> || 0;
>     my $timeunit =
> 
> ?
> 
> This would mean one change in one spot instead of changing 2.
> 
> We could just "do onthing" if DefaultHoldExpirationdatePeriod is
> undefined/empty.
That's a good point, I'm just not sure if there's point to trigger sub _set_default_expirationdate at all. I mean, with this current patch we don't proceed to sub _set_default_expirationdate if DefaultHoldExpirationdatePeriod is not set. There's also less code to execute this way. But that's just my hunch :D If someone has any other points to this, please comment.    

> 2) For the note I suggest a tiny rephrase:
> 
>  If <a
> href="/cgi-bin/koha/admin/preferences.
> pl?op=search&searchfield=DefaultHoldExpirationdatePeriod">DefaultHoldExpirati
> ondatePeriod</a> is left blank default expiration date is not set.</br>
> 
> ..., the default expiration date is not set.
> ..., no default expiration date is set.
Makes sense. I'll adjust this after we decide what to do with point 1, since if we proceed with that, it would probably be good idea to rewrite these patches.
Comment 15 Katrin Fischer 2023-11-06 22:06:55 UTC
Calling for reinforcements to hopefully get another review/opinion.