Bug 29074 - DefaultHoldExpirationdatePeriod blank value interpreted as zero
Summary: DefaultHoldExpirationdatePeriod blank value interpreted as zero
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Emmi Takkinen
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 26498
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-21 15:39 UTC by Andrew Fuerste-Henry
Modified: 2025-05-12 16:31 UTC (History)
8 users (show)

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


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 (lukeg)
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 (lukeg)
Details | Diff | Splinter Review
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.31 KB, patch)
2025-04-04 11:41 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29074: Prevent undef warning when DefaultHoldExpirationdate is read (1.32 KB, patch)
2025-04-04 11:41 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29074: Adjust syspref DefaultHoldExpirationdateUnitOfTime description (1.47 KB, patch)
2025-04-04 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.26 KB, patch)
2025-04-22 06:19 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.32 KB, patch)
2025-04-22 12:14 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29074: Do not set default expiration date for hold if DefaultHoldExpirationdatePeriod is left blank (4.39 KB, patch)
2025-05-09 14:20 UTC, Martin Renvoize (ashimema)
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 (tcohen) 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 (lukeg) 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 (lukeg) 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.
Comment 16 Emmi Takkinen 2025-04-04 11:41:17 UTC
Created attachment 180628 [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 17 Emmi Takkinen 2025-04-04 11:41:41 UTC
Created attachment 180629 [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 18 Emmi Takkinen 2025-04-04 11:42:05 UTC
Created attachment 180630 [details] [review]
Bug 29074: Adjust syspref DefaultHoldExpirationdateUnitOfTime description
Comment 19 Emmi Takkinen 2025-04-04 11:44:35 UTC
Rebased patches and adjusted DefaultHoldExpirationdateUnitOfTime description according Katrins second comment. I'll leave this as Failed QA since we still need more comments on should we proceed with these changes or instead make changes to sub _set_default_expirationdate.
Comment 20 Emily Lamancusa (emlam) 2025-04-04 15:09:41 UTC
I'm inclined to agree with Katrin for maintainability purposes. If we need to debug or change this behavior for default expiration dates later, it's going to be easier to find the appropriate code in the _set_default_expirationdate method, rather than needing to trace the code to find it in the store method. _set_default_expirationdate is also only three lines of code - checking two sysprefs and then setting the new expiration date - so we don't save much by avoiding the function call.

That said, since the change is in the pm file and covered by tests either way, I don't think it's particularly higher-risk to keep the patch as-is if there's more support/reason for doing it that way. (But if the current patch is kept, since it touches both branches of an if/else, the tests should test both cases, also.)
Comment 21 Emmi Takkinen 2025-04-22 06:19:57 UTC
Created attachment 181237 [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 22 Emmi Takkinen 2025-04-22 06:21:44 UTC
Adjusted changes based on feedback. I kept the testes same as before.
Comment 23 Andrew Fuerste-Henry 2025-04-22 12:14:45 UTC
Created attachment 181249 [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: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 24 Martin Renvoize (ashimema) 2025-05-09 14:20:21 UTC
Created attachment 182180 [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: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 25 Martin Renvoize (ashimema) 2025-05-09 14:20:35 UTC
Nice work
Comment 26 Katrin Fischer 2025-05-12 16:31:54 UTC
Pushed for 25.05!

Well done everyone, thank you!