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)
(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.
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
If I don't put the packet, with "DefaultHoldExpirationdate" on the set and with DefaultHoldExpirationdatePeriod blank, the reservation expiration date remains blank.
(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.
(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".
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>
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>
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.
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>
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
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>
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>
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.
(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.
Calling for reinforcements to hopefully get another review/opinion.