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
Created attachment 178875 [details] [review] Bug 38842: Add reusable modal wrapper This patch adds the block modal_wrapper to html_helpers.inc. 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
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!
Created attachment 179084 [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
Created attachment 179085 [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.
Created attachment 179086 [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
Created attachment 179087 [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
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
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>' %]
Created attachment 179127 [details] [review] Bug 38842: (follow-up) Fix translatability Add t() calls to make text translatable
Thanks for catching that, Owen! I submitted a follow-up patch to make the text translatable. Setting back to NSO
You should use the uri/html filters when needed, not always $raw.
(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.
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
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.
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
Created attachment 179524 [details] [review] Bug 38842: (follow-up) Fix translatability Add t() calls to make text translatable
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.