Bug 38842 - Refactor modal logic in returns.tt / returns.pl
Summary: Refactor modal logic in returns.tt / returns.pl
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Emily Lamancusa (emlam)
QA Contact: Paul Derscheid
URL:
Keywords: RM_priority
Depends on: 38588 38789 38793
Blocks: 39613
  Show dependency treegraph
 
Reported: 2025-01-07 17:21 UTC by Emily Lamancusa (emlam)
Modified: 2025-04-11 16:48 UTC (History)
4 users (show)

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


Attachments
Bug 38842: Add reusable modal wrapper (7.77 KB, patch)
2025-02-28 21:13 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Add reusable modal wrapper (7.88 KB, patch)
2025-03-07 17:34 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Add block to DRY out checkin page pass-through data (2.29 KB, patch)
2025-03-07 17:34 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Use new reusable components for hold-found2 modal (18.44 KB, patch)
2025-03-07 17:34 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Use new reusable components for hold-found2 modal (18.32 KB, patch)
2025-03-07 17:37 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: (follow-up) Fix translatability (7.31 KB, patch)
2025-03-10 16:46 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Add reusable modal wrapper (7.88 KB, patch)
2025-03-20 13:13 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Add block to DRY out checkin page pass-through data (2.29 KB, patch)
2025-03-20 13:13 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Use new reusable components for hold-found2 modal (19.95 KB, patch)
2025-03-20 13:13 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: (follow-up) Fix translatability (7.31 KB, patch)
2025-03-20 13:13 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: (follow-up) Fix TT filters in modal_wrapper (9.18 KB, patch)
2025-03-20 13:13 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38842: Add reusable modal wrapper (7.94 KB, patch)
2025-03-20 15:22 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38842: Add block to DRY out checkin page pass-through data (2.35 KB, patch)
2025-03-20 15:23 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38842: Use new reusable components for hold-found2 modal (20.01 KB, patch)
2025-03-20 15:23 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38842: (follow-up) Fix translatability (7.37 KB, patch)
2025-03-20 15:23 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38842: (follow-up) Fix TT filters in modal_wrapper (9.24 KB, patch)
2025-03-20 15:23 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38842: Add reusable modal wrapper (8.00 KB, patch)
2025-04-10 08:25 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38842: Add block to DRY out checkin page pass-through data (2.41 KB, patch)
2025-04-10 08:25 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38842: Use new reusable components for hold-found2 modal (20.08 KB, patch)
2025-04-10 08:25 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38842: (follow-up) Fix translatability (7.43 KB, patch)
2025-04-10 08:25 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38842: (follow-up) Fix TT filters in modal_wrapper (9.30 KB, patch)
2025-04-10 08:25 UTC, Paul Derscheid
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa (emlam) 2025-01-07 17:21:49 UTC
The logic for displaying modals on the checkin page is badly in need of refactoring. There are 10 separate modals coded in returns.tt, plus an additional 2 modal include files. There is also at least one parameter (found) that is duplicated for completely different meanings in different contexts, and several other similar-sounding parameters in different contexts whose meaning is no longer clear without code tracing how the value was assigned.

All of this leads to a lot of code duplication and error-prone code, and makes it very confusing to follow the template/controller logic when developing or debugging. We should refactor down to one modal with clear logic for filling in its contents.

See also comments on bug 38588
Comment 1 Emily Lamancusa (emlam) 2025-02-28 21:13:57 UTC Comment hidden (obsolete)
Comment 2 Emily Lamancusa (emlam) 2025-02-28 21:27:02 UTC
Work in progress for a reusable modal wrapper - posting for feedback!

I tried to walk the line between flexibility and reusability, but I'm definitely open to feedback on the overall structure and how data is passed.

I wasn't really sure about how to do the print button, since existing modals seem to do it in many different ways. Is a standardized print button realistic, or are different methods really necessary depending on the circumstances?

