Bug 40656

Summary: bookings/list.tt needs to be refactored
Product: Koha Reporter: Paul Derscheid <paul.derscheid>
Component: CirculationAssignee: Paul Derscheid <paul.derscheid>
Status: Pushed to stable --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, kyle, lucas, martin.renvoize, paul.derscheid, thibaud.guillot
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00,25.05.04
Circulation function: Bookings
Bug Depends on: 40296    
Bug Blocks: 36789, 40838    
Attachments: Bug 40656: Refactor bookings/list.tt to use better patterns and improve code organization
Bug 40656: (follow-up) Create reusable additional filters library for DataTables
Bug 40656: (follow-up) Correct inverted logic for column visibility
Bug 40656: Refactor bookings/list.tt to use better patterns and improve code organization
Bug 40656: (follow-up) Create reusable additional filters library for DataTables
Bug 40656: (follow-up) Correct inverted logic for column visibility
Bug 40656: (follow-up) Always display Status column in bookings table
Bug 40656: (QA follow-up) Destroy formatting for QA script

Description Paul Derscheid 2025-08-15 10:57:54 UTC
When QAing Bug 40296 and looking at Bug 36789, I felt like it was time to clean some of the more wip bits up.

This includes:
- Inlined CSS
- Incomprehensible logic for the table filters (sorry, that was on me)
- QA complaints from Bug 36789
Comment 1 Paul Derscheid 2025-08-15 15:24:02 UTC
Created attachment 185468 [details] [review]
Bug 40656: Refactor bookings/list.tt to use better patterns and improve code organization

This patch aims to improve bookings/list.tt:
- Converting filter attributes to boolean data attributes (presence-based)
- Creating a reusable table-filters.inc for filter controls
- Moving inline styles to proper SCSS files for better caching and separation of concerns
- Fixing a runtime error when cancelling bookings from the timeline
- Improving JavaScript readability with helper functions
- Addressing the QA complaints from Bug 36789:
    - Cleaning up the long lines for the button elements by moving them into template literals
    - Using the format method instead of direct interpolation
    - Using escape_str for all values received from the API.

Test plan:
1. Apply the patch and compile css
2. Navigate to a biblio record with bookings (Catalog > Search > View record > Bookings)
3. Verify the filter buttons (Exclude expired/cancelled) appear correctly
4. Click each filter button and verify:
 - The button text toggles between "Include" and "Exclude"
 - The table updates to show/hide the appropriate bookings
 - The status column appears when any items are filtered
5. In the timeline view:
 - Verify cancelled bookings show with a strikethrough pattern
 - Try to cancel a booking by clicking the X on a timeline item
 - Confirm the cancel modal opens without JavaScript errors
6. Check that the table actions (Edit, Cancel) work correctly
7. Verify no inline styles remain in the page source
8. Confirm the filter styles are properly loaded from compiled CSS
Comment 2 Paul Derscheid 2025-08-15 15:24:04 UTC
Created attachment 185469 [details] [review]
Bug 40656: (follow-up) Create reusable additional filters library for DataTables

This follow-up patch extracts the filter functionality into a reusable library:
- Create AdditionalFilters.js library with chainable API for filter
functionality
- Rename table-filters.inc to additional-filters.inc for clarity
- Refactor to use explicit element IDs instead of interpolation for
better greppability (filter-expired vs filter-${id})
- Include proper cleanup methods (destroy) for memory management

The library provides a fluent API:
AdditionalFilters.init(['filter-expired', 'filter-cancelled'])
    .onChange((filters, { anyFiltersApplied }) => { ... })
    .build({ status: ({ filters, isNotApplied }) => ... });

Test plan remains identical except for applying both patches on step 1.
Comment 3 Paul Derscheid 2025-08-15 15:30:10 UTC
Created attachment 185470 [details] [review]
Bug 40656: (follow-up) Correct inverted logic for column visibility
Comment 4 Martin Renvoize (ashimema) 2025-08-29 06:49:35 UTC
Created attachment 185860 [details] [review]
Bug 40656: Refactor bookings/list.tt to use better patterns and improve code organization

