Bug 35788

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.

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.

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>

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>

Jonathan: if you agree with the follow-up, I'd say this is PQA.

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.

(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.

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.

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

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

Branch pushed to https://github.com/PTFS-Europe/koha/tree/bug_35788

Pushed for 24.05!

Well done everyone, thank you!

Enhancement not pushed to 23.11.x

Architecture change, nothing to add/edit in the Koha manual.