Bug 33242 - Allow passing things like add_days => 3 to KohaDates filter
Summary: Allow passing things like add_days => 3 to KohaDates filter
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Kyle M Hall
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2023-03-16 09:35 UTC by Marcel de Rooy
Modified: 2023-06-09 14:44 UTC (History)
5 users (show)

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


Attachments
Bug 33242: Allow passing add_{duration} options to KohaDates (2.23 KB, patch)
2023-03-16 10:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33242: Add unit test (1.69 KB, patch)
2023-03-16 10:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33242: Allow passing add_{duration} options to KohaDates (2.21 KB, patch)
2023-03-16 12:49 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33242: Add unit test (1.70 KB, patch)
2023-03-16 12:49 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33242: Allow passing add_{duration} options to KohaDates (2.24 KB, patch)
2023-03-16 12:52 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33242: Add unit test (1.70 KB, patch)
2023-03-16 12:52 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33242: Allow passing add_{duration} options to KohaDates (2.30 KB, patch)
2023-04-28 15:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33242: Allow passing add_{duration} options to KohaDates (2.30 KB, patch)
2023-04-28 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33242: Add unit test (1.76 KB, patch)
2023-04-28 15:50 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 Marcel de Rooy 2023-03-16 09:35:03 UTC
I have a use case where I want to add some days to an issue date in a notice.

We either need a dt_from_string wrapper in our template plugin like bug 33029 proposes or allow passing options like add_days => X to KohaDates which looks nicer but is less generic. Perhaps we can do both btw ! These approaches each have their benefits and do not exclude each other.

Compare the 33029 approach
    [% KohaDates.datetime_from_string(checkout.issuedate).add( days => 3) | $KohaDates %]
with the approch of this report
    [% checkout.issuedate | $KohaDates add_days => 3 %]

The latter notation is definitely shorter and prettier in my use case and does only need a few lines more.
Comment 1 Martin Renvoize 2023-03-16 09:50:39 UTC
Ack, I'm torn now.

I can see the utility of a dt method allowing us full access to dt functions from the template..  but I also love the idea of the 'pretty' syntax we could introduce with this approach.. `[% checkout.issuesdate | $KohaDates add => "3 days" %]`.. I'm overthinking.
Comment 2 Marcel de Rooy 2023-03-16 10:04:46 UTC
Created attachment 148256 [details] [review]
Bug 33242: Allow passing add_{duration} options to KohaDates

For example, if I want to add a few days to the issuedate in a
TT template, I could do this:
    [% checkout.issuedate | $KohaDates add_days => 3 %]

This development allows you to pass add/subtract years, months, weeks,
days, hours, minutes and seconds.

Test plan:
Pick a notice like CHECKOUT.
Add a line like:
    [% checkout.date_due | $KohaDates with_hours => 1, add_minutes => 15 %]
Do a checkout. Verify that the notice generated contains a time that
shifted 15 minutes.
Undo your change.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2023-03-16 10:04:49 UTC
Created attachment 148257 [details] [review]
Bug 33242: Add unit test

Test plan:
Run t/db_dependent/Letters/TemplateToolkit.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Kyle M Hall 2023-03-16 12:16:44 UTC
(In reply to Martin Renvoize from comment #1)
> Ack, I'm torn now.
> 
> I can see the utility of a dt method allowing us full access to dt functions
> from the template..  but I also love the idea of the 'pretty' syntax we
> could introduce with this approach.. `[% checkout.issuesdate | $KohaDates
> add => "3 days" %]`.. I'm overthinking.

I think both this bug and bug 33029 can live in harmony. This would be the "easy" mode to bug 33029's "advanced" mode.
Comment 5 Nick Clemens (kidclamp) 2023-03-16 12:49:55 UTC
Created attachment 148261 [details] [review]
Bug 33242: Allow passing add_{duration} options to KohaDates

For example, if I want to add a few days to the issuedate in a
TT template, I could do this:
    [% checkout.issuedate | $KohaDates add_days => 3 %]

This development allows you to pass add/subtract years, months, weeks,
days, hours, minutes and seconds.

Test plan:
Pick a notice like CHECKOUT.
Add a line like:
    [% checkout.date_due | $KohaDates with_hours => 1, add_minutes => 15 %]
Do a checkout. Verify that the notice generated contains a time that
shifted 15 minutes.
Undo your change.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens (kidclamp) 2023-03-16 12:49:57 UTC
Created attachment 148262 [details] [review]
Bug 33242: Add unit test

Test plan:
Run t/db_dependent/Letters/TemplateToolkit.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens (kidclamp) 2023-03-16 12:52:55 UTC
Created attachment 148264 [details] [review]
Bug 33242: Allow passing add_{duration} options to KohaDates

For example, if I want to add a few days to the issuedate in a
TT template, I could do this:
    [% checkout.issuedate | $KohaDates add_days => 3 %]

This development allows you to pass add/subtract years, months, weeks,
days, hours, minutes and seconds.

Test plan:
Pick a notice like CHECKOUT.
Add a line like:
    [% checkout.date_due | $KohaDates with_hours => 1, add_minutes => 15 %]
Do a checkout. Verify that the notice generated contains a time that
shifted 15 minutes.
Undo your change.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens (kidclamp) 2023-03-16 12:52:57 UTC
Created attachment 148265 [details] [review]
Bug 33242: Add unit test

Test plan:
Run t/db_dependent/Letters/TemplateToolkit.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens (kidclamp) 2023-03-16 12:54:19 UTC
Tested with 33029 (accidentally applied merged patches first time)
They both work well, I think having both provides nice flexibility, easy way to add to dates here, and full access to dt object from 33029
Comment 10 Jonathan Druart 2023-03-22 15:57:22 UTC
I was going to PQA until I saw bug 33029, which gives us more flexibility. Not sure we need both personally.
Comment 11 Marcel de Rooy 2023-03-22 16:06:09 UTC
(In reply to Jonathan Druart from comment #10)
> I was going to PQA until I saw bug 33029, which gives us more flexibility.
> Not sure we need both personally.

Yes we looked at that. Kyle, Nick and I concluded that both go fine together.
See description too, saying this:

Compare the 33029 approach
    [% KohaDates.datetime_from_string(checkout.issuedate).add( days => 3) | $KohaDates %]
with the approch of this report
    [% checkout.issuedate | $KohaDates add_days => 3 %]

The latter notation is definitely shorter and prettier in my use case and does only need a few lines more.
Comment 12 Jonathan Druart 2023-03-22 16:12:56 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > I was going to PQA until I saw bug 33029, which gives us more flexibility.
> > Not sure we need both personally.
> 
> Yes we looked at that. Kyle, Nick and I concluded that both go fine together.
> See description too, saying this:
> 
> Compare the 33029 approach
>     [% KohaDates.datetime_from_string(checkout.issuedate).add( days => 3) |
> $KohaDates %]
> with the approch of this report
>     [% checkout.issuedate | $KohaDates add_days => 3 %]
> 
> The latter notation is definitely shorter and prettier in my use case and
> does only need a few lines more.

Yes, more code to maintain, and the parsing here looks hacky to me, for less flexibility.

Note that I didn't FQA but I am not the one who is going to PQA this ;)
Comment 13 Marcel de Rooy 2023-03-22 17:37:27 UTC
(In reply to Jonathan Druart from comment #12)
> Yes, more code to maintain, and the parsing here looks hacky to me, for less
> flexibility.

What is hacky in this parsing ?
if( $entry =~ /^(add|subtract)_(years|months|weeks|days|hours|minutes|seconds)$/ ) {
Comment 14 Jonathan Druart 2023-03-23 08:10:52 UTC
(In reply to Marcel de Rooy from comment #13)
> (In reply to Jonathan Druart from comment #12)
> > Yes, more code to maintain, and the parsing here looks hacky to me, for less
> > flexibility.
> 
> What is hacky in this parsing ?
> if( $entry =~
> /^(add|subtract)_(years|months|weeks|days|hours|minutes|seconds)$/ ) {

Here you are providing an handy way to do date calculation, but without its flexibility. It's all what I am saying.

For instance what about the end_of_month limit? You cannot change it.

But really, if others find it useful, just let it go ;)
Comment 15 Kyle M Hall 2023-04-28 15:43:47 UTC
Created attachment 150390 [details] [review]
Bug 33242: Allow passing add_{duration} options to KohaDates

For example, if I want to add a few days to the issuedate in a
TT template, I could do this:
    [% checkout.issuedate | $KohaDates add_days => 3 %]

This development allows you to pass add/subtract years, months, weeks,
days, hours, minutes and seconds.

Test plan:
Pick a notice like CHECKOUT.
Add a line like:
    [% checkout.date_due | $KohaDates with_hours => 1, add_minutes => 15 %]
Do a checkout. Verify that the notice generated contains a time that
shifted 15 minutes.
Undo your change.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2023-04-28 15:50:34 UTC
Created attachment 150391 [details] [review]
Bug 33242: Allow passing add_{duration} options to KohaDates

For example, if I want to add a few days to the issuedate in a
TT template, I could do this:
    [% checkout.issuedate | $KohaDates add_days => 3 %]

This development allows you to pass add/subtract years, months, weeks,
days, hours, minutes and seconds.

Test plan:
Pick a notice like CHECKOUT.
Add a line like:
    [% checkout.date_due | $KohaDates with_hours => 1, add_minutes => 15 %]
Do a checkout. Verify that the notice generated contains a time that
shifted 15 minutes.
Undo your change.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2023-04-28 15:50:47 UTC
Created attachment 150392 [details] [review]
Bug 33242: Add unit test

Test plan:
Run t/db_dependent/Letters/TemplateToolkit.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Marcel de Rooy 2023-05-01 07:03:00 UTC
(In reply to Kyle M Hall from comment #17)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thx !
Comment 19 Tomás Cohen Arazi 2023-06-06 13:18:03 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Tomás Cohen Arazi 2023-06-09 14:44:38 UTC
Enhancement won't be backported to stable release.