This patch aims to improve bookings/list.tt:
- Converting filter attributes to boolean data attributes (presence-based)
- Creating a reusable table-filters.inc for filter controls
- Moving inline styles to proper SCSS files for better caching and separation of concerns
- Fixing a runtime error when cancelling bookings from the timeline
- Improving JavaScript readability with helper functions
- Addressing the QA complaints from Bug 36789:
    - Cleaning up the long lines for the button elements by moving them into template literals
    - Using the format method instead of direct interpolation
    - Using escape_str for all values received from the API.

Test plan:
1. Apply the patch and compile css
2. Navigate to a biblio record with bookings (Catalog > Search > View record > Bookings)
3. Verify the filter buttons (Exclude expired/cancelled) appear correctly
4. Click each filter button and verify:
 - The button text toggles between "Include" and "Exclude"
 - The table updates to show/hide the appropriate bookings
 - The status column appears when any items are filtered
5. In the timeline view:
 - Verify cancelled bookings show with a strikethrough pattern
 - Try to cancel a booking by clicking the X on a timeline item
 - Confirm the cancel modal opens without JavaScript errors
6. Check that the table actions (Edit, Cancel) work correctly
7. Verify no inline styles remain in the page source
8. Confirm the filter styles are properly loaded from compiled CSS

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 5 Martin Renvoize (ashimema) 2025-08-29 06:49:37 UTC
Created attachment 185861 [details] [review]
Bug 40656: (follow-up) Create reusable additional filters library for DataTables

This follow-up patch extracts the filter functionality into a reusable library:
- Create AdditionalFilters.js library with chainable API for filter
functionality
- Rename table-filters.inc to additional-filters.inc for clarity
- Refactor to use explicit element IDs instead of interpolation for
better greppability (filter-expired vs filter-${id})
- Include proper cleanup methods (destroy) for memory management

The library provides a fluent API:
AdditionalFilters.init(['filter-expired', 'filter-cancelled'])
    .onChange((filters, { anyFiltersApplied }) => { ... })
    .build({ status: ({ filters, isNotApplied }) => ... });

Test plan remains identical except for applying both patches on step 1.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 6 Martin Renvoize (ashimema) 2025-08-29 06:49:40 UTC
Created attachment 185862 [details] [review]
Bug 40656: (follow-up) Correct inverted logic for column visibility

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 7 Martin Renvoize (ashimema) 2025-08-29 06:49:42 UTC
Created attachment 185863 [details] [review]
Bug 40656: (follow-up) Always display Status column in bookings table

The Status column was previously only visible when filters were applied,
making the interface inconsistent and potentially confusing. This change
ensures the Status column is always displayed regardless of filter state.

Test plan:
1. Navigate to a biblio record with bookings
2. Go to the Bookings tab
3. Verify the Status column is visible in the bookings table
4. Apply filters (Include expired/Include cancelled)
5. Verify the Status column remains visible
6. Remove all filters
7. Confirm the Status column is still visible and shows appropriate
   status badges (New, Pending, Active, etc.)

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 8 Martin Renvoize (ashimema) 2025-08-29 06:49:44 UTC
Created attachment 185864 [details] [review]
Bug 40656: (QA follow-up) Destroy formatting for QA script

We really need a better fix at the translation script level for bug
18675. This change makes the template so much less readable!

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 9 Martin Renvoize (ashimema) 2025-08-29 06:51:09 UTC
This is some great cleanup work.. going straight for a QA run as one of the key bookings code players I feel I can.

Passing
Comment 10 Lucas Gass (lukeg) 2025-09-03 20:02:41 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 11 Paul Derscheid 2025-09-20 08:59:19 UTC
Nice work everyone!

Pushed to 25.05.x