See the commit message for usage. Most inputs that should be included in the modal form should usually be included in the modal body, though modal_footer_append can be used to add additional inputs into the footer. The <form> tag itself should NOT be included in the modal body; it is already included in the wrapper.

Most components within the modal have a predictable id, based on the modal_id parameter, so that they can easily be targeted with Javascript.

Any feedback is welcome and appreciated!
Comment 3 Emily Lamancusa (emlam) 2025-03-07 17:34:39 UTC Comment hidden (obsolete)
Comment 4 Emily Lamancusa (emlam) 2025-03-07 17:34:41 UTC Comment hidden (obsolete)
Comment 5 Emily Lamancusa (emlam) 2025-03-07 17:34:43 UTC Comment hidden (obsolete)
Comment 6 Emily Lamancusa (emlam) 2025-03-07 17:37:55 UTC Comment hidden (obsolete)
Comment 7 Emily Lamancusa (emlam) 2025-03-07 17:41:35 UTC
I think this is ready for testing and code review as a proof of concept. When this code is agreed upon, I'll make a follow-up bug to refactor the rest of the modals on returns.tt
Comment 8 Owen Leonard 2025-03-08 15:08:25 UTC
Taking a quick look at the patch... This will not be translatable text:

[% confirm_button_text = 'Confirm hold and transfer' %]

This works:

[% confirm_button_text = t('Confirm hold and transfer') %]

This also will not work:

[% print_button = '<button type="button" class="btn btn-default print" accesskey="p"> <i class="fa fa-print"></i> Print slip, transfer, and confirm (P) </button>' %]

I think it would work to concatenate the elements:

[% print_button = '<button type="button" class="btn btn-default print" accesskey="p"> <i class="fa fa-print"></i> ' _ t('Print slip, transfer, and confirm') _ ' (P) </button>' %]
Comment 9 Emily Lamancusa (emlam) 2025-03-10 16:46:52 UTC
Created attachment 179127 [details] [review]
Bug 38842: (follow-up) Fix translatability

Add t() calls to make text translatable
Comment 10 Emily Lamancusa (emlam) 2025-03-10 16:49:15 UTC
Thanks for catching that, Owen!

I submitted a follow-up patch to make the text translatable. Setting back to NSO
Comment 11 Jonathan Druart 2025-03-19 14:47:33 UTC
You should use the uri/html filters when needed, not always $raw.
Comment 12 Emily Lamancusa (emlam) 2025-03-20 12:30:43 UTC
(In reply to Jonathan Druart from comment #11)
> You should use the uri/html filters when needed, not always $raw.

Oop, thanks for catching that. Some of them do need to be $raw as the caller needs to be able to pass in a complete url string or HTML, but you're right, a lot of them don't need to be. I'll upload a follow-up patch shortly.
Comment 13 Emily Lamancusa (emlam) 2025-03-20 13:13:33 UTC
Created attachment 179520 [details] [review]
Bug 38842: Add reusable modal wrapper

This patch adds the block modal_wrapper to html_helpers.inc.

NOTE: The caller is responsible for sanitizing all data inputs.

Minimal usage:
[% WRAPPER modal_wrapper modal_id=<id> modal_header_contents=<contents> %]
    <modal body goes here>
[ % END %]

The above will produce an informational modal with an "OK" button to
dismiss, and no actions taken.

Optional parameters to add functionality to the modal:
noblock=1 - if the modal should not block
modal_action - if the modal should submit a form, the action url for
               the form

To add a "confirm" button that will submit the modal form:
modal_confirm_text - text to show on the button (must be specified)
modal_confirm_name - (optional) specify a name attribute
modal_confirm_val - (optional) specify a value attribute
modal_confirm_accesskey - (optional) specify an accesskey attribute

To add a "print" button that will open a print window to the specified url:
modal_print_text - text to show on the button (must be specified)
modal_print_url - url of the page to print (must be specified)
modal_print_name - (optional) specify a name attribute
modal_print_accesskey - (optional) specify an accesskey attribute

To add a "cancel/deny/dismiss" button:
modal_deny_text - text to show on the button (must be specified)
modal_deny_action - "submit" if the deny button should submit the modal
                     (to take some denial/cancellation action)
                  - "dismiss" if the deny button should dismiss the
                     modal without submitting the form
                  - This parameter can be omitted if the button should
                    neither submit nor dismiss the modal (for example,
                    if the button click will be handled with JS)
modal_deny_name - (optional) specify a name attribute
modal_deny_val - (optional) specify a value attribute
modal_deny_accesskey - (optional) specify an accesskey attribute

If the above generic buttons do not suffice, the below parameters can be
used in addition or instead of the above options to specify custom
button code and/or other customized footer content:
modal_custom_confirm
modal_custom_utility_button
modal_custom_deny
modal_footer_append
Comment 14 Emily Lamancusa (emlam) 2025-03-20 13:13:36 UTC
Created attachment 179521 [details] [review]
Bug 38842: Add block to DRY out checkin page pass-through data

There are a number of variable values that need to get passed back to
the controller after submitting a modal to maintain the checkin page's
state. Currently, all of these inputs are manually coded for each modal.
Add a Template Toolkit block that can be used instead to add all of
these mandatory "pass-through" values.
Comment 15 Emily Lamancusa (emlam) 2025-03-20 13:13:38 UTC
Created attachment 179523 [details] [review]
Bug 38842: Use new reusable components for hold-found2 modal

Refactor the code for hold-found2 to use the new modal_wrapper and
passthrough_data block as a proof of concept.

The modal is also renamed hold-found-modal.

To test:
Setup:
0. Set system preferences:
    - HoldsAutoFill = Don't
    - ShowAllCheckins = Show
1. Find a biblio record with multiple items
2. Set the homebranch on all of those items to the logged-in branch
3. Place two holds on that record:
    - One for pickup at the logged-in branch (Hold A)
    - One for pickup at a different branch (Hold B)
4. Keep the tab for the biblio record open, and open a new tab for
   checkins
5. Check in an item from that bib
6. Take a screenshot of the modal on main for easy comparison
7. Ignore the hold
8. Apply patches

Test a hold for pickup at the logged-in branch:
9. Check in the item again
--> Confirm that the new modal contains all the correct information for
Hold A. The only change in the modal should be that the title is now
in the modal body instead of the modal header.
10. Open all of the links in the modal body in new tabs.
--> Confirm the links all work as expected, then close those tabs
11. Click to ignore the hold
--> Confirm that the checkins table displays correctly
12. Reload the Holds tab of the biblio record
--> Confirm the holds are unchanged
13. Switch to the Normal tab of the biblio record
--> Confirm the item still shows as Available
14. Check in the item again and click the button to confirm the hold
--> Confirm that the checkins table displays correctly
15. Reload the Holds tab of the biblio record
--> Confirm Hold A is now set to Waiting
16. Revert the waiting status on Hold A
17. Check in the item again
18. Click the button to confirm and print slip
--> Confirm the print dialog appears and slip information is correct
19. Cancel the print dialog
--> Confirm that the checkins table displays correctly
20. Reload the Holds tab of the biblio record
--> Confirm Hold A is now set to Waiting

Test a hold for pickup at a different branch:
21. Check in a different item from that bib
--> Confirm that the new modal contains all the correct information for
Hold B, and correctly states that the item needs to be transferred
22. Click to ignore the hold
--> Confirm that the checkins table displays correctly
23. Reload the Holds tab of the biblio record
--> Confirm the hold is unchanged
24. Switch to the Normal tab of the biblio record
--> Confirm the item still shows as Available
25. Check in the item again and click the button to confirm and transfer
--> Confirm that the checkins table displays correctly
26. Reload the Holds tab of the biblio record
--> Confirm Hold B is now set to in-transit
27. Revert the transit status on Hold B
28. Check in the item again
29. Click the button to print slip, transfer, and confirm
--> Confirm the print dialog appears and slip information is correct
30. Cancel the print dialog
--> Confirm that the checkins table displays correctly
31. Reload the Holds tab of the biblio record
--> Confirm Hold B is now set to in-transit
32. Change your logged-in branch to match Hold B's pickup location
33. Check in the item again
--> Confirm the modal still contains all the correct information for
Hold B and does not say it needs to be transferred
34. Click to print slip and confirm
--> Confirm that the print dialog appears and looks correct
35. Cancel the print dialog
36. Reload the Holds tab of the biblio record
--> Confirm that both holds are now Waiting
37. Sign off :D
Comment 16 Emily Lamancusa (emlam) 2025-03-20 13:13:40 UTC
Created attachment 179524 [details] [review]
Bug 38842: (follow-up) Fix translatability

Add t() calls to make text translatable
Comment 17 Emily Lamancusa (emlam) 2025-03-20 13:13:42 UTC
Created attachment 179526 [details] [review]
Bug 38842: (follow-up) Fix TT filters in modal_wrapper

Use html filters for most of the parameter variables, when possible.

The following parameters do need to be used raw, as they are expected to
contain HTML code or complete url strings:
modal_action
modal_custom_confirm
modal_print_url
modal_custom_utility_button
modal_custom_deny
modal_footer_append

If the caller includes any inputs from elsewhere in the above
parameters, the caller must filter those inputs using the appropriate
TT filter.

This patch also adjusts the handling of header data so that only plain
text should be passed.
Comment 18 Baptiste Wojtkowski (bwoj) 2025-03-20 15:22:57 UTC
Created attachment 179538 [details] [review]
Bug 38842: Add reusable modal wrapper

This patch adds the block modal_wrapper to html_helpers.inc.

NOTE: The caller is responsible for sanitizing all data inputs.

Minimal usage:
[% WRAPPER modal_wrapper modal_id=<id> modal_header_contents=<contents> %]
    <modal body goes here>
[ % END %]

The above will produce an informational modal with an "OK" button to
dismiss, and no actions taken.

Optional parameters to add functionality to the modal:
noblock=1 - if the modal should not block
modal_action - if the modal should submit a form, the action url for
               the form

To add a "confirm" button that will submit the modal form:
modal_confirm_text - text to show on the button (must be specified)
modal_confirm_name - (optional) specify a name attribute
modal_confirm_val - (optional) specify a value attribute
modal_confirm_accesskey - (optional) specify an accesskey attribute

To add a "print" button that will open a print window to the specified url:
modal_print_text - text to show on the button (must be specified)
modal_print_url - url of the page to print (must be specified)
modal_print_name - (optional) specify a name attribute
modal_print_accesskey - (optional) specify an accesskey attribute

To add a "cancel/deny/dismiss" button:
modal_deny_text - text to show on the button (must be specified)
modal_deny_action - "submit" if the deny button should submit the modal
                     (to take some denial/cancellation action)
                  - "dismiss" if the deny button should dismiss the
                     modal without submitting the form
                  - This parameter can be omitted if the button should
                    neither submit nor dismiss the modal (for example,
                    if the button click will be handled with JS)
modal_deny_name - (optional) specify a name attribute
modal_deny_val - (optional) specify a value attribute
modal_deny_accesskey - (optional) specify an accesskey attribute

If the above generic buttons do not suffice, the below parameters can be
used in addition or instead of the above options to specify custom
button code and/or other customized footer content:
modal_custom_confirm
modal_custom_utility_button
modal_custom_deny
modal_footer_append

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 19 Baptiste Wojtkowski (bwoj) 2025-03-20 15:23:00 UTC
Created attachment 179539 [details] [review]
Bug 38842: Add block to DRY out checkin page pass-through data

There are a number of variable values that need to get passed back to
the controller after submitting a modal to maintain the checkin page's
state. Currently, all of these inputs are manually coded for each modal.
Add a Template Toolkit block that can be used instead to add all of
these mandatory "pass-through" values.

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 20 Baptiste Wojtkowski (bwoj) 2025-03-20 15:23:04 UTC
Created attachment 179540 [details] [review]
Bug 38842: Use new reusable components for hold-found2 modal

Refactor the code for hold-found2 to use the new modal_wrapper and
passthrough_data block as a proof of concept.

The modal is also renamed hold-found-modal.

To test:
Setup:
0. Set system preferences:
    - HoldsAutoFill = Don't
    - ShowAllCheckins = Show
1. Find a biblio record with multiple items
2. Set the homebranch on all of those items to the logged-in branch
3. Place two holds on that record:
    - One for pickup at the logged-in branch (Hold A)
    - One for pickup at a different branch (Hold B)
4. Keep the tab for the biblio record open, and open a new tab for
   checkins
5. Check in an item from that bib
6. Take a screenshot of the modal on main for easy comparison
7. Ignore the hold
8. Apply patches

Test a hold for pickup at the logged-in branch:
9. Check in the item again
--> Confirm that the new modal contains all the correct information for
Hold A. The only change in the modal should be that the title is now
in the modal body instead of the modal header.
10. Open all of the links in the modal body in new tabs.
--> Confirm the links all work as expected, then close those tabs
11. Click to ignore the hold
--> Confirm that the checkins table displays correctly
12. Reload the Holds tab of the biblio record
--> Confirm the holds are unchanged
13. Switch to the Normal tab of the biblio record
--> Confirm the item still shows as Available
14. Check in the item again and click the button to confirm the hold
--> Confirm that the checkins table displays correctly
15. Reload the Holds tab of the biblio record
--> Confirm Hold A is now set to Waiting
16. Revert the waiting status on Hold A
17. Check in the item again
18. Click the button to confirm and print slip
--> Confirm the print dialog appears and slip information is correct
19. Cancel the print dialog
--> Confirm that the checkins table displays correctly
20. Reload the Holds tab of the biblio record
--> Confirm Hold A is now set to Waiting

Test a hold for pickup at a different branch:
21. Check in a different item from that bib
--> Confirm that the new modal contains all the correct information for
Hold B, and correctly states that the item needs to be transferred
22. Click to ignore the hold
--> Confirm that the checkins table displays correctly
23. Reload the Holds tab of the biblio record
--> Confirm the hold is unchanged
24. Switch to the Normal tab of the biblio record
--> Confirm the item still shows as Available
25. Check in the item again and click the button to confirm and transfer
--> Confirm that the checkins table displays correctly
26. Reload the Holds tab of the biblio record
--> Confirm Hold B is now set to in-transit
27. Revert the transit status on Hold B
28. Check in the item again
29. Click the button to print slip, transfer, and confirm
--> Confirm the print dialog appears and slip information is correct
30. Cancel the print dialog
--> Confirm that the checkins table displays correctly
31. Reload the Holds tab of the biblio record
--> Confirm Hold B is now set to in-transit
32. Change your logged-in branch to match Hold B's pickup location
33. Check in the item again
--> Confirm the modal still contains all the correct information for
Hold B and does not say it needs to be transferred
34. Click to print slip and confirm
--> Confirm that the print dialog appears and looks correct
35. Cancel the print dialog
36. Reload the Holds tab of the biblio record
--> Confirm that both holds are now Waiting
37. Sign off :D

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 21 Baptiste Wojtkowski (bwoj) 2025-03-20 15:23:07 UTC
Created attachment 179541 [details] [review]
Bug 38842: (follow-up) Fix translatability

Add t() calls to make text translatable

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 22 Baptiste Wojtkowski (bwoj) 2025-03-20 15:23:10 UTC
Created attachment 179542 [details] [review]
Bug 38842: (follow-up) Fix TT filters in modal_wrapper

Use html filters for most of the parameter variables, when possible.

The following parameters do need to be used raw, as they are expected to
contain HTML code or complete url strings:
modal_action
modal_custom_confirm
modal_print_url
modal_custom_utility_button
modal_custom_deny
modal_footer_append

If the caller includes any inputs from elsewhere in the above
parameters, the caller must filter those inputs using the appropriate
TT filter.

This patch also adjusts the handling of header data so that only plain
text should be passed.

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 23 Paul Derscheid 2025-04-10 08:25:45 UTC
Created attachment 180752 [details] [review]
Bug 38842: Add reusable modal wrapper

This patch adds the block modal_wrapper to html_helpers.inc.

NOTE: The caller is responsible for sanitizing all data inputs.

Minimal usage:
[% WRAPPER modal_wrapper modal_id=<id> modal_header_contents=<contents> %]
    <modal body goes here>
[ % END %]

The above will produce an informational modal with an "OK" button to
dismiss, and no actions taken.

Optional parameters to add functionality to the modal:
noblock=1 - if the modal should not block
modal_action - if the modal should submit a form, the action url for
               the form

To add a "confirm" button that will submit the modal form:
modal_confirm_text - text to show on the button (must be specified)
modal_confirm_name - (optional) specify a name attribute
modal_confirm_val - (optional) specify a value attribute
modal_confirm_accesskey - (optional) specify an accesskey attribute

To add a "print" button that will open a print window to the specified url:
modal_print_text - text to show on the button (must be specified)
modal_print_url - url of the page to print (must be specified)
modal_print_name - (optional) specify a name attribute
modal_print_accesskey - (optional) specify an accesskey attribute

To add a "cancel/deny/dismiss" button:
modal_deny_text - text to show on the button (must be specified)
modal_deny_action - "submit" if the deny button should submit the modal
                     (to take some denial/cancellation action)
                  - "dismiss" if the deny button should dismiss the
                     modal without submitting the form
                  - This parameter can be omitted if the button should
                    neither submit nor dismiss the modal (for example,
                    if the button click will be handled with JS)
modal_deny_name - (optional) specify a name attribute
modal_deny_val - (optional) specify a value attribute
modal_deny_accesskey - (optional) specify an accesskey attribute

If the above generic buttons do not suffice, the below parameters can be
used in addition or instead of the above options to specify custom
button code and/or other customized footer content:
modal_custom_confirm
modal_custom_utility_button
modal_custom_deny
modal_footer_append

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 24 Paul Derscheid 2025-04-10 08:25:48 UTC
Created attachment 180753 [details] [review]
Bug 38842: Add block to DRY out checkin page pass-through data

There are a number of variable values that need to get passed back to
the controller after submitting a modal to maintain the checkin page's
state. Currently, all of these inputs are manually coded for each modal.
Add a Template Toolkit block that can be used instead to add all of
these mandatory "pass-through" values.

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 25 Paul Derscheid 2025-04-10 08:25:51 UTC
Created attachment 180754 [details] [review]
Bug 38842: Use new reusable components for hold-found2 modal

Refactor the code for hold-found2 to use the new modal_wrapper and
passthrough_data block as a proof of concept.

The modal is also renamed hold-found-modal.

To test:
Setup:
0. Set system preferences:
    - HoldsAutoFill = Don't
    - ShowAllCheckins = Show
1. Find a biblio record with multiple items
2. Set the homebranch on all of those items to the logged-in branch
3. Place two holds on that record:
    - One for pickup at the logged-in branch (Hold A)
    - One for pickup at a different branch (Hold B)
4. Keep the tab for the biblio record open, and open a new tab for
   checkins
5. Check in an item from that bib
6. Take a screenshot of the modal on main for easy comparison
7. Ignore the hold
8. Apply patches

Test a hold for pickup at the logged-in branch:
9. Check in the item again
--> Confirm that the new modal contains all the correct information for
Hold A. The only change in the modal should be that the title is now
in the modal body instead of the modal header.
10. Open all of the links in the modal body in new tabs.
--> Confirm the links all work as expected, then close those tabs
11. Click to ignore the hold
--> Confirm that the checkins table displays correctly
12. Reload the Holds tab of the biblio record
--> Confirm the holds are unchanged
13. Switch to the Normal tab of the biblio record
--> Confirm the item still shows as Available
14. Check in the item again and click the button to confirm the hold
--> Confirm that the checkins table displays correctly
15. Reload the Holds tab of the biblio record
--> Confirm Hold A is now set to Waiting
16. Revert the waiting status on Hold A
17. Check in the item again
18. Click the button to confirm and print slip
--> Confirm the print dialog appears and slip information is correct
19. Cancel the print dialog
--> Confirm that the checkins table displays correctly
20. Reload the Holds tab of the biblio record
--> Confirm Hold A is now set to Waiting

Test a hold for pickup at a different branch:
21. Check in a different item from that bib
--> Confirm that the new modal contains all the correct information for
Hold B, and correctly states that the item needs to be transferred
22. Click to ignore the hold
--> Confirm that the checkins table displays correctly
23. Reload the Holds tab of the biblio record
--> Confirm the hold is unchanged
24. Switch to the Normal tab of the biblio record
--> Confirm the item still shows as Available
25. Check in the item again and click the button to confirm and transfer
--> Confirm that the checkins table displays correctly
26. Reload the Holds tab of the biblio record
--> Confirm Hold B is now set to in-transit
27. Revert the transit status on Hold B
28. Check in the item again
29. Click the button to print slip, transfer, and confirm
--> Confirm the print dialog appears and slip information is correct
30. Cancel the print dialog
--> Confirm that the checkins table displays correctly
31. Reload the Holds tab of the biblio record
--> Confirm Hold B is now set to in-transit
32. Change your logged-in branch to match Hold B's pickup location
33. Check in the item again
--> Confirm the modal still contains all the correct information for
Hold B and does not say it needs to be transferred
34. Click to print slip and confirm
--> Confirm that the print dialog appears and looks correct
35. Cancel the print dialog
36. Reload the Holds tab of the biblio record
--> Confirm that both holds are now Waiting
37. Sign off :D

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 26 Paul Derscheid 2025-04-10 08:25:53 UTC
Created attachment 180755 [details] [review]
Bug 38842: (follow-up) Fix translatability

Add t() calls to make text translatable

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 27 Paul Derscheid 2025-04-10 08:25:56 UTC
Created attachment 180756 [details] [review]
Bug 38842: (follow-up) Fix TT filters in modal_wrapper

Use html filters for most of the parameter variables, when possible.

The following parameters do need to be used raw, as they are expected to
contain HTML code or complete url strings:
modal_action
modal_custom_confirm
modal_print_url
modal_custom_utility_button
modal_custom_deny
modal_footer_append

If the caller includes any inputs from elsewhere in the above
parameters, the caller must filter those inputs using the appropriate
TT filter.

This patch also adjusts the handling of header data so that only plain
text should be passed.

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 28 Paul Derscheid 2025-04-10 08:34:06 UTC
This is a big leap for modals in Koha, the implementation of the wrapper is great, maybe still needs refinement in some places but not worth not PQAing this right now :)

I was starting to write a cypress test which would be nice but don't know how to do the syspref assertions. That probably requires selenium (ugh).

Just went over the test plan again after perusing the files and this does everything it says it does and is even a UI improvement compared to the previous iteration.

Great job Emily!

Some things I still crave:
- Similar documentation with one simple and one complex example following the style of the other bootstrap component wrappers in html_helpers.inc. 
Would be important for quickly using this wrapper to refactor, I think.
- Would be great if we had some kind of test in place to make sure we do not introduce regressions in later versions.