Bug 35788 - Remove Koha::Template::Plugin::Biblio::BookingsCount
Summary: Remove Koha::Template::Plugin::Biblio::BookingsCount
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL: https://github.com/PTFS-Europe/koha/t...
Keywords:
Depends on: 35787
Blocks: 35789 35248
  Show dependency treegraph
 
Reported: 2024-01-12 13:06 UTC by Jonathan Druart
Modified: 2024-03-18 09:01 UTC (History)
4 users (show)

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


Attachments
Bug 35788: Remove Koha::Template::Plugin::Biblio::BookingsCount (5.89 KB, patch)
2024-01-12 13:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35788: Remove Koha::Template::Plugin::Biblio::BookingsCount (5.89 KB, patch)
2024-01-15 13:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35788: Test for current day booking (1.49 KB, patch)
2024-01-15 13:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35788: Simplify code (1.59 KB, patch)
2024-01-15 13:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-01-12 13:06:56 UTC

    
Comment 1 Jonathan Druart 2024-01-12 13:23:22 UTC
Created attachment 160961 [details] [review]
Bug 35788: Remove Koha::Template::Plugin::Biblio::BookingsCount

We need a new method filter_by_future in Koha::Bookings.

Additionally this TT plugin's method was not covered by tests. Neither
is the whole Koha::Booking[s] objects btw.

Test plan:
Confirm that the number of bookings is displayed in the "Bookings" tab
in the left menu of the bibliographic record detail page.
Comment 2 Tomás Cohen Arazi 2024-01-15 13:47:55 UTC
Created attachment 161039 [details] [review]
Bug 35788: Remove Koha::Template::Plugin::Biblio::BookingsCount

We need a new method filter_by_future in Koha::Bookings.

Additionally this TT plugin's method was not covered by tests. Neither
is the whole Koha::Booking[s] objects btw.

Test plan:
Confirm that the number of bookings is displayed in the "Bookings" tab
in the left menu of the bibliographic record detail page.
Comment 3 Tomás Cohen Arazi 2024-01-15 13:47:58 UTC
Created attachment 161040 [details] [review]
Bug 35788: Test for current day booking

The implemented tests checked past and future dates for filtering
bookings. But `> NOW()` excludes the current date, and this case was not
covered. It is now.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2024-01-15 13:48:01 UTC
Created attachment 161041 [details] [review]
Bug 35788: Simplify code

We should not load DateUtils and deal with DateTime objects and
formatting when we can use the DB for it, using \'NOW()' should be
enough.

This patch makes that change. To test:

1. Apply the previous patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Bookings.t
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2024-01-15 17:51:28 UTC
Jonathan: if you agree with the follow-up, I'd say this is PQA.
Comment 6 Martin Renvoize 2024-01-15 19:23:57 UTC
I have a branch with tests in progress, they need submitting up.. I'll get in that.

Thanks for this patchset chaps, looking good to me.
Comment 7 Jonathan Druart 2024-01-16 09:10:49 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> Jonathan: if you agree with the follow-up, I'd say this is PQA.

I don't know, we never decided what was the correct pattern. I always use format_datetime and dt_from_string for this.
Comment 8 Martin Renvoize 2024-01-16 09:56:47 UTC
I think Jonathan is right here.. as much as I'd love to just lean on the database to handle this, it doesn't take our timezone handling into account.. These are DATETIME fields, not TIMESTAMPS.. so there's no internal munging of the dates to UTC and back.. we need the koha munging of that to consistently get back the datetimes we put in.
Comment 9 Martin Renvoize 2024-01-17 12:10:12 UTC
OK, I've been talked around having been shown the DB connection logic where we pass timezone information on connect.

I think the use of \'NOW()' is a little more performant so am happy with the follow-up.

I'd love to see a coding guideline around this at some point, it would be nice to clean up existing cases where we could use this and currently don't.  Consistency is good to aim for.

As such.. I'll switch to PQA
Comment 10 David Nind 2024-03-09 19:50:41 UTC
Patch doesn't apply - sha1/fake ancestor error:

...
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 35788: Remove Koha::Template::Plugin::Biblio::BookingsCount
error: sha1 information is lacking or useless (Koha/Template/Plugin/Biblio.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 35788: Remove Koha::Template::Plugin::Biblio::BookingsCount
Comment 11 Martin Renvoize 2024-03-11 08:09:21 UTC
Branch pushed to https://github.com/PTFS-Europe/koha/tree/bug_35788
Comment 12 Katrin Fischer 2024-03-15 08:40:07 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 13 Fridolin Somers 2024-03-18 09:01:29 UTC
Enhancement not pushed to 23.11.x