Many libraries in my system lend DVDs out for a shorter period than their normal checkout period, like 2 days versus 14 or 21 days for other items. They also charge larger fines on DVDs than other items. They would like to be able to generate overdue notices for DVDs on the first day overdue, unlike the first notice generation of 7 days on their other items. So this is an enhancement request to allow libraries to generate an overdue notice for a particular item type separate from the three notice triggers.
This is standard behavior now, right?
Right now we can only set the overdue notices by patron category, but we can't differentiate by item type. I think this is still valid.
Here in 2017 still wishing for greater flexibility for overdue notices based on Item Type, not only Patron Category -- in our case (school library) for better communication about items on Reserve (in our case loan period is 2 hours).
+1
This is important in our consortium of mainly public libraries as our Library of Things (LoT) items go out for shorter periods of time and are overdue on a different time schedule to regular items like books. We would like overdues to be based on item type not just patron type. We also have one college member library in our consortium and their items go out on for different time periods again based on Item type not just patron type.
Matt and I are working on this together.. we're not too far off submitting code as per the spec above.. getting the API patches linked above through would certainly help it keep marching forward.
Created attachment 170357 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group
Created attachment 170358 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group
Created attachment 170359 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group
Created attachment 170360 [details] [review] Bug 10190: Add a vue app for circulation triggers This patch adds a new vue based page for administering circulation triggers. This aims to replace the 'Overdue notice/status triggers' page available from Tools. The new page is available from Koha administration > Circulation triggers Sponsored-by: Glasgow Colleges Library Group
Created attachment 170361 [details] [review] Bug 10190: Add components to list/add and edit circulation triggers These new components are used by the new 'Circulation triggers' administration page to display existing trigger rules and add the ability to add new rules and edit existing ones. Sponsored-by: Glasgow Colleges Library Group
Created attachment 170362 [details] [review] Bug 10190: Add a library API client This patch adds a library api client to the vue ecosystem for later use in the Circulation triggers page. Sponsored-by: Glasgow Colleges Library Group
Created attachment 170363 [details] [review] Bug 10190: Add itemtypes and patron categories to the item/patron api clients This patch adds patron and itemtype api clients for the Vue ecosystem Sponsored-by: Glasgow Colleges Library Group
Created attachment 170364 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group
Created attachment 170365 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group
Created attachment 170366 [details] [review] Bug 10190: Update overdue_notices script to refer to circ rules This patch updates the overdue_notices cron script to refer to the new location for overdue rules found in the circulation rules table now. Sponsored-by: Glasgow Colleges Library Group
Created attachment 170367 [details] [review] Bug 10190: Add itemtype handling to overdue_notices script This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. Sponsored-by: Glasgow Colleges Library Group
Created attachment 170368 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group
Created attachment 170369 [details] [review] Bug 10190: Add letter names to tables Sponsored-by: Glasgow Colleges Library Group
Created attachment 170370 [details] [review] Bug 10190: Set tab based on trigger edited/created Sponsored-by: Glasgow Colleges Library Group
Created attachment 170371 [details] [review] Bug 10190: Add fallback to 'Default' text for notice Sponsored-by: Glasgow Colleges Library Group
Created attachment 170372 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group
Created attachment 170373 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group
This is going to be fun to test.. Matt and I worked hard together on it, so whilst it's assigned to me we're both available to comment. I'm happy to walk through intentions in a Jistsi or whatever.. reach out on chat.koha-community.org if you want any guidance.
Created attachment 170374 [details] [review] Bug 10190: Drop overduerules and overduerules_transport_types
Created attachment 170375 [details] [review] Bug 10190: DBIC Schema update
Created attachment 170376 [details] [review] Bug 10190: Remove OverdueRule Koha Objects
Created attachment 170380 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules
Whilst I've updated the itiva script to reflect the move to circulation rules.. I've not got any way to really test this. We also need to expose itiva in the new admin page.. I'm going to ask Matt to take a look at that.
Created attachment 170449 [details] [review] Bug 10190: Add comments to overdue_notices and rename variable Trying to fully understand how this script operates.. this patch adds some notes to the script for developers and also renames a variable that I found was being re-used.
https://github.com/PTFS-Europe/koha/tree/bug_10190_submission
I'm still cleaning up the commits and Matt and I are working on adding tests. But, it's all working pretty well now and testing is ongoing in production at a customer site. I've set up a sandbox for anyone wanting to have a play: https://staff-overdues.sandboxes.ptfs-europe.co.uk/ Test plan: 1) Head on over to either 'Tools > Overdue notice/status triggers' or 'Administration > Circulation triggers' (They're the same page) 2) Add a base ALL, ALL, ALL rule 3) Add rule overrides for various combinations of Library, Patron category and Item type
Created attachment 171727 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group
Created attachment 171728 [details] [review] Bug 10190: DBIC Schema update
Created attachment 171729 [details] [review] Bug 10190: Remove OverdueRule Koha Objects
Created attachment 171730 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group
Created attachment 171731 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group
Created attachment 171732 [details] [review] Bug 10190: Add a vue app for circulation triggers This patch adds a new vue based page for administering circulation triggers. This aims to replace the 'Overdue notice/status triggers' page available from Tools. The new page is available from Koha administration > Circulation triggers Sponsored-by: Glasgow Colleges Library Group
Created attachment 171733 [details] [review] Bug 10190: Add components to list/add and edit circulation triggers These new components are used by the new 'Circulation triggers' administration page to display existing trigger rules and add the ability to add new rules and edit existing ones. Sponsored-by: Glasgow Colleges Library Group
Created attachment 171734 [details] [review] Bug 10190: Add a library API client This patch adds a library api client to the vue ecosystem for later use in the Circulation triggers page. Sponsored-by: Glasgow Colleges Library Group
Created attachment 171735 [details] [review] Bug 10190: Add itemtypes and patron categories to the item/patron api clients This patch adds patron and itemtype api clients for the Vue ecosystem Sponsored-by: Glasgow Colleges Library Group
Created attachment 171736 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group
Created attachment 171737 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group
Created attachment 171738 [details] [review] Bug 10190: Update overdue_notices script to refer to circ rules This patch updates the overdue_notices cron script to refer to the new location for overdue rules found in the circulation rules table now. Sponsored-by: Glasgow Colleges Library Group
Created attachment 171739 [details] [review] Bug 10190: Add itemtype handling to overdue_notices script This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. Sponsored-by: Glasgow Colleges Library Group
Created attachment 171740 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group
Created attachment 171741 [details] [review] Bug 10190: Add letter names to tables Sponsored-by: Glasgow Colleges Library Group
Created attachment 171742 [details] [review] Bug 10190: Set tab based on trigger edited/created Sponsored-by: Glasgow Colleges Library Group
Created attachment 171743 [details] [review] Bug 10190: Add fallback to 'Default' text for notice Sponsored-by: Glasgow Colleges Library Group
Created attachment 171744 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group
Created attachment 171745 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group
Created attachment 171746 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules
Created attachment 171747 [details] [review] Bug 10190: Add comments to overdue_notices and rename variable Trying to fully understand how this script operates.. this patch adds some notes to the script for developers and also renames a variable that I found was being re-used.
Created attachment 171748 [details] [review] Bug 10190: Rework logic of overdue_notices The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level.
Created attachment 171749 [details] [review] Bug 10190: Display effective values and highlight them
Created attachment 171750 [details] [review] Bug 10190: Restore 'edit' from 'Add/Edit' Modal
Created attachment 171751 [details] [review] Bug 10190: Modal form improvements We now fetch all rules on modal open and the effective rule on context selection. This allows us to compare the specific values of the rule we're adding/editing to the fallback values present in the all rules set. We use the fallback rule values as placeholders in the input boxes to highlight to the end user what value will be used should they choose to wipe the input. We also allow wiping inputs and pass 'null' to the API appropriately. Finally, we set a minimum value for delay when adding a new rule to highlight to the user that the new trigger will be added after existing triggers.
Created attachment 171752 [details] [review] Bug 10190: Add custom input for delay This custom input allows for dynamic min/max rules and it updates the input styling to match other inputs on the page including a 'clear' button.
Created attachment 171753 [details] [review] Bug 10190: Clarify workflow This patch re-arranges the edit/add modal form to clarify the need to select context, then decide on editing an existing trigger or adding a new one and then, and only then, exposes the actual rule inputs
Created attachment 171754 [details] [review] Bug 10190: Update for bootstrap 5
Created attachment 171755 [details] [review] Bug 10190: Rebase for rspack
This is all ready for testing now and can be done using the sandbox above, a fresh sandbox or a local dev env.
Installed on sandboxes : updatedatabase error, problem of foreign key constraint
Notes from Nick Clemens at KohaCon24 * Use syspref for 'All libraries' or 'Logged in library' by default - DefaultToLoggedInLibraryOverdueTriggers * We should not be able to save a notice without a transport type (including aking into account fallbacks) * Note about whether we're talking aobut 'patrons home library' or 'items home library' * If a notice template is only set up for one library only display it in the notice dropdown when in that context * Can we expose somewhere the list of all notice trigger context combinations..
Created attachment 172588 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group
Created attachment 172589 [details] [review] Bug 10190: DBIC Schema update
Created attachment 172590 [details] [review] Bug 10190: Remove OverdueRule Koha Objects
Created attachment 172591 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group
Created attachment 172592 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group
Created attachment 172593 [details] [review] Bug 10190: Add a vue app for circulation triggers This patch adds a new vue based page for administering circulation triggers. This aims to replace the 'Overdue notice/status triggers' page available from Tools. The new page is available from Koha administration > Circulation triggers Sponsored-by: Glasgow Colleges Library Group
Created attachment 172594 [details] [review] Bug 10190: Add components to list/add and edit circulation triggers These new components are used by the new 'Circulation triggers' administration page to display existing trigger rules and add the ability to add new rules and edit existing ones. Sponsored-by: Glasgow Colleges Library Group
Created attachment 172595 [details] [review] Bug 10190: Add a library API client This patch adds a library api client to the vue ecosystem for later use in the Circulation triggers page. Sponsored-by: Glasgow Colleges Library Group
Created attachment 172596 [details] [review] Bug 10190: Add itemtypes and patron categories to the item/patron api clients This patch adds patron and itemtype api clients for the Vue ecosystem Sponsored-by: Glasgow Colleges Library Group
Created attachment 172597 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group
Created attachment 172598 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group
Created attachment 172599 [details] [review] Bug 10190: Update overdue_notices script to refer to circ rules This patch updates the overdue_notices cron script to refer to the new location for overdue rules found in the circulation rules table now. Sponsored-by: Glasgow Colleges Library Group
Created attachment 172600 [details] [review] Bug 10190: Add itemtype handling to overdue_notices script This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. Sponsored-by: Glasgow Colleges Library Group
Created attachment 172601 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group
Created attachment 172602 [details] [review] Bug 10190: Add letter names to tables Sponsored-by: Glasgow Colleges Library Group
Created attachment 172603 [details] [review] Bug 10190: Set tab based on trigger edited/created Sponsored-by: Glasgow Colleges Library Group
Created attachment 172604 [details] [review] Bug 10190: Add fallback to 'Default' text for notice Sponsored-by: Glasgow Colleges Library Group
Created attachment 172605 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group
Created attachment 172606 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group
Created attachment 172607 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules
Created attachment 172608 [details] [review] Bug 10190: Add comments to overdue_notices and rename variable Trying to fully understand how this script operates.. this patch adds some notes to the script for developers and also renames a variable that I found was being re-used.
Created attachment 172609 [details] [review] Bug 10190: Rework logic of overdue_notices The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level.
Created attachment 172612 [details] [review] Bug 10190: Display effective values and highlight them
Created attachment 172614 [details] [review] Bug 10190: Restore 'edit' from 'Add/Edit' Modal
Created attachment 172617 [details] [review] Bug 10190: Modal form improvements We now fetch all rules on modal open and the effective rule on context selection. This allows us to compare the specific values of the rule we're adding/editing to the fallback values present in the all rules set. We use the fallback rule values as placeholders in the input boxes to highlight to the end user what value will be used should they choose to wipe the input. We also allow wiping inputs and pass 'null' to the API appropriately. Finally, we set a minimum value for delay when adding a new rule to highlight to the user that the new trigger will be added after existing triggers.
Created attachment 172619 [details] [review] Bug 10190: Add custom input for delay This custom input allows for dynamic min/max rules and it updates the input styling to match other inputs on the page including a 'clear' button.
Created attachment 172621 [details] [review] Bug 10190: Clarify workflow This patch re-arranges the edit/add modal form to clarify the need to select context, then decide on editing an existing trigger or adding a new one and then, and only then, exposes the actual rule inputs
Created attachment 172622 [details] [review] Bug 10190: Update for bootstrap 5
Created attachment 172623 [details] [review] Bug 10190: Rebase for rspack
Created attachment 172624 [details] [review] Bug 10190: Rename 'All *' to 'Default rule for *' Try to clarify that the 'All' rules are actually fallback/default rules for when there's not a more specific rule for the context. We need to expose these clearly for each 'Library' view too which we're not currently doing.
Created attachment 172625 [details] [review] Bug 10190: Take DefaultToLoggedInLibraryOverdueTriggers into account This updates the logic for the default page load to take account of DefaultToLoggedInLibraryOverdueTriggers.
Created attachment 172626 [details] [review] Bug 10190: Only display default and library specific letters This patch filters the list of letter templates available to use by only those available for the context the rule is being added for (i.e. the correct library relation)
Created attachment 172630 [details] [review] Bug 10190: Display value of OverdueNoticeFrom in note
Created attachment 172631 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172632 [details] [review] Bug 10190: DBIC Schema update Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172633 [details] [review] Bug 10190: Remove OverdueRule Koha Objects Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172634 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172635 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172636 [details] [review] Bug 10190: Add a vue app for circulation triggers This patch adds a new vue based page for administering circulation triggers. This aims to replace the 'Overdue notice/status triggers' page available from Tools. The new page is available from Koha administration > Circulation triggers Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172637 [details] [review] Bug 10190: Add components to list/add and edit circulation triggers These new components are used by the new 'Circulation triggers' administration page to display existing trigger rules and add the ability to add new rules and edit existing ones. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172638 [details] [review] Bug 10190: Add a library API client This patch adds a library api client to the vue ecosystem for later use in the Circulation triggers page. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172639 [details] [review] Bug 10190: Add itemtypes and patron categories to the item/patron api clients This patch adds patron and itemtype api clients for the Vue ecosystem Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172640 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172641 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172642 [details] [review] Bug 10190: Update overdue_notices script to refer to circ rules This patch updates the overdue_notices cron script to refer to the new location for overdue rules found in the circulation rules table now. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172643 [details] [review] Bug 10190: Add itemtype handling to overdue_notices script This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172644 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172645 [details] [review] Bug 10190: Add letter names to tables Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172646 [details] [review] Bug 10190: Set tab based on trigger edited/created Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172647 [details] [review] Bug 10190: Add fallback to 'Default' text for notice Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172648 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172649 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172650 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172651 [details] [review] Bug 10190: Add comments to overdue_notices and rename variable Trying to fully understand how this script operates.. this patch adds some notes to the script for developers and also renames a variable that I found was being re-used. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172652 [details] [review] Bug 10190: Rework logic of overdue_notices The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172653 [details] [review] Bug 10190: Display effective values and highlight them Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172654 [details] [review] Bug 10190: Restore 'edit' from 'Add/Edit' Modal Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172655 [details] [review] Bug 10190: Modal form improvements We now fetch all rules on modal open and the effective rule on context selection. This allows us to compare the specific values of the rule we're adding/editing to the fallback values present in the all rules set. We use the fallback rule values as placeholders in the input boxes to highlight to the end user what value will be used should they choose to wipe the input. We also allow wiping inputs and pass 'null' to the API appropriately. Finally, we set a minimum value for delay when adding a new rule to highlight to the user that the new trigger will be added after existing triggers. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172656 [details] [review] Bug 10190: Add custom input for delay This custom input allows for dynamic min/max rules and it updates the input styling to match other inputs on the page including a 'clear' button. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172657 [details] [review] Bug 10190: Clarify workflow This patch re-arranges the edit/add modal form to clarify the need to select context, then decide on editing an existing trigger or adding a new one and then, and only then, exposes the actual rule inputs Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172658 [details] [review] Bug 10190: Update for bootstrap 5 Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172659 [details] [review] Bug 10190: Rebase for rspack Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172660 [details] [review] Bug 10190: Rename 'All *' to 'Default rule for *' Try to clarify that the 'All' rules are actually fallback/default rules for when there's not a more specific rule for the context. We need to expose these clearly for each 'Library' view too which we're not currently doing. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172661 [details] [review] Bug 10190: Take DefaultToLoggedInLibraryOverdueTriggers into account This updates the logic for the default page load to take account of DefaultToLoggedInLibraryOverdueTriggers. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172662 [details] [review] Bug 10190: Only display default and library specific letters This patch filters the list of letter templates available to use by only those available for the context the rule is being added for (i.e. the correct library relation) Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 172663 [details] [review] Bug 10190: Display value of OverdueNoticeFrom in note Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
(In reply to Martin Renvoize (ashimema) from comment #65) > Notes from Nick Clemens at KohaCon24 > * Use syspref for 'All libraries' or 'Logged in library' by default - > DefaultToLoggedInLibraryOverdueTriggers DONE > * Note about whether we're talking aobut 'patrons home library' or 'items > home library' DONE > * If a notice template is only set up for one library only display it in the > notice dropdown when in that context DONE > * Can we expose somewhere the list of all notice trigger context > combinations.. In a follow-up bug I reckon.
Created attachment 178643 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178644 [details] [review] Bug 10190: DBIC Schema update Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178645 [details] [review] Bug 10190: Remove OverdueRule Koha Objects Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178646 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178647 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178648 [details] [review] Bug 10190: Add a vue app for circulation triggers This patch adds a new vue based page for administering circulation triggers. This aims to replace the 'Overdue notice/status triggers' page available from Tools. The new page is available from Koha administration > Circulation triggers Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178649 [details] [review] Bug 10190: Add components to list/add and edit circulation triggers These new components are used by the new 'Circulation triggers' administration page to display existing trigger rules and add the ability to add new rules and edit existing ones. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178650 [details] [review] Bug 10190: Add a library API client This patch adds a library api client to the vue ecosystem for later use in the Circulation triggers page. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178651 [details] [review] Bug 10190: Add itemtypes and patron categories to the item/patron api clients This patch adds patron and itemtype api clients for the Vue ecosystem Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178652 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178653 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178654 [details] [review] Bug 10190: Update overdue_notices script to refer to circ rules This patch updates the overdue_notices cron script to refer to the new location for overdue rules found in the circulation rules table now. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178655 [details] [review] Bug 10190: Add itemtype handling to overdue_notices script This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178656 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178657 [details] [review] Bug 10190: Add letter names to tables Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178658 [details] [review] Bug 10190: Set tab based on trigger edited/created Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178659 [details] [review] Bug 10190: Add fallback to 'Default' text for notice Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178660 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178661 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178662 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178663 [details] [review] Bug 10190: Add comments to overdue_notices and rename variable Trying to fully understand how this script operates.. this patch adds some notes to the script for developers and also renames a variable that I found was being re-used. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178664 [details] [review] Bug 10190: Rework logic of overdue_notices The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178665 [details] [review] Bug 10190: Display effective values and highlight them Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178666 [details] [review] Bug 10190: Restore 'edit' from 'Add/Edit' Modal Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178667 [details] [review] Bug 10190: Modal form improvements We now fetch all rules on modal open and the effective rule on context selection. This allows us to compare the specific values of the rule we're adding/editing to the fallback values present in the all rules set. We use the fallback rule values as placeholders in the input boxes to highlight to the end user what value will be used should they choose to wipe the input. We also allow wiping inputs and pass 'null' to the API appropriately. Finally, we set a minimum value for delay when adding a new rule to highlight to the user that the new trigger will be added after existing triggers. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178668 [details] [review] Bug 10190: Add custom input for delay This custom input allows for dynamic min/max rules and it updates the input styling to match other inputs on the page including a 'clear' button. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178669 [details] [review] Bug 10190: Clarify workflow This patch re-arranges the edit/add modal form to clarify the need to select context, then decide on editing an existing trigger or adding a new one and then, and only then, exposes the actual rule inputs Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178670 [details] [review] Bug 10190: Update for bootstrap 5 Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178671 [details] [review] Bug 10190: Rebase for rspack Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178672 [details] [review] Bug 10190: Rename 'All *' to 'Default rule for *' Try to clarify that the 'All' rules are actually fallback/default rules for when there's not a more specific rule for the context. We need to expose these clearly for each 'Library' view too which we're not currently doing. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178673 [details] [review] Bug 10190: Take DefaultToLoggedInLibraryOverdueTriggers into account This updates the logic for the default page load to take account of DefaultToLoggedInLibraryOverdueTriggers. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178674 [details] [review] Bug 10190: Only display default and library specific letters This patch filters the list of letter templates available to use by only those available for the context the rule is being added for (i.e. the correct library relation) Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 178675 [details] [review] Bug 10190: Display value of OverdueNoticeFrom in note Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Comment on attachment 178644 [details] [review] Bug 10190: DBIC Schema update Review of attachment 178644 [details] [review]: ----------------------------------------------------------------- ::: Koha/Schema/Result/MessageTransportType.pm @@ +133,5 @@ > ); > > > +# Created by DBIx::Class::Schema::Loader v0.07051 @ 2024-08-15 11:53:13 > +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:Bx96QiqHxZ9SNbgK8dSwMg There are no manual changes to split in separate commit. Whole commit should be marked as [DO NOT PUSH] https://wiki.koha-community.org/wiki/Database_updates#Files_to_change
updatedatabase doesn't work: (have a koha on main, apply the patches and update) $ updatedatabase Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl [23:24:08]: Bug 10190 - Migrate overduerules to circulation_rules Inserting :undef: default most frequent delay for overdue_1: Inserting :undef:undef default most frequent notice for overdue_1: Inserting :undef:undef default most frequent mtt for overdue_1: Inserting :undef:undef default most frequent restrict for overdue_1: 0 Inserting :undef: default most frequent delay for overdue_2: Inserting :undef:undef default most frequent notice for overdue_2: Inserting :undef:undef default most frequent mtt for overdue_2: Inserting :undef:undef default most frequent restrict for overdue_2: 0 Inserting :undef: default most frequent delay for overdue_3: Inserting :undef:undef default most frequent notice for overdue_3: Inserting :undef:undef default most frequent mtt for overdue_3: Inserting :undef:undef default most frequent restrict for overdue_3: 0 ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails at /kohadevbox/koha/C4/Installer.pm line 771
MariaDB 10.5.26 in case that matters.
Comment on attachment 178648 [details] [review] Bug 10190: Add a vue app for circulation triggers Review of attachment 178648 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/circulation_triggers.tt @@ +25,5 @@ > + <!-- this is closed in intranet-bottom.inc --> > + > + [% MACRO jsinclude BLOCK %] > + [% INCLUDE 'calendar.inc' %] > + <!-- FIXME: this shouldn't be needed --> Is the FIXME for calendar.inc above or datatables.inc bellow or the 4 includes below?
(In reply to Victor Grousset/tuxayo from comment #169) > Comment on attachment 178648 [details] [review] [review] > Bug 10190: Add a vue app for circulation triggers > > Review of attachment 178648 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/circulation_triggers.tt > @@ +25,5 @@ > > + <!-- this is closed in intranet-bottom.inc --> > > + > > + [% MACRO jsinclude BLOCK %] > > + [% INCLUDE 'calendar.inc' %] > > + <!-- FIXME: this shouldn't be needed --> > > Is the FIXME for calendar.inc above or datatables.inc bellow or the 4 > includes below? It was for calendar.inc and if you look at the other vue apps, it's consistent throughout them so far.
*** Bug 12772 has been marked as a duplicate of this bug. ***
I wanted to see what it looked like, but unfortunately, the patch doesn't apply anymore...
Koha's been moving so fast.. probably best to use the public branch for this: https://github.com/openfifth/koha/tree/bug_10190_submission-rebased
We've been reworking this considerably, given live use feedback. Chloe's taken over the reins, so I'm updating the assignee, but the reality is Chloe, Matt, and I have all done a lot of work between us on this one.
Created attachment 188075 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188076 [details] [review] Bug 10190: DBIC Schema update Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188077 [details] [review] Bug 10190: Remove OverdueRule Koha Objects Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188078 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188079 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188080 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188081 [details] [review] Bug 10190: Rework logic of overdue_notices This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188082 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188083 [details] [review] Bug 10190: Complete Vue.js rewrite of Circulation Triggers administration interface This patch introduces a modern Vue.js-based interface for managing circulation triggers (overdue notice/status triggers), replacing the legacy Tools-based interface with a more intuitive, feature-rich administration page. New Features Enhanced Visibility & Navigation - Exhaustive effective rule lists showing all rules that apply to each context - Toggle between "all exhaustive rules" and "explicitly set rules" views - Advanced filtering by patron category and/or item type - Clear display of fallback/default rules for each context Rule Management Actions - Delete triggers (last trigger in a sequence for any given library) - Reset entire rule sets for a given context (with safeguards) - Edit existing triggers with improved context awareness - Add new triggers with intelligent delay constraints Improved User Experience - Multi-step form wizard with clear workflow progression - Context-aware letter template filtering (only shows applicable templates) - Placeholder values showing fallback rules when clearing inputs - Dynamic min/max constraints for delay values based on neighboring triggers - Real-time form validation and feedback - Partial page reloads (only reload affected sections, not entire page) - Specific loading messages for different operations Technical Improvements - Centralized Vuex store for state management - Reusable CirculationTriggersForm component - API clients for libraries, circulation rules, item types, and patron categories - Consistent async/await pattern throughout - Separation of repositories, utilities, and services - Improved naming consistency and code maintainability - "Dumb" TriggersTable component for better separation of concerns Test Plan Prerequisites 1. Log in to staff interface with appropriate permissions (tools_edit_notice_status_triggers) 2. Ensure you have test data: multiple libraries, patron categories, item types, and notice templates 3. Navigate to Administration > Circulation triggers (new interface) Part 1: Basic Navigation & Interface 1. Verify the new "Circulation triggers" link appears in Administration home under the Circulation section 2. Access the new interface and confirm it loads successfully 3. Verify the default view shows either "All libraries" or your logged-in library based on DefaultToLoggedInLibraryOverdueTriggers system preference 4. Confirm the interface displays a list of triggers organized by context (library/patron category/item type combinations) Part 2: Viewing and Filtering Rules 1. Toggle between "All exhaustive rules" and "Explicitly set rules" views - Verify "All exhaustive rules" shows inherited/fallback rules - Verify "Explicitly set rules" shows only rules you've specifically configured 2. Test the patron category filter: - Select a specific patron category - Confirm only rules for that category are displayed 3. Test the item type filter: - Select a specific item type - Confirm only rules for that item type are displayed 4. Test combined filters (patron category + item type simultaneously) 5. Clear filters and verify all rules display again Part 3: Adding New Triggers 1. Click "Add trigger" button 2. Step 1 - Confirm Context: - Select Library (required) - Select Patron category (required) - Select Item type (required) - Click "Confirm context" 3. Step 2 - Review Existing Triggers: - Verify existing triggers for this context are displayed - Note the trigger numbers and delays 4. Step 3 - Configure New Trigger: - Enter a delay value (should be constrained by neighboring triggers) - Try entering an invalid delay (too low or too high) - verify validation - Set "Restricts checkouts" (Yes/No/Fallback to default) - Note the placeholder showing the fallback value - Select a letter template - Verify only templates for the selected library context appear - Set "Debarring" options if desired - Select "Charged notice fees" if applicable 5. Submit the form 6. Verify the new trigger appears in the list without a full page reload 7. Verify success message is displayed Part 4: Editing Existing Triggers 1. Locate an existing trigger in the list 2. Click the edit icon/button 3. Verify the form pre-populates with existing values 4. Test delay modification: - Try changing the delay within valid bounds - should succeed - Try changing delay beyond neighboring triggers - should show validation error - Use the increment/decrement buttons to adjust delay 5. Test clearing values: - Clear a field (e.g., delay or restrict checkouts) - Verify placeholder shows the fallback/default value - Submit with cleared value - should save as null and inherit fallback 6. Change the letter template to a different valid template 7. Submit changes 8. Verify the trigger updates in the list without full page reload 9. Verify the changes persist after refreshing the page Part 5: Deleting Triggers 1. Identify a trigger that is the last in its sequence 2. Click the delete button/icon 3. Verify a confirmation dialog appears 4. Confirm deletion 5. Verify the trigger is removed from the list without page reload 6. Verify the deletion persists after page refresh 7. Test delete constraints: - Try to delete a trigger that's not the last in sequence - Verify appropriate error/warning message Part 6: Resetting Rule Sets 1. Locate a context with explicitly set rules (not just inherited) 2. Ensure this is not the only rule set for this trigger sequence 3. Click the "Reset" button for that context 4. Verify confirmation dialog explains what will be reset 5. Confirm the reset 6. Verify the explicit rules are removed and context now shows inherited rules 7. Verify changes persist after page refresh 8. Test reset constraints: - Try to reset when it would leave no rules for that trigger - Verify appropriate warning/prevention Part 7: Context-Specific Behavior 1. Test library-specific letter filtering: - Create/edit a trigger for a specific library - Verify only letter templates with matching branchcode or default templates appear - Switch to a different library context - Verify the letter list updates accordingly 2. Test default rules: - View rules for "All libraries" / "All patron categories" / "All item types" - Verify these are labeled as "Default rule for [context]" - Confirm these serve as fallback for more specific rules 3. Test DefaultToLoggedInLibraryOverdueTriggers preference: - Set preference to "Yes" - Reload circulation triggers page - Verify it defaults to your logged-in library - Set preference to "No" - Verify it defaults to "All libraries" Part 8: Form Workflow & Validation 1. Test required fields: - Try submitting add/edit form without selecting library - should fail - Try submitting without patron category - should fail - Try submitting without item type - should fail 2. Test stepper navigation: - Start adding a trigger - Verify you must confirm context before proceeding - Verify you see existing triggers before editing/adding - Verify trigger form only appears after context confirmation 3. Test cancellation: - Start adding/editing a trigger - Click Cancel - Verify you return to the list without changes - Verify no partial data was saved Part 9: Loading States & Messages 1. On slow connections or with many rules: - Verify "Loading context..." appears while fetching context data - Verify "Loading rule set information..." appears while fetching rules - Verify section-specific loading messages appear during updates 2. After form submission: - Verify appropriate success/error messages display - Verify loading indicators during async operations Part 10: Permissions 1. Log in as a user without tools_edit_notice_status_triggers permission 2. Verify the "Circulation triggers" link does not appear in Administration 3. Try accessing /cgi-bin/koha/admin/circulation_triggers directly 4. Verify access is denied Part 11: Data Integrity & Persistence 1. Create a complex set of rules with multiple triggers across different contexts 2. Refresh the page and verify all rules persist 3. Restart the Koha services 4. Verify all rules still display correctly 5. Check the database circulation_rules table: - Verify rules are stored with correct rule_name pattern (overdue_1_delay, etc.) - Verify null values are properly stored for inheritance - Verify the overdue_X_has_rules rule kind is present Part 12: Edge Cases 1. Test with no existing triggers - verify empty state message 2. Test with maximum trigger sequences (e.g., trigger 1, 2, 3) 3. Test rapid successive edits 4. Test with special characters in letter template names 5. Test browser back/forward buttons during form workflow Part 13: Comparison with Legacy Interface 1. Access the old interface if still available (via Tools menu) 2. Create a trigger using the new interface 3. Verify it appears correctly in the old interface (if applicable) 4. Create a trigger using the old interface (if still available) 5. Verify it appears correctly in the new interface --- Notes for QA - This is a complete UI rewrite; the underlying API remains unchanged - The interface is mobile-responsive and should be tested on various screen sizes - Vue.js components are compiled via Rspack; run yarn build after pulling - All existing circulation trigger functionality should continue to work - The URL rewrite rule ensures /admin/circulation_triggers routes correctly Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Co-authored-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Co-authored-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188084 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188085 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188086 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188087 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 188088 [details] [review] Bug 10190: Comprehensive Cypress tests for Circulation Triggers UI This patch significantly enhances the Cypress test coverage for the new Circulation Triggers administration interface, expanding from basic breadcrumb checks to comprehensive end-to-end testing. Test Coverage: - Initial page load and API error handling - Context filtering (library, patron category, item type) - View toggling between explicit and all applicable rules - Tab navigation between multiple triggers - Full CRUD operations (Create, Read, Update, Delete) - Rule set reset functionality - Letter template filtering by library context - Form validation and constraint checking - Loading states and empty states - Error handling for all operations - Browser navigation (back/forward buttons) - Complex scenarios with inheritance hierarchies New Features: - Mock data helper functions for test fixtures - Comprehensive API interception for all endpoints - Tests for both success and error paths - Placeholder value testing for fallback rules - Increment/decrement button testing - Multi-step form workflow validation - Confirmation dialog testing for destructive operations The tests follow Koha's Cypress testing patterns established in the ERM module, using proper API mocking, clear test descriptions, and organized test suites.
As Victor Grousset/tuxayo wrote in Comment 167 (2025-02-26) updatedatabase doesn't work: (have a koha on main, apply the patches and update) $ updatedatabase Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl [08:57:11]: Bug 10190 - Migrate overduerules to circulation_rules Inserting :undef: default most frequent delay for overdue_1: Inserting :undef:undef default most frequent notice for overdue_1: Inserting :undef:undef default most frequent mtt for overdue_1: Inserting :undef:undef default most frequent restrict for overdue_1: 0 Inserting :undef: default most frequent delay for overdue_2: Inserting :undef:undef default most frequent notice for overdue_2: Inserting :undef:undef default most frequent mtt for overdue_2: Inserting :undef:undef default most frequent restrict for overdue_2: 0 Inserting :undef: default most frequent delay for overdue_3: Inserting :undef:undef default most frequent notice for overdue_3: Inserting :undef:undef default most frequent mtt for overdue_3: Inserting :undef:undef default most frequent restrict for overdue_3: 0 ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails at /kohadevbox/koha/C4/Installer.pm line 825
Created attachment 188476 [details] [review] Bug 10190: (QA follow-up): Fix removal of tables
(In reply to Pedro Amorim from comment #190) > Created attachment 188476 [details] [review] [review] > Bug 10190: (QA follow-up): Fix removal of tables This fixes the database error but does not address the undef warnings.
To have it working on a upgrade, I had to run the following: Apply patches $ updatabase $ yarn js:build $ sudo perl /kohadevbox/misc4dev/cp_debian_files.pl --instance=kohadev --koha_dir=/kohadevbox/koha --gitify_dir=/kohadevbox/gitify $ restart_all The cp_debian_files.pl is required or else the URL always returns 404: http://localhost:8081/cgi-bin/koha/admin/circulation_triggers I believe this is why applying patches and doing reset_all, but applying patches and doing restart_all does not, it's missing the cp_debian_files part, which I believe copies debian/templates/apache-shared-intranet.conf among other files. It is now producing a JavaScript error on the console though, which may or may not highlight an issue with the data migration script.
We tested several hours and I will provide more feedback this week but here are the most relevant Bugs we found: 1.) Migration scenario have some overdue rule, then apply the patch and do updatabase and yarn buid Error 404 in staff interface Please see Pedros comment 192 In your testplan in comment 183 you write in Part 11: Data Integrity & Persistence 5. Check the database circulation_rules table: - Verify the overdue_X_has_rules rule kind is present Is this the missing piece for the migrated data ? 2.) Adding a trigger for a combination that exists in circ rules it is not possible to configure an overdue trigger with a combination that already exists as a circulation rule. Example to reproduce in ktd with test data: - Go to circ rules (/cgi-bin/koha/admin/smart-rules.pl) - Set a rule for All Libraries / All patron categories / Itemtype Books - Check via SQL: select * from circulation_rules - Go to overdue rules (/cgi-bin/koha/admin/circulation_triggers ) - Set a rule for All Libraries / All patron categories / All Itemtype - Now try to edit the delay for the applied rule with combination All Libraries / All patron categories / Itemtype Books --> Message in blue box "Your changes could not be saved as this circulation trigger was updated elsewhere. Please see the updated trigger below." It is not possible to save / submit - Now edit the delay for the applied rule with combination All Libraries / Patron category Homebound / Itemtype Books --> You can save without problems, if this combination does not exist in the database table circulation_rules 3.) Permissions Error if you are not an admin The Overlay rules are part of the tools and also the permission is part of the tools section. That means that it was enough to give users the edit_notice_status_triggers permission, it was not neccessary to give any administration permission (parameters) . Now you moved the overdue triggers to administration. If you now log in as a person with edit_notice_status_triggers permission you can see the tools module and click on the menu for overdue triggers but then you get the error message "Something went wrong: Error: Authorization failure. Missing required permission(s)." 4.) Edit an existing trigger does not pull delay from database Edit an existing trigger and check that the delay value is 0 , it is not pulled from the database. This is confusing and dangerous because you see the delay value just a few centimeters above in the table. If you save the form, the value in the database is overwritten with 0. To reproduce: follow testplan in comment 183 Part 4.3
(In reply to Michaela Sieber from comment #193) > We tested several hours and I will provide more feedback this week but here > are the most relevant Bugs we found: > > 1.) Migration scenario > > have some overdue rule, then apply the patch and do updatabase and yarn buid > > Error 404 in staff interface > > Please see Pedros comment 192 > > In your testplan in comment 183 you write in > > Part 11: Data Integrity & Persistence > 5. Check the database circulation_rules table: > - Verify the overdue_X_has_rules rule kind is present > > Is this the missing piece for the migrated data ? > > > 2.) Adding a trigger for a combination that exists in circ rules > > it is not possible to configure an overdue trigger with a combination that > already exists as a circulation rule. > > Example to reproduce in ktd with test data: > > - Go to circ rules (/cgi-bin/koha/admin/smart-rules.pl) > - Set a rule for All Libraries / All patron categories / Itemtype Books > - Check via SQL: select * from circulation_rules > - Go to overdue rules (/cgi-bin/koha/admin/circulation_triggers ) > - Set a rule for All Libraries / All patron categories / All Itemtype > - Now try to edit the delay for the applied rule > with combination All Libraries / All patron categories / Itemtype Books > --> Message in blue box "Your changes could not be saved as this > circulation > trigger was updated elsewhere. Please see the updated trigger below." > It is not possible to save / submit > - Now edit the delay for the applied rule > with combination All Libraries / Patron category Homebound / Itemtype > Books > --> You can save without problems, if this combination does not exist in > the database table circulation_rules > > > 3.) Permissions Error if you are not an admin > > The Overlay rules are part of the tools and also the permission is part of > the tools section. That means that it was enough to give users the > edit_notice_status_triggers permission, it was not neccessary to give any > administration permission (parameters) . > Now you moved the overdue triggers to administration. > > If you now log in as a person with edit_notice_status_triggers permission > you can see the tools module and click on the menu for overdue triggers but > then you get the error message "Something went wrong: Error: Authorization > failure. Missing required permission(s)." > > > 4.) Edit an existing trigger does not pull delay from database > > Edit an existing trigger and check that the delay value is 0 , it is not > pulled from the database. This is confusing and dangerous because you see > the delay value just a few centimeters above in the table. > If you save the form, the value in the database is overwritten with 0. > > To reproduce: follow testplan in comment 183 Part 4.3 Thank you for the testing and feedback, much appreciated! Re - 1.) overdue_X_has_rules is a property indicating that some rule(s) exist for a given context/trigger number combination. The most recent version of the integration ensure it is created whenever any rule is created for a given context/trigger number combination. The staff clients relies on this to ascertain whether a rule set exists efficiently. I think the solution might be to have it added to each existing context/trigger number combination - would that be sensible? 2., 3., and 4.: I will do some debugging and put patches together for those, many thanks for reporting them!
Created attachment 189345 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules: - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set
(In reply to Chloé Zermatten from comment #194) > (In reply to Michaela Sieber from comment #193) > > We tested several hours and I will provide more feedback this week but here > > are the most relevant Bugs we found: > > > > 1.) Migration scenario > > > > have some overdue rule, then apply the patch and do updatabase and yarn buid > > > > Error 404 in staff interface > > > > Please see Pedros comment 192 > > > > In your testplan in comment 183 you write in > > > > Part 11: Data Integrity & Persistence > > 5. Check the database circulation_rules table: > > - Verify the overdue_X_has_rules rule kind is present > > > > Is this the missing piece for the migrated data ? > > > > > > 2.) Adding a trigger for a combination that exists in circ rules > > > > it is not possible to configure an overdue trigger with a combination that > > already exists as a circulation rule. > > > > Example to reproduce in ktd with test data: > > > > - Go to circ rules (/cgi-bin/koha/admin/smart-rules.pl) > > - Set a rule for All Libraries / All patron categories / Itemtype Books > > - Check via SQL: select * from circulation_rules > > - Go to overdue rules (/cgi-bin/koha/admin/circulation_triggers ) > > - Set a rule for All Libraries / All patron categories / All Itemtype > > - Now try to edit the delay for the applied rule > > with combination All Libraries / All patron categories / Itemtype Books > > --> Message in blue box "Your changes could not be saved as this > > circulation > > trigger was updated elsewhere. Please see the updated trigger below." > > It is not possible to save / submit > > - Now edit the delay for the applied rule > > with combination All Libraries / Patron category Homebound / Itemtype > > Books > > --> You can save without problems, if this combination does not exist in > > the database table circulation_rules > > > > > > 3.) Permissions Error if you are not an admin > > > > The Overlay rules are part of the tools and also the permission is part of > > the tools section. That means that it was enough to give users the > > edit_notice_status_triggers permission, it was not neccessary to give any > > administration permission (parameters) . > > Now you moved the overdue triggers to administration. > > > > If you now log in as a person with edit_notice_status_triggers permission > > you can see the tools module and click on the menu for overdue triggers but > > then you get the error message "Something went wrong: Error: Authorization > > failure. Missing required permission(s)." > > > > > > 4.) Edit an existing trigger does not pull delay from database > > > > Edit an existing trigger and check that the delay value is 0 , it is not > > pulled from the database. This is confusing and dangerous because you see > > the delay value just a few centimeters above in the table. > > If you save the form, the value in the database is overwritten with 0. > > > > To reproduce: follow testplan in comment 183 Part 4.3 > > Thank you for the testing and feedback, much appreciated! > > > Re - 1.) overdue_X_has_rules is a property indicating that some rule(s) > exist for a given context/trigger number combination. The most recent > version of the integration ensure it is created whenever any rule is created > for a given context/trigger number combination. The staff clients relies on > this to ascertain whether a rule set exists efficiently. > I think the solution might be to have it added to each existing > context/trigger number combination - would that be sensible? > > 2., 3., and 4.: I will do some debugging and put patches together for those, > many thanks for reporting them! Quick update on this: The latest commit (Attachment #189345 [details]) intends to address point 1, 2, and 4 from the QA feedback. 3 is, to my knowledge, one for discussion as it looks to me like expected behaviour. In the meantime, if any other bugs are found, I'll be happy to look at addressing them!
Unfortunately, the patch doesn't apply: Applying: Bug 10190: Update has_restricting_overdues to refer to circulation rules Using index info to reconstruct a base tree... M Koha/Patron.pm Falling back to patching base and 3-way merge... Auto-merging Koha/Patron.pm CONFLICT (content): Merge conflict in Koha/Patron.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 10190: Update has_restricting_overdues to refer to circulation rules
Created attachment 189863 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 189864 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 189865 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 189866 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 189867 [details] [review] Bug 10190: Comprehensive Cypress tests for Circulation Triggers UI This patch significantly enhances the Cypress test coverage for the new Circulation Triggers administration interface, expanding from basic breadcrumb checks to comprehensive end-to-end testing. Test Coverage: - Initial page load and API error handling - Context filtering (library, patron category, item type) - View toggling between explicit and all applicable rules - Tab navigation between multiple triggers - Full CRUD operations (Create, Read, Update, Delete) - Rule set reset functionality - Letter template filtering by library context - Form validation and constraint checking - Loading states and empty states - Error handling for all operations - Browser navigation (back/forward buttons) - Complex scenarios with inheritance hierarchies New Features: - Mock data helper functions for test fixtures - Comprehensive API interception for all endpoints - Tests for both success and error paths - Placeholder value testing for fallback rules - Increment/decrement button testing - Multi-step form workflow validation - Confirmation dialog testing for destructive operations The tests follow Koha's Cypress testing patterns established in the ERM module, using proper API mocking, clear test descriptions, and organized test suites.
Created attachment 189868 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules: - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set
In the last patch: - min; + `${min}`; Why this kind of changes exactly?
(In reply to Jonathan Druart from comment #204) > In the last patch: > - min; > + `${min}`; > > Why this kind of changes exactly? The idea here is to consistently prevent JS data-types shenanigans and oddness from occurring, really - we're ensuring that once the number is calculated, we convert it to a string. Effectively, I went with: - anything that gets assigned to the Vue template: convert to string first. - when manipulating numbers (which we mostly get as string from the relevant input field): convert to integer first.
The patch doesn't apply: Applying: Bug 10190: Complete Vue.js rewrite of Circulation Triggers administration interface Using index info to reconstruct a base tree... M debian/templates/apache-shared-intranet.conf M koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt M koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js M koha-tmpl/intranet-tmpl/prog/js/fetch/patron-api-client.js M koha-tmpl/intranet-tmpl/prog/js/vue/fetch/api-client.js M rspack.config.js Falling back to patching base and 3-way merge... Auto-merging rspack.config.js CONFLICT (content): Merge conflict in rspack.config.js Auto-merging koha-tmpl/intranet-tmpl/prog/js/vue/fetch/api-client.js Auto-merging koha-tmpl/intranet-tmpl/prog/js/fetch/patron-api-client.js CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/fetch/patron-api-client.js Auto-merging koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt Auto-merging debian/templates/apache-shared-intranet.conf error: Failed to merge in the changes. Patch failed at 0001 Bug 10190: Complete Vue.js rewrite of Circulation Triggers administration interface
Created attachment 191749 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191750 [details] [review] Bug 10190: DBIC Schema update Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191751 [details] [review] Bug 10190: Remove OverdueRule Koha Objects Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191752 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191753 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191754 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191755 [details] [review] Bug 10190: Rework logic of overdue_notices This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191756 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191757 [details] [review] Bug 10190: Complete Vue.js rewrite of Circulation Triggers administration interface This patch introduces a modern Vue.js-based interface for managing circulation triggers (overdue notice/status triggers), replacing the legacy Tools-based interface with a more intuitive, feature-rich administration page. New Features Enhanced Visibility & Navigation - Exhaustive effective rule lists showing all rules that apply to each context - Toggle between "all exhaustive rules" and "explicitly set rules" views - Advanced filtering by patron category and/or item type - Clear display of fallback/default rules for each context Rule Management Actions - Delete triggers (last trigger in a sequence for any given library) - Reset entire rule sets for a given context (with safeguards) - Edit existing triggers with improved context awareness - Add new triggers with intelligent delay constraints Improved User Experience - Multi-step form wizard with clear workflow progression - Context-aware letter template filtering (only shows applicable templates) - Placeholder values showing fallback rules when clearing inputs - Dynamic min/max constraints for delay values based on neighboring triggers - Real-time form validation and feedback - Partial page reloads (only reload affected sections, not entire page) - Specific loading messages for different operations Technical Improvements - Centralized Vuex store for state management - Reusable CirculationTriggersForm component - API clients for libraries, circulation rules, item types, and patron categories - Consistent async/await pattern throughout - Separation of repositories, utilities, and services - Improved naming consistency and code maintainability - "Dumb" TriggersTable component for better separation of concerns Test Plan Prerequisites 1. Log in to staff interface with appropriate permissions (tools_edit_notice_status_triggers) 2. Ensure you have test data: multiple libraries, patron categories, item types, and notice templates 3. Navigate to Administration > Circulation triggers (new interface) Part 1: Basic Navigation & Interface 1. Verify the new "Circulation triggers" link appears in Administration home under the Circulation section 2. Access the new interface and confirm it loads successfully 3. Verify the default view shows either "All libraries" or your logged-in library based on DefaultToLoggedInLibraryOverdueTriggers system preference 4. Confirm the interface displays a list of triggers organized by context (library/patron category/item type combinations) Part 2: Viewing and Filtering Rules 1. Toggle between "All exhaustive rules" and "Explicitly set rules" views - Verify "All exhaustive rules" shows inherited/fallback rules - Verify "Explicitly set rules" shows only rules you've specifically configured 2. Test the patron category filter: - Select a specific patron category - Confirm only rules for that category are displayed 3. Test the item type filter: - Select a specific item type - Confirm only rules for that item type are displayed 4. Test combined filters (patron category + item type simultaneously) 5. Clear filters and verify all rules display again Part 3: Adding New Triggers 1. Click "Add trigger" button 2. Step 1 - Confirm Context: - Select Library (required) - Select Patron category (required) - Select Item type (required) - Click "Confirm context" 3. Step 2 - Review Existing Triggers: - Verify existing triggers for this context are displayed - Note the trigger numbers and delays 4. Step 3 - Configure New Trigger: - Enter a delay value (should be constrained by neighboring triggers) - Try entering an invalid delay (too low or too high) - verify validation - Set "Restricts checkouts" (Yes/No/Fallback to default) - Note the placeholder showing the fallback value - Select a letter template - Verify only templates for the selected library context appear - Set "Debarring" options if desired - Select "Charged notice fees" if applicable 5. Submit the form 6. Verify the new trigger appears in the list without a full page reload 7. Verify success message is displayed Part 4: Editing Existing Triggers 1. Locate an existing trigger in the list 2. Click the edit icon/button 3. Verify the form pre-populates with existing values 4. Test delay modification: - Try changing the delay within valid bounds - should succeed - Try changing delay beyond neighboring triggers - should show validation error - Use the increment/decrement buttons to adjust delay 5. Test clearing values: - Clear a field (e.g., delay or restrict checkouts) - Verify placeholder shows the fallback/default value - Submit with cleared value - should save as null and inherit fallback 6. Change the letter template to a different valid template 7. Submit changes 8. Verify the trigger updates in the list without full page reload 9. Verify the changes persist after refreshing the page Part 5: Deleting Triggers 1. Identify a trigger that is the last in its sequence 2. Click the delete button/icon 3. Verify a confirmation dialog appears 4. Confirm deletion 5. Verify the trigger is removed from the list without page reload 6. Verify the deletion persists after page refresh 7. Test delete constraints: - Try to delete a trigger that's not the last in sequence - Verify appropriate error/warning message Part 6: Resetting Rule Sets 1. Locate a context with explicitly set rules (not just inherited) 2. Ensure this is not the only rule set for this trigger sequence 3. Click the "Reset" button for that context 4. Verify confirmation dialog explains what will be reset 5. Confirm the reset 6. Verify the explicit rules are removed and context now shows inherited rules 7. Verify changes persist after page refresh 8. Test reset constraints: - Try to reset when it would leave no rules for that trigger - Verify appropriate warning/prevention Part 7: Context-Specific Behavior 1. Test library-specific letter filtering: - Create/edit a trigger for a specific library - Verify only letter templates with matching branchcode or default templates appear - Switch to a different library context - Verify the letter list updates accordingly 2. Test default rules: - View rules for "All libraries" / "All patron categories" / "All item types" - Verify these are labeled as "Default rule for [context]" - Confirm these serve as fallback for more specific rules 3. Test DefaultToLoggedInLibraryOverdueTriggers preference: - Set preference to "Yes" - Reload circulation triggers page - Verify it defaults to your logged-in library - Set preference to "No" - Verify it defaults to "All libraries" Part 8: Form Workflow & Validation 1. Test required fields: - Try submitting add/edit form without selecting library - should fail - Try submitting without patron category - should fail - Try submitting without item type - should fail 2. Test stepper navigation: - Start adding a trigger - Verify you must confirm context before proceeding - Verify you see existing triggers before editing/adding - Verify trigger form only appears after context confirmation 3. Test cancellation: - Start adding/editing a trigger - Click Cancel - Verify you return to the list without changes - Verify no partial data was saved Part 9: Loading States & Messages 1. On slow connections or with many rules: - Verify "Loading context..." appears while fetching context data - Verify "Loading rule set information..." appears while fetching rules - Verify section-specific loading messages appear during updates 2. After form submission: - Verify appropriate success/error messages display - Verify loading indicators during async operations Part 10: Permissions 1. Log in as a user without tools_edit_notice_status_triggers permission 2. Verify the "Circulation triggers" link does not appear in Administration 3. Try accessing /cgi-bin/koha/admin/circulation_triggers directly 4. Verify access is denied Part 11: Data Integrity & Persistence 1. Create a complex set of rules with multiple triggers across different contexts 2. Refresh the page and verify all rules persist 3. Restart the Koha services 4. Verify all rules still display correctly 5. Check the database circulation_rules table: - Verify rules are stored with correct rule_name pattern (overdue_1_delay, etc.) - Verify null values are properly stored for inheritance - Verify the overdue_X_has_rules rule kind is present Part 12: Edge Cases 1. Test with no existing triggers - verify empty state message 2. Test with maximum trigger sequences (e.g., trigger 1, 2, 3) 3. Test rapid successive edits 4. Test with special characters in letter template names 5. Test browser back/forward buttons during form workflow Part 13: Comparison with Legacy Interface 1. Access the old interface if still available (via Tools menu) 2. Create a trigger using the new interface 3. Verify it appears correctly in the old interface (if applicable) 4. Create a trigger using the old interface (if still available) 5. Verify it appears correctly in the new interface --- Notes for QA - This is a complete UI rewrite; the underlying API remains unchanged - The interface is mobile-responsive and should be tested on various screen sizes - Vue.js components are compiled via Rspack; run yarn build after pulling - All existing circulation trigger functionality should continue to work - The URL rewrite rule ensures /admin/circulation_triggers routes correctly Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Co-authored-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Co-authored-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191758 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191759 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191760 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191761 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 191762 [details] [review] Bug 10190: Comprehensive Cypress tests for Circulation Triggers UI This patch significantly enhances the Cypress test coverage for the new Circulation Triggers administration interface, expanding from basic breadcrumb checks to comprehensive end-to-end testing. Test Coverage: - Initial page load and API error handling - Context filtering (library, patron category, item type) - View toggling between explicit and all applicable rules - Tab navigation between multiple triggers - Full CRUD operations (Create, Read, Update, Delete) - Rule set reset functionality - Letter template filtering by library context - Form validation and constraint checking - Loading states and empty states - Error handling for all operations - Browser navigation (back/forward buttons) - Complex scenarios with inheritance hierarchies New Features: - Mock data helper functions for test fixtures - Comprehensive API interception for all endpoints - Tests for both success and error paths - Placeholder value testing for fallback rules - Increment/decrement button testing - Multi-step form workflow validation - Confirmation dialog testing for destructive operations The tests follow Koha's Cypress testing patterns established in the ERM module, using proper API mocking, clear test descriptions, and organized test suites.
Created attachment 191763 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules: - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set
(In reply to Clemens Tubach from comment #206) > The patch doesn't apply: > > Applying: Bug 10190: Complete Vue.js rewrite of Circulation Triggers > administration interface > Using index info to reconstruct a base tree... > M debian/templates/apache-shared-intranet.conf > M koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt > M koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js > M koha-tmpl/intranet-tmpl/prog/js/fetch/patron-api-client.js > M koha-tmpl/intranet-tmpl/prog/js/vue/fetch/api-client.js > M rspack.config.js > Falling back to patching base and 3-way merge... > Auto-merging rspack.config.js > CONFLICT (content): Merge conflict in rspack.config.js > Auto-merging koha-tmpl/intranet-tmpl/prog/js/vue/fetch/api-client.js > Auto-merging koha-tmpl/intranet-tmpl/prog/js/fetch/patron-api-client.js > CONFLICT (content): Merge conflict in > koha-tmpl/intranet-tmpl/prog/js/fetch/patron-api-client.js > Auto-merging koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js > CONFLICT (content): Merge conflict in > koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js > Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt > Auto-merging debian/templates/apache-shared-intranet.conf > error: Failed to merge in the changes. > Patch failed at 0001 Bug 10190: Complete Vue.js rewrite of Circulation > Triggers administration interface Thanks for letting me know! Rebase done; hopefully it should apply now :) If not, I can take another look.
Unfortunately, the patch doesn't apply: Applying: Bug 10190: (QA follow-up): Fix removal of tables Using index info to reconstruct a base tree... A installer/data/mysql/atomicupdate/bug_10190.pl Falling back to patching base and 3-way merge... CONFLICT (modify/delete): installer/data/mysql/atomicupdate/bug_10190.pl deleted in HEAD and modified in Bug 10190: (QA follow-up): Fix removal of tables. Version Bug 10190: (QA follow-up): Fix removal of tables of installer/data/mysql/atomicupdate/bug_10190.pl left in tree. error: Failed to merge in the changes. Patch failed at 0001 Bug 10190: (QA follow-up): Fix removal of tables
Created attachment 191795 [details] [review] Bug 10190: (QA follow-up): Fix removal of tables
Created attachment 191796 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules: - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set
(In reply to Clemens Tubach from comment #223) > Unfortunately, the patch doesn't apply: > > Applying: Bug 10190: (QA follow-up): Fix removal of tables > Using index info to reconstruct a base tree... > A installer/data/mysql/atomicupdate/bug_10190.pl > Falling back to patching base and 3-way merge... > CONFLICT (modify/delete): installer/data/mysql/atomicupdate/bug_10190.pl > deleted in HEAD and modified in Bug 10190: (QA follow-up): Fix removal of > tables. Version Bug 10190: (QA follow-up): Fix removal of tables of > installer/data/mysql/atomicupdate/bug_10190.pl left in tree. > error: Failed to merge in the changes. > Patch failed at 0001 Bug 10190: (QA follow-up): Fix removal of tables Apologies, that one's on me: it looks like I had left out 294be92e8e051570dbbfbb66f796e490dbecda27 when rebasing. I've amended the commit history to restore the commits order.
Patch applies now, but again Error 404 in staff interface kohadev-koha@kohadevbox:koha(bug_10190)$ updatedatabase Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $itemtype in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. Use of uninitialized value $branchcode_value in concatenation (.) or string at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190-mynewcircrule.pl [16:23:05]: Bug 10190 - Add overdue_X_has_rules to pre-existing overdue rule sets in the circulation_rules table DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl [16:23:05]: Bug 10190 - Migrate overduerules to circulation_rules Inserting :undef: default most frequent delay for overdue_1: Inserting :undef:undef default most frequent notice for overdue_1: Inserting :undef:undef default most frequent mtt for overdue_1: Inserting :undef:undef default most frequent restrict for overdue_1: 0 Inserting :undef: default most frequent delay for overdue_2: Inserting :undef:undef default most frequent notice for overdue_2: Inserting :undef:undef default most frequent mtt for overdue_2: Inserting :undef:undef default most frequent restrict for overdue_2: 0 Inserting :undef: default most frequent delay for overdue_3: Inserting :undef:undef default most frequent notice for overdue_3: Inserting :undef:undef default most frequent mtt for overdue_3: Inserting :undef:undef default most frequent restrict for overdue_3: 0
Created attachment 191805 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules (amended to account for comment 227) - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set
(In reply to Michaela Sieber from comment #227) > Patch applies now, but again Error 404 in staff interface > > kohadev-koha@kohadevbox:koha(bug_10190)$ updatedatabase > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. > Use of uninitialized value $itemtype in concatenation (.) or string at > /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. > Use of uninitialized value $itemtype in concatenation (.) or string at > /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. > Use of uninitialized value $itemtype in concatenation (.) or string at > /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 218. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 229. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 240. > Use of uninitialized value $branchcode_value in concatenation (.) or string > at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl line 251. > DEV atomic update > /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190-mynewcircrule. > pl [16:23:05]: Bug 10190 - Add overdue_X_has_rules to pre-existing overdue > rule sets in the circulation_rules table > DEV atomic update > /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_10190.pl [16:23:05]: > Bug 10190 - Migrate overduerules to circulation_rules > Inserting :undef: default most frequent delay for overdue_1: > Inserting :undef:undef default most frequent notice for overdue_1: > Inserting :undef:undef default most frequent mtt for overdue_1: > Inserting :undef:undef default most frequent restrict for overdue_1: 0 > Inserting :undef: default most frequent delay for overdue_2: > Inserting :undef:undef default most frequent notice for overdue_2: > Inserting :undef:undef default most frequent mtt for overdue_2: > Inserting :undef:undef default most frequent restrict for overdue_2: 0 > Inserting :undef: default most frequent delay for overdue_3: > Inserting :undef:undef default most frequent notice for overdue_3: > Inserting :undef:undef default most frequent mtt for overdue_3: > Inserting :undef:undef default most frequent restrict for overdue_3: 0 Thanks for letting me know - I have updated bug_10190-mynewcircrule.pl - particularly the SQL query which had a few issues. If anything else comes up, I will be sure to get back to this tomorrow!
Created attachment 191854 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules (amended to account for comment 227) - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set - address bug_10190.pl db script warnings (undef values were being concatenated with strings) - add a guard clause to bug_101090.pl: check that the tables the script pulls the data from do still exist, otherwise exit.
Created attachment 191873 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules (amended to account for comment 227) - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set - address bug_10190.pl db script warnings (undef values were being concatenated with strings) - add a guard clause to bug_101090.pl: check that the tables the script pulls the data from do still exist, otherwise exit.
I got around 28 tests failing. On a ktd started with the patches. And also when starting on main and upgrading. And with defaultCommandTimeout: 20000 (that's what is usually needed on my slow laptop) Is there something else to do to run the tests besides starting KTD and running them directly? cypress run --config video=false --spec t/cypress/integration/Admin/CirculationTriggers_spec.ts Otherwise that looks like a legit failure. For example, for "Should display correct breadcrumbs", the .current class is indeed not on "Home". --- For the next patch version, can the steps to perform an upgrade added to the test plan? From what I have gathered it's: sudo perl /kohadevbox/misc4dev/cp_debian_files.pl --instance=kohadev --koha_dir=/kohadevbox/koha --gitify_dir=/kohadevbox/gitify yarn build But maybe they are wrong. And it took me some time to find about needing cp_debian_files.pl ---------- If I want to test the migration of the old triggers. Is there any interesting data to test? Or do I just put multiple triggers and check that the most popular stuff is picked as default? ------- 1. open the form "Confirm trigger context" 2. click on the "Patron category" list 3. try to scroll because you aren't finding the desired category 4. It barely scrolls. You have to thing about moving the mouse outside of the list and scroll there. If the size of the things lines up enough, when trying to scroll you don't get to see the beginning of the next patron category. So you can really think the list is 3 or 4 elements long and that's all. I almost reported that the list was incomplete instead of reporting this. ------ When not logged into Koha and going to http://localhost:8081/cgi-bin/koha/admin/circulation_triggers One is redirected to the login page. After login, the redirection is done to http://localhost:8081/cgi-bin/koha/admin/circulation_triggers.pl Which isn't valid and goes 404. I though I missed some setup step at some point. But it was just an invalid redirect. Maybe it's from the rewrite rule in debian/templates/apache-shared-intranet.conf
(In reply to Victor Grousset/tuxayo from comment #232) > I got around 28 tests failing. On a ktd started with the patches. And also > when starting on main and upgrading. And with defaultCommandTimeout: 20000 > (that's what is usually needed on my slow laptop) > > Is there something else to do to run the tests besides starting KTD and > running them directly? > cypress run --config video=false --spec > t/cypress/integration/Admin/CirculationTriggers_spec.ts > > Otherwise that looks like a legit failure. For example, for "Should display > correct breadcrumbs", the .current class is indeed not on "Home". > > > --- > > For the next patch version, can the steps to perform an upgrade added to the > test plan? > > From what I have gathered it's: > > sudo perl /kohadevbox/misc4dev/cp_debian_files.pl --instance=kohadev > --koha_dir=/kohadevbox/koha --gitify_dir=/kohadevbox/gitify > yarn build > > But maybe they are wrong. And it took me some time to find about needing > cp_debian_files.pl > > ---------- > > If I want to test the migration of the old triggers. Is there any > interesting data to test? Or do I just put multiple triggers and check that > the most popular stuff is picked as default? > > ------- > > 1. open the form "Confirm trigger context" > 2. click on the "Patron category" list > 3. try to scroll because you aren't finding the desired category > 4. It barely scrolls. > > You have to thing about moving the mouse outside of the list and scroll > there. > If the size of the things lines up enough, when trying to scroll you don't > get to see the beginning of the next patron category. So you can really > think the list is 3 or 4 elements long and that's all. I almost reported > that the list was incomplete instead of reporting this. > > ------ > > When not logged into Koha and going to > http://localhost:8081/cgi-bin/koha/admin/circulation_triggers > One is redirected to the login page. > After login, the redirection is done to > http://localhost:8081/cgi-bin/koha/admin/circulation_triggers.pl > Which isn't valid and goes 404. > I though I missed some setup step at some point. But it was just an invalid > redirect. > Maybe it's from the rewrite rule in > debian/templates/apache-shared-intranet.conf Hi Victor, Many thanks for the feedback! I have a few UI/UX and permission fixes in the work (which includes dropdown select / scroll behaviour improvements, and should address what you've reported here), working from testing feedback from Michaela, and I'm aiming to resubmit by Friday this week. I'll be taking a look at the failing cypress tests as well. For data migration testing, what you've described is in effect what I have done. I haven't stumbled upon any particular edge case that needs looking into. The login/redirect fail: I'll need to look into that one. What I can say is the URL needs to be http://localhost:8081/cgi-bin/koha/admin/circulation_triggers, not http://localhost:8081/cgi-bin/koha/admin/circulation_triggers.pl. I'll see if the RewriteRules can help resolve this. I did find the new circulation triggers RewriteRule was missing from etc/koha-httpd.conf (fix not submitted yet - another one for Friday!)
> which includes dropdown select / scroll behaviour improvements :D ----- > I'll be taking a look at the failing cypress tests as well. Thanks, it's very handy we can have UI tests due to being done with Vue ^^ ------ I forgot there was also this failure in the lower level tests: t/db_dependent/Overdues.t .. 8/19 # Looks like you planned 19 tests but ran 9. I don't know if that's just the expected change from the removed tests or if there is an actual issue. -------- > Inserting :undef: default most frequent delay for overdue_1: > Inserting :undef:undef default most frequent notice for overdue_1: > Inserting :undef:undef default most frequent mtt for overdue_1: About this output, at first I though there where issues of undef variables. Like with the uninitialized values earlier reports. , does "all" fits better than undef? I picked it from the circulation rules page but maybe it doesn't have the same meaning? It's only about not scaring an admin about the update having an issue. -------- I tried on main to add two default actions and one action on a specific library. So 3 records in overduerules tables. And the update output is Inserting :J: overdue_1_delay: 2 Inserting :J: overdue_1_notice: AR_COMPLETED Inserting :J: overdue_1_restrict: 1 Inserting :J: overdue_1has_rules: 1 Inserting :J: overdue_2_delay: 6 Inserting :J: overdue_2_restrict: 1 Inserting :J: overdue_2has_rules: 1 Inserting :J: overdue_3_delay: 9 Inserting :J: overdue_3_notice: AR_CANCELED Inserting :J: overdue_3_restrict: 1 Inserting :J: overdue_3has_rules: 1 Inserting :HB: overdue_1_delay: 2 Inserting :HB: overdue_1_notice: ACCOUNT_DEBIT Inserting :HB: overdue_1_mtt: email,print Inserting :HB: overdue_1_restrict: 1 Inserting :HB: overdue_1has_rules: 1 Inserting :HB: overdue_2_delay: 5 Inserting :HB: overdue_2_notice: ACCOUNT_WRITEOFF Inserting :HB: overdue_2_mtt: email,print Inserting :HB: overdue_2_restrict: 1 Inserting :HB: overdue_2has_rules: 1 Inserting :HB: overdue_3_delay: 6 Inserting :HB: overdue_3_notice: PREDUEDGST Inserting :HB: overdue_3_mtt: email,print Inserting :HB: overdue_3_restrict: 1 Inserting :HB: overdue_3has_rules: 1 Inserting :undef: default most frequent delay for overdue_1: Inserting :undef:undef default most frequent notice for overdue_1: Inserting :undef:undef default most frequent mtt for overdue_1: Inserting :undef:undef default most frequent restrict for overdue_1: 0 Inserting :undef:undef default has_rules for overdue_1: 1 Inserting :undef: default most frequent delay for overdue_2: Inserting :undef:undef default most frequent notice for overdue_2: Inserting :undef:undef default most frequent mtt for overdue_2: Inserting :undef:undef default most frequent restrict for overdue_2: 0 Inserting :undef:undef default has_rules for overdue_2: 1 Inserting :undef: default most frequent delay for overdue_3: Inserting :undef:undef default most frequent notice for overdue_3: Inserting :undef:undef default most frequent mtt for overdue_3: Inserting :undef:undef default most frequent restrict for overdue_3: 0 Inserting :undef:undef default has_rules for overdue_3: 1 Inserting FFL:undef: default most frequent delay for overdue_1: 1 Inserting FFL:undef:undef default most frequent notice for overdue_1: AR_CANCELED Inserting FFL:undef:undef default most frequent mtt for overdue_1: email Inserting FFL:undef:undef default most frequent restrict for overdue_1: 1 Inserting FFL:undef:undef default has_rules for overdue_1: 1 Inserting FFL:undef: default most frequent delay for overdue_2: 2 Inserting FFL:undef:undef default most frequent notice for overdue_2: PREDUEDGST Inserting FFL:undef:undef default most frequent mtt for overdue_2: email Inserting FFL:undef:undef default most frequent restrict for overdue_2: 1 Inserting FFL:undef:undef default has_rules for overdue_2: 1 Inserting FFL:undef: default most frequent delay for overdue_3: 3 Inserting FFL:undef:undef default most frequent notice for overdue_3: ODUE Inserting FFL:undef:undef default most frequent mtt for overdue_3: email Inserting FFL:undef:undef default most frequent restrict for overdue_3: 1 Inserting FFL:undef:undef default has_rules for overdue_3: 1 Does anyone know that are expectable sizes of the overduerules table with a large network of libraries? Because even without much per-library specific rules, the output of this bug might be much longer than the rest of the upgrade and flood it. While still being very useful here to test and review. And eventually investigate issues when production upgrades will happen. Wait, for that, this should do: SELECT * FROM circulation_rules WHERE rule_name LIKE 'overdue\_%'; It's not as nice as with the explicit "frequent delay" but that might be a trade-off. Anyway it depends on how large all the current output can be. -------- > I'm aiming to resubmit by Friday this week. No worries, I hope there won't be too many hurdles. And the stuff about the upgrade output can wait the next round of patch revisions, actual testing doesn't depend on it.
There is a ghost of PTFS Europe still haunting the the copyright notice in admin/circulation_triggers.pl
qa script is finding various issues. Like tidy, spelling, filters, permissions. All minor stuff. Tidy stuff might be a pain, we shouldn't use tidy commits anymore, it should be done inline. If the untidy stuff comes from many commits (maybe old commits were done on a machine without the git hook that should tidy automatically), then it would be bad to have all the fixes in the last commit. If that's the situation, it might work to do an interactive rebase and mark all commits for "edit". Then amend them one by one without any change, but it should trigger the tidy hook. Hopefully that doesn't start doing tons of conflicts if commits touch the same places that were untidy. In that case, I think it would be a ok have tidy commit. The history on main shows there are a very few other tidy commits. So we can sneak another one if that's needed to fix an old mess without being a total pain. Hope that helps. ---- Code coverage is good, whether on new code and changes being covered. And on test removal not removing coverage on any still existing code.
Thought I would drop a quick progress update, seeing as I'm not ready to resubmit just yet: What I got sorted: - some UI fixes written - permissions are getting there - removed the link on the Tools page (this is an admin configuration option) The permissions piece has turned out to be more complex than I'd anticipated though I do think it's necessary to add this in properly, so I'm happy to be spending the time on it now :) That does mean that I'm going to push back the resubmission to next week, as I'm still working through my todo list for this. (I'll also rebase it against main to account for 41315 as that one's now been merged.)
(In reply to Victor Grousset/tuxayo from comment #236) > qa script is finding various issues. Like tidy, spelling, filters, > permissions. All minor stuff. > > Tidy stuff might be a pain, we shouldn't use tidy commits anymore, it should > be done inline. If the untidy stuff comes from many commits (maybe old > commits were done on a machine without the git hook that should tidy > automatically), then it would be bad to have all the fixes in the last > commit. > If that's the situation, it might work to do an interactive rebase and mark > all commits for "edit". Then amend them one by one without any change, but > it should trigger the tidy hook. Hopefully that doesn't start doing tons of > conflicts if commits touch the same places that were untidy. In that case, I > think it would be a ok have tidy commit. The history on main shows there are > a very few other tidy commits. So we can sneak another one if that's needed > to fix an old mess without being a total pain. > > Hope that helps. > > > ---- > > > Code coverage is good, whether on new code and changes being covered. And on > test removal not removing coverage on any still existing code. Currently working my way through said issues, and I've got a quick question on one of them if that's alright: [FAIL] koha-tmpl/intranet-tmpl/prog/js/vue/stores/circulation-rules.js FAIL forbidden_patterns forbidden pattern: Use __("") instead of _("") (line 35) seems to have been triggered by this regex: /overdue_(\d+)_has_rules/. Would that be a case of the qa script picking up on something it shouldn't?
(In reply to Chloé Zermatten from comment #238) > [FAIL] koha-tmpl/intranet-tmpl/prog/js/vue/stores/circulation-rules.js > FAIL forbidden_patterns > forbidden pattern: Use __("") instead of _("") (line 35) > > seems to have been triggered by this regex: /overdue_(\d+)_has_rules/. > > Would that be a case of the qa script picking up on something it shouldn't? Look like it, yes. Ignore it. You can open an issue to report the problem https://gitlab.com/koha-community/qa-test-tools
Created attachment 192871 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192872 [details] [review] Bug 10190: DBIC Schema update Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192873 [details] [review] Bug 10190: Remove OverdueRule Koha Objects Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192874 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192875 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192876 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192877 [details] [review] Bug 10190: Rework logic of overdue_notices This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192878 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192879 [details] [review] Bug 10190: Complete Vue.js rewrite of Circulation Triggers administration interface This patch introduces a modern Vue.js-based interface for managing circulation triggers (overdue notice/status triggers), replacing the legacy Tools-based interface with a more intuitive, feature-rich administration page. New Features Enhanced Visibility & Navigation - Exhaustive effective rule lists showing all rules that apply to each context - Toggle between "all exhaustive rules" and "explicitly set rules" views - Advanced filtering by patron category and/or item type - Clear display of fallback/default rules for each context Rule Management Actions - Delete triggers (last trigger in a sequence for any given library) - Reset entire rule sets for a given context (with safeguards) - Edit existing triggers with improved context awareness - Add new triggers with intelligent delay constraints Improved User Experience - Multi-step form wizard with clear workflow progression - Context-aware letter template filtering (only shows applicable templates) - Placeholder values showing fallback rules when clearing inputs - Dynamic min/max constraints for delay values based on neighboring triggers - Real-time form validation and feedback - Partial page reloads (only reload affected sections, not entire page) - Specific loading messages for different operations Technical Improvements - Centralized Vuex store for state management - Reusable CirculationTriggersForm component - API clients for libraries, circulation rules, item types, and patron categories - Consistent async/await pattern throughout - Separation of repositories, utilities, and services - Improved naming consistency and code maintainability - "Dumb" TriggersTable component for better separation of concerns Test Plan Prerequisites 1. Log in to staff interface with appropriate permissions (tools_edit_notice_status_triggers) 2. Ensure you have test data: multiple libraries, patron categories, item types, and notice templates 3. Navigate to Administration > Circulation triggers (new interface) Part 1: Basic Navigation & Interface 1. Verify the new "Circulation triggers" link appears in Administration home under the Circulation section 2. Access the new interface and confirm it loads successfully 3. Verify the default view shows either "All libraries" or your logged-in library based on DefaultToLoggedInLibraryOverdueTriggers system preference 4. Confirm the interface displays a list of triggers organized by context (library/patron category/item type combinations) Part 2: Viewing and Filtering Rules 1. Toggle between "All exhaustive rules" and "Explicitly set rules" views - Verify "All exhaustive rules" shows inherited/fallback rules - Verify "Explicitly set rules" shows only rules you've specifically configured 2. Test the patron category filter: - Select a specific patron category - Confirm only rules for that category are displayed 3. Test the item type filter: - Select a specific item type - Confirm only rules for that item type are displayed 4. Test combined filters (patron category + item type simultaneously) 5. Clear filters and verify all rules display again Part 3: Adding New Triggers 1. Click "Add trigger" button 2. Step 1 - Confirm Context: - Select Library (required) - Select Patron category (required) - Select Item type (required) - Click "Confirm context" 3. Step 2 - Review Existing Triggers: - Verify existing triggers for this context are displayed - Note the trigger numbers and delays 4. Step 3 - Configure New Trigger: - Enter a delay value (should be constrained by neighboring triggers) - Try entering an invalid delay (too low or too high) - verify validation - Set "Restricts checkouts" (Yes/No/Fallback to default) - Note the placeholder showing the fallback value - Select a letter template - Verify only templates for the selected library context appear - Set "Debarring" options if desired - Select "Charged notice fees" if applicable 5. Submit the form 6. Verify the new trigger appears in the list without a full page reload 7. Verify success message is displayed Part 4: Editing Existing Triggers 1. Locate an existing trigger in the list 2. Click the edit icon/button 3. Verify the form pre-populates with existing values 4. Test delay modification: - Try changing the delay within valid bounds - should succeed - Try changing delay beyond neighboring triggers - should show validation error - Use the increment/decrement buttons to adjust delay 5. Test clearing values: - Clear a field (e.g., delay or restrict checkouts) - Verify placeholder shows the fallback/default value - Submit with cleared value - should save as null and inherit fallback 6. Change the letter template to a different valid template 7. Submit changes 8. Verify the trigger updates in the list without full page reload 9. Verify the changes persist after refreshing the page Part 5: Deleting Triggers 1. Identify a trigger that is the last in its sequence 2. Click the delete button/icon 3. Verify a confirmation dialog appears 4. Confirm deletion 5. Verify the trigger is removed from the list without page reload 6. Verify the deletion persists after page refresh 7. Test delete constraints: - Try to delete a trigger that's not the last in sequence - Verify appropriate error/warning message Part 6: Resetting Rule Sets 1. Locate a context with explicitly set rules (not just inherited) 2. Ensure this is not the only rule set for this trigger sequence 3. Click the "Reset" button for that context 4. Verify confirmation dialog explains what will be reset 5. Confirm the reset 6. Verify the explicit rules are removed and context now shows inherited rules 7. Verify changes persist after page refresh 8. Test reset constraints: - Try to reset when it would leave no rules for that trigger - Verify appropriate warning/prevention Part 7: Context-Specific Behavior 1. Test library-specific letter filtering: - Create/edit a trigger for a specific library - Verify only letter templates with matching branchcode or default templates appear - Switch to a different library context - Verify the letter list updates accordingly 2. Test default rules: - View rules for "All libraries" / "All patron categories" / "All item types" - Verify these are labeled as "Default rule for [context]" - Confirm these serve as fallback for more specific rules 3. Test DefaultToLoggedInLibraryOverdueTriggers preference: - Set preference to "Yes" - Reload circulation triggers page - Verify it defaults to your logged-in library - Set preference to "No" - Verify it defaults to "All libraries" Part 8: Form Workflow & Validation 1. Test required fields: - Try submitting add/edit form without selecting library - should fail - Try submitting without patron category - should fail - Try submitting without item type - should fail 2. Test stepper navigation: - Start adding a trigger - Verify you must confirm context before proceeding - Verify you see existing triggers before editing/adding - Verify trigger form only appears after context confirmation 3. Test cancellation: - Start adding/editing a trigger - Click Cancel - Verify you return to the list without changes - Verify no partial data was saved Part 9: Loading States & Messages 1. On slow connections or with many rules: - Verify "Loading context..." appears while fetching context data - Verify "Loading rule set information..." appears while fetching rules - Verify section-specific loading messages appear during updates 2. After form submission: - Verify appropriate success/error messages display - Verify loading indicators during async operations Part 10: Permissions 1. Log in as a user without tools_edit_notice_status_triggers permission 2. Verify the "Circulation triggers" link does not appear in Administration 3. Try accessing /cgi-bin/koha/admin/circulation_triggers directly 4. Verify access is denied Part 11: Data Integrity & Persistence 1. Create a complex set of rules with multiple triggers across different contexts 2. Refresh the page and verify all rules persist 3. Restart the Koha services 4. Verify all rules still display correctly 5. Check the database circulation_rules table: - Verify rules are stored with correct rule_name pattern (overdue_1_delay, etc.) - Verify null values are properly stored for inheritance - Verify the overdue_X_has_rules rule kind is present Part 12: Edge Cases 1. Test with no existing triggers - verify empty state message 2. Test with maximum trigger sequences (e.g., trigger 1, 2, 3) 3. Test rapid successive edits 4. Test with special characters in letter template names 5. Test browser back/forward buttons during form workflow Part 13: Comparison with Legacy Interface 1. Access the old interface if still available (via Tools menu) 2. Create a trigger using the new interface 3. Verify it appears correctly in the old interface (if applicable) 4. Create a trigger using the old interface (if still available) 5. Verify it appears correctly in the new interface
Created attachment 192880 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192881 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192882 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192883 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 192884 [details] [review] Bug 10190: Comprehensive Cypress tests for Circulation Triggers UI This patch significantly enhances the Cypress test coverage for the new Circulation Triggers administration interface, expanding from basic breadcrumb checks to comprehensive end-to-end testing. Test Coverage: - Initial page load and API error handling - Context filtering (library, patron category, item type) - View toggling between explicit and all applicable rules - Tab navigation between multiple triggers - Full CRUD operations (Create, Read, Update, Delete) - Rule set reset functionality - Letter template filtering by library context - Form validation and constraint checking - Loading states and empty states - Error handling for all operations - Browser navigation (back/forward buttons) - Complex scenarios with inheritance hierarchies New Features: - Mock data helper functions for test fixtures - Comprehensive API interception for all endpoints - Tests for both success and error paths - Placeholder value testing for fallback rules - Increment/decrement button testing - Multi-step form workflow validation - Confirmation dialog testing for destructive operations The tests follow Koha's Cypress testing patterns established in the ERM module, using proper API mocking, clear test descriptions, and organized test suites.
Created attachment 192885 [details] [review] Bug 10190: (QA follow-up): Fix removal of tables
Created attachment 192886 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules (amended to account for comment 227) - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set - address bug_10190.pl db script warnings (undef values were being concatenated with strings) - add a guard clause to bug_101090.pl: check that the tables the script pulls the data from do still exist, otherwise exit. - remove duplicate Letters options in the circulation triggers add / edit form
Created attachment 192887 [details] [review] Bug 10190: feat: permissions - add a config API endpoint to Circulations Triggers so permissions can be fetched - update the permissions in the Vue App so that manage_circ_rules is required - update the permissions in the Circulation Rules API so that manage_circ_rules is required for PUT operations - update the circulation-rules store to so that it is compatible with recently intorduced compostables - implement a redirect into the route so that permission requirements are honoured
Created attachment 192888 [details] [review] Bug 10190: chore: remove circ triggers link from tools menu
Created attachment 192889 [details] [review] Bug 10190: refactor: remove redundant try..catch
Created attachment 192890 [details] [review] Bug 10190: (QA follow-up): fix unix permissions on bug_10190.pl
Created attachment 192891 [details] [review] Bug 10190: (QA follow-up): fix unix permissions on circulation_triggers.pl
Created attachment 192892 [details] [review] Bug 10190: (QA follow-up): chore: add new line at end of MessageTransportType.pm
Created attachment 192893 [details] [review] Bug 10190: (QA follow-up): chore: amend copyright statement
Created attachment 192894 [details] [review] Bug 10190: (QA follow-up): fix: typo
Created attachment 192895 [details] [review] Bug 10190: feat: enhance trigger context display in modal - improve modal scroll behaviour - improve modal dimensions - display contexts as a horizontal list to make better use of the available space - fully contain dropdowns within the modal body
Created attachment 192896 [details] [review] Bug 10190: (QA follow-up): fix: add missing filter
Created attachment 192897 [details] [review] Bug 10190: feat: trigger add/edit form UI/UX enhancements - scroll to the add/edit form on button click - prevent redundant full page reload upon the user swapping between adding and editing while the modal is open - ensure robust scroll behaviour, handling: - the URL being accessed directly - the edit button in the table in the main page being used to access the form directly - the stepper workflow being used - the user swapping between add and edit once already looking at the form - dynamic display of 'Transport Type' consistency: only show if a 'Notice' has been selected - if a notice has been selected, prevent form submission if transport type isn't set and display an alert on submission attempt
Created attachment 192898 [details] [review] Bug 10190: (QA follow-up): feat: swap undef for all in update db output
Created attachment 192899 [details] [review] Bug 10190: fix: prevent duplicate ruleset load - prevent the initial API call and subsequent exhaustive applied ruleset list generation logic from running twice
resubmitting this now as a fair few changes have been made, and I think it's a good time for another round of testing / feedback :) The changes are listed in the bug but highlights are pretty much: - permissions have been implemented - improved UI/UX on the edit/add form - fixes based on qa script outcomes - rebased against main (which now includes 41315) - performance: a fix to prevent the applied rule sets from being loaded twice upon initial UI load Test plan for the perl script: https://docs.google.com/document/d/1RJVz-jwks4OBymC5HaF0GBsJ4xQCZ9Qay03lnfoRMEs/edit?usp=sharing
Today testing results: 1.) Still problems with permissions: - permission error 403 with restricted user 192.168.56.1 - - [11/Feb/2026:16:29:39 +0000] "GET /api/v1/app.pl/api/v1/patron_categories?_per_page=-1 HTTP/1.1" 403 - "http://192.168.56.101:8081/cgi-bin/koha/admin/circulation_triggers" 192.168.56.1 - - [11/Feb/2026:16:29:40 +0000] "GET /api/v1/app.pl/api/v1/circulation_rules/config HTTP/1.1" 403 - "http://192.168.56.101:8081/cgi-bin/koha/admin/circulation_triggers" To reproduce : Edit an existing borrower (e.g. Casey King) and give Permissions for staff, reports and manage_circ_rules and edit_notice_status_triggers Login with this user. Verify that you see Circulation Triggers under Administration. Click on Link "Circulation Triggers" , then you see the error above for two seconds before you are redirected to Koha start page. Question : is the permission edit_notice_status_triggers still relevant? It is still under Tools and not under "Manage Koha system settings (Administration panel) (parameters)" 2.) default x-Button in transport types (Modal edit trigger) - for users it seems that only one transport type is possible To reproduce : Edit a trigger and go to the section "Edit notice for trigger x" select a letter, this adds a new line for transport types with the mentioned default x-Button that prevents to add more than one transport type (e.g. Print+email) 3.) Transport type should be required field if letter exists It makes no sense to have a notice without a definition how the notice is sent. The transport type should therefor be a required field @Chloé : I've sent you Screenshots in Mattermost 4.) Migration scenario We found some odd behavior but need more time to document the scenario. TBC
There is a fundamental conceptual error in the migration of the overdue rules with branch code. No default rules must be created here, since there were no default rules in the previous version either. The identification of “common” rules is far too simplistic. Consider the following scenario: Your library has 10 patron categories. Branch Centerville: for 6 of these categories, an overdue rule is configured with delay = 3 and the ODUE letter sent by email. However, for the remaining four categories, no rule is configured at all. In the current version, these four categories therefore do not receive any overdue notices. With the migration, you would now create a default rule that would also apply to these four categories. This is incorrect, because we must not make assumptions or create default rules for other institutions whose specific setup and context we do not know. Our conclusion is that only explicit rules may be migrated. This applies both to rules with a branch code and to those without a branch code. We strongly recommend testing with existing, differing overdue rules. For this purpose, we have created ready-to-use SQL insert statements and would be happy to provide them for testing, so that the various migration scenarios can be tested more easily. This is what we've done for testing with KTD: git checkout main ktd up ktd --shell koha-mysql kohadev SQL-Insert statements for overduerules and overduerules_transport_types : INSERT INTO overduerules (overduerules_id, branchcode, categorycode, delay1, letter1, debarred1, delay2, debarred2, letter2, delay3, letter3, debarred3) VALUES (1, '', 'HB', 2, 'ODUE', 0, 4, 0, 'ODUE', 6, 'ODUE', 0), (2, '', 'B', 1, 'ODUE', 0, 2, 0, 'ODUE', 3, 'ODUE', 1), (3, '', 'K', 3, 'ODUE', 0, 6, 0, 'ODUE', 9, 'ODUE', 1), (4, 'LPL', 'K', 5, 'ODUE', 0, 10, 1, 'ODUE', 0, '', 0), (5, 'LPL', 'ST', 7, 'ODUE', 0, 14, 1, 'ODUE', 0, '', 0), (6, 'LPL', 'YA', 10, 'ODUE', 0, 20, 1, '', 0, '', 0), (7, 'SPL', 'SC', 8, 'DUE', 1, 0, 0, '', 0, '', 0); INSERT INTO overduerules_transport_types (id, letternumber, message_transport_type, overduerules_id) VALUES (1, 1, 'print', 1), (2, 2, 'print', 1), (3, 3, 'print', 1), (4, 1, 'email', 2), (5, 2, 'email', 2), (6, 3, 'email', 2), (7, 3, 'print', 2), (8, 1, 'email', 3), (9, 1, 'print', 3), (10, 2, 'email', 3), (11, 2, 'print', 3), (12, 3, 'print', 3), (13, 1, 'email', 4), (14, 2, 'print', 4), (15, 1, 'email', 5), (16, 2, 'email', 5), (17, 2, 'print', 5), (18, 1, 'email', 6); Now everything is prepared for testing the migration scenario. Check the overduerule in Koha staff interface before you apply the patch! # if branch already exists, delete it : git branch -D bug_10190 git checkout -b bug_10190 origin/main git bz apply 10190 updatedatabase yarn build # copies the Apache-Config files to the correct place sudo perl /kohadevbox/misc4dev/cp_debian_files.pl --instance=kohadev --koha_dir=/kohadevbox/koha --gitify_dir=/kohadevbox/gitify restart_all Check the migrated circulation triggers.
Just an FYI: this patch set may conflict with Bug 30515 and Bug 40791.
(In reply to Lucas Gass (lukeg) from comment #272) > Just an FYI: this patch set may conflict with Bug 30515 and Bug 40791. Thanks for the heads up!
Created attachment 193864 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193866 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193867 [details] [review] Bug 10190: DBIC Schema update Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193868 [details] [review] Bug 10190: Remove OverdueRule Koha Objects Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193869 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193870 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193871 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193872 [details] [review] Bug 10190: Rework logic of overdue_notices This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193873 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193874 [details] [review] Bug 10190: Complete Vue.js rewrite of Circulation Triggers administration interface This patch introduces a modern Vue.js-based interface for managing circulation triggers (overdue notice/status triggers), replacing the legacy Tools-based interface with a more intuitive, feature-rich administration page. New Features Enhanced Visibility & Navigation - Exhaustive effective rule lists showing all rules that apply to each context - Toggle between "all exhaustive rules" and "explicitly set rules" views - Advanced filtering by patron category and/or item type - Clear display of fallback/default rules for each context Rule Management Actions - Delete triggers (last trigger in a sequence for any given library) - Reset entire rule sets for a given context (with safeguards) - Edit existing triggers with improved context awareness - Add new triggers with intelligent delay constraints Improved User Experience - Multi-step form wizard with clear workflow progression - Context-aware letter template filtering (only shows applicable templates) - Placeholder values showing fallback rules when clearing inputs - Dynamic min/max constraints for delay values based on neighboring triggers - Real-time form validation and feedback - Partial page reloads (only reload affected sections, not entire page) - Specific loading messages for different operations Technical Improvements - Centralized Vuex store for state management - Reusable CirculationTriggersForm component - API clients for libraries, circulation rules, item types, and patron categories - Consistent async/await pattern throughout - Separation of repositories, utilities, and services - Improved naming consistency and code maintainability - "Dumb" TriggersTable component for better separation of concerns Test Plan Prerequisites 1. Log in to staff interface with appropriate permissions (tools_edit_notice_status_triggers) 2. Ensure you have test data: multiple libraries, patron categories, item types, and notice templates 3. Navigate to Administration > Circulation triggers (new interface) Part 1: Basic Navigation & Interface 1. Verify the new "Circulation triggers" link appears in Administration home under the Circulation section 2. Access the new interface and confirm it loads successfully 3. Verify the default view shows either "All libraries" or your logged-in library based on DefaultToLoggedInLibraryOverdueTriggers system preference 4. Confirm the interface displays a list of triggers organized by context (library/patron category/item type combinations) Part 2: Viewing and Filtering Rules 1. Toggle between "All exhaustive rules" and "Explicitly set rules" views - Verify "All exhaustive rules" shows inherited/fallback rules - Verify "Explicitly set rules" shows only rules you've specifically configured 2. Test the patron category filter: - Select a specific patron category - Confirm only rules for that category are displayed 3. Test the item type filter: - Select a specific item type - Confirm only rules for that item type are displayed 4. Test combined filters (patron category + item type simultaneously) 5. Clear filters and verify all rules display again Part 3: Adding New Triggers 1. Click "Add trigger" button 2. Step 1 - Confirm Context: - Select Library (required) - Select Patron category (required) - Select Item type (required) - Click "Confirm context" 3. Step 2 - Review Existing Triggers: - Verify existing triggers for this context are displayed - Note the trigger numbers and delays 4. Step 3 - Configure New Trigger: - Enter a delay value (should be constrained by neighboring triggers) - Try entering an invalid delay (too low or too high) - verify validation - Set "Restricts checkouts" (Yes/No/Fallback to default) - Note the placeholder showing the fallback value - Select a letter template - Verify only templates for the selected library context appear - Set "Debarring" options if desired - Select "Charged notice fees" if applicable 5. Submit the form 6. Verify the new trigger appears in the list without a full page reload 7. Verify success message is displayed Part 4: Editing Existing Triggers 1. Locate an existing trigger in the list 2. Click the edit icon/button 3. Verify the form pre-populates with existing values 4. Test delay modification: - Try changing the delay within valid bounds - should succeed - Try changing delay beyond neighboring triggers - should show validation error - Use the increment/decrement buttons to adjust delay 5. Test clearing values: - Clear a field (e.g., delay or restrict checkouts) - Verify placeholder shows the fallback/default value - Submit with cleared value - should save as null and inherit fallback 6. Change the letter template to a different valid template 7. Submit changes 8. Verify the trigger updates in the list without full page reload 9. Verify the changes persist after refreshing the page Part 5: Deleting Triggers 1. Identify a trigger that is the last in its sequence 2. Click the delete button/icon 3. Verify a confirmation dialog appears 4. Confirm deletion 5. Verify the trigger is removed from the list without page reload 6. Verify the deletion persists after page refresh 7. Test delete constraints: - Try to delete a trigger that's not the last in sequence - Verify appropriate error/warning message Part 6: Resetting Rule Sets 1. Locate a context with explicitly set rules (not just inherited) 2. Ensure this is not the only rule set for this trigger sequence 3. Click the "Reset" button for that context 4. Verify confirmation dialog explains what will be reset 5. Confirm the reset 6. Verify the explicit rules are removed and context now shows inherited rules 7. Verify changes persist after page refresh 8. Test reset constraints: - Try to reset when it would leave no rules for that trigger - Verify appropriate warning/prevention Part 7: Context-Specific Behavior 1. Test library-specific letter filtering: - Create/edit a trigger for a specific library - Verify only letter templates with matching branchcode or default templates appear - Switch to a different library context - Verify the letter list updates accordingly 2. Test default rules: - View rules for "All libraries" / "All patron categories" / "All item types" - Verify these are labeled as "Default rule for [context]" - Confirm these serve as fallback for more specific rules 3. Test DefaultToLoggedInLibraryOverdueTriggers preference: - Set preference to "Yes" - Reload circulation triggers page - Verify it defaults to your logged-in library - Set preference to "No" - Verify it defaults to "All libraries" Part 8: Form Workflow & Validation 1. Test required fields: - Try submitting add/edit form without selecting library - should fail - Try submitting without patron category - should fail - Try submitting without item type - should fail 2. Test stepper navigation: - Start adding a trigger - Verify you must confirm context before proceeding - Verify you see existing triggers before editing/adding - Verify trigger form only appears after context confirmation 3. Test cancellation: - Start adding/editing a trigger - Click Cancel - Verify you return to the list without changes - Verify no partial data was saved Part 9: Loading States & Messages 1. On slow connections or with many rules: - Verify "Loading context..." appears while fetching context data - Verify "Loading rule set information..." appears while fetching rules - Verify section-specific loading messages appear during updates 2. After form submission: - Verify appropriate success/error messages display - Verify loading indicators during async operations Part 10: Permissions 1. Log in as a user without tools_edit_notice_status_triggers permission 2. Verify the "Circulation triggers" link does not appear in Administration 3. Try accessing /cgi-bin/koha/admin/circulation_triggers directly 4. Verify access is denied Part 11: Data Integrity & Persistence 1. Create a complex set of rules with multiple triggers across different contexts 2. Refresh the page and verify all rules persist 3. Restart the Koha services 4. Verify all rules still display correctly 5. Check the database circulation_rules table: - Verify rules are stored with correct rule_name pattern (overdue_1_delay, etc.) - Verify null values are properly stored for inheritance - Verify the overdue_X_has_rules rule kind is present Part 12: Edge Cases 1. Test with no existing triggers - verify empty state message 2. Test with maximum trigger sequences (e.g., trigger 1, 2, 3) 3. Test rapid successive edits 4. Test with special characters in letter template names 5. Test browser back/forward buttons during form workflow Part 13: Comparison with Legacy Interface 1. Access the old interface if still available (via Tools menu) 2. Create a trigger using the new interface 3. Verify it appears correctly in the old interface (if applicable) 4. Create a trigger using the old interface (if still available) 5. Verify it appears correctly in the new interface
Created attachment 193875 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193876 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193877 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193878 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 193879 [details] [review] Bug 10190: Comprehensive Cypress tests for Circulation Triggers UI This patch significantly enhances the Cypress test coverage for the new Circulation Triggers administration interface, expanding from basic breadcrumb checks to comprehensive end-to-end testing. Test Coverage: - Initial page load and API error handling - Context filtering (library, patron category, item type) - View toggling between explicit and all applicable rules - Tab navigation between multiple triggers - Full CRUD operations (Create, Read, Update, Delete) - Rule set reset functionality - Letter template filtering by library context - Form validation and constraint checking - Loading states and empty states - Error handling for all operations - Browser navigation (back/forward buttons) - Complex scenarios with inheritance hierarchies New Features: - Mock data helper functions for test fixtures - Comprehensive API interception for all endpoints - Tests for both success and error paths - Placeholder value testing for fallback rules - Increment/decrement button testing - Multi-step form workflow validation - Confirmation dialog testing for destructive operations The tests follow Koha's Cypress testing patterns established in the ERM module, using proper API mocking, clear test descriptions, and organized test suites.
Created attachment 193880 [details] [review] Bug 10190: (QA follow-up): Fix removal of tables Patch from commit d8fc872
Created attachment 193881 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules (amended to account for comment 227) - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set - address bug_10190.pl db script warnings (undef values were being concatenated with strings) - add a guard clause to bug_101090.pl: check that the tables the script pulls the data from do still exist, otherwise exit. - remove duplicate Letters options in the circulation triggers add / edit form
Created attachment 193882 [details] [review] Bug 10190: fix permissions - add a config API endpoint to Circulations Triggers so permissions can be fetched - update the permissions in the Vue App so that manage_circ_rules is required - update the permissions in the Circulation Rules API so that manage_circ_rules is required for PUT operations - update the circulation-rules store to so that it is compatible with recently intorduced compostables - implement a redirect into the route so that permission requirements are honoured
Created attachment 193883 [details] [review] Bug 10190: remove circ triggers link from tools menu Patch from commit 9cc23ff
Created attachment 193884 [details] [review] Bug 10190: refactor to remove redundant try..catch Patch from commit 2d3a737
Created attachment 193885 [details] [review] Bug 10190: (QA follow-up) fix unix permissions on bug_10190.pl Patch from commit 75c72bf
Created attachment 193886 [details] [review] Bug 10190: (QA follow-up) fix unix permissions on circulation_triggers.pl Patch from commit 77c082f
Created attachment 193887 [details] [review] Bug 10190: (QA follow-up) add new line at end of MessageTransportType.pm Patch from commit dc1da0a
Created attachment 193888 [details] [review] Bug 10190: (QA follow-up) amend copyright statement Patch from commit d8e0d75
Created attachment 193889 [details] [review] Bug 10190: (QA follow-up) typo Patch from commit 0b7cb77
Created attachment 193890 [details] [review] Bug 10190: enhance trigger context display in modal - improve modal scroll behaviour - improve modal dimensions - display contexts as a horizontal list to make better use of the available space - fully contain dropdowns within the modal body
Created attachment 193891 [details] [review] Bug 10190: (QA follow-up) add missing filter Patch from commit 274eec8
Created attachment 193892 [details] [review] Bug 10190: trigger add/edit form UI/UX enhancements - scroll to the add/edit form on button click - prevent redundant full page reload upon the user swapping between adding and editing while the modal is open - ensure robust scroll behaviour, handling: - the URL being accessed directly - the edit button in the table in the main page being used to access the form directly - the stepper workflow being used - the user swapping between add and edit once already looking at the form - dynamic display of 'Transport Type' consistency: only show if a 'Notice' has been selected - if a notice has been selected, prevent form submission if transport type isn't set and display an alert on submission attempt
Created attachment 193893 [details] [review] Bug 10190: (QA follow-up) swap undef for all in update db output Patch from commit e9264df
Created attachment 193894 [details] [review] Bug 10190: prevent duplicate ruleset load - prevent the initial API call and subsequent exhaustive applied ruleset list generation logic from running twice
Created attachment 193895 [details] [review] Bug 10190: Deprecate permission user_permissions rows do not need manual cleanup because the FK on user_permissions(module_bit, code) references permissions(module_bit, code) ON DELETE CASCADE, so deleting from permissions automatically removes per-patron rows.
Created attachment 193896 [details] [review] Bug 10190: rewrite atomicupdate with correct 1:1 rule migration The previous migration had two fundamental errors: 1. It manufactured phantom empty rows for all patron categories that had no rule, which corrupted the "most frequent" calculation. 2. It created categorycode=NULL (all-categories) default rules for every branchcode seen, silently extending overdue notice coverage to patron categories that had no configured rule in the old system. The backend (overdue_notices.pl) does not consult has_rules, so these invented defaults would cause notices to be sent to patrons that were deliberately uncovered. The rewrite migrates only explicit (branchcode, categorycode) pairs that had a positive delay configured. No default rules are created. branchcode='' maps to NULL as before. exit is replaced with return to avoid killing subsequent atomicupdate scripts.
Created attachment 193897 [details] [review] Bug 10190: fix three regressions in overdue_notices.pl - Store patron name/address fields in \$borrower_overdues so prepare_letter_for_printing has data for printed/CSV/text notices - Fix email->print fallback: look up items under original MTT key, not the effective (fallback) key - Set \$admin_email_address from \$library unconditionally in _enact_trigger; previously undef in non-patron_homelibrary mode, stripping the from_address on all queued notices
Created attachment 193898 [details] [review] Bug 10190: fix four more issues spotted in the overdues script - Patron.pm has_restricting_overdues: sort ASC not DESC so the oldest (most overdue) item is checked per branch/itemtype; DESC caused the deduplication skip to discard the very items most likely to trigger a restriction - overdue_notices.pl: fix --itemtypesout option name in GetOptions (was 'itemtypeouts', never matched the documented --itemtypesout flag) - overdue_notices.pl: fix typo 'catagorycode' -> 'categorycode' in --borcatout search, which silently broke category exclusion - circulation_triggers.pl: add flagsrequired => parameters/manage_circ_rules so the admin page enforces the correct permission; also add missing use Koha::Notice::Templates
Created attachment 193899 [details] [review] Bug 10190: correction to x-koha-authorization on /circulation_rules/config The config endpoint is used exclusively by the Circulation Triggers Vue app, which is gated on parameters/manage_circ_rules. Using acquisition/'*' and erm/1 was inconsistent copy-paste from an unrelated endpoint and would 403 circ-rules admins while granting access to unrelated module users.
Created attachment 193900 [details] [review] Bug 10190: three minor issues - overdue_notices.pl: fix verbose sprintf - remove spurious \$triggered argument (CLI 0/1 flag had replaced delay value in output) and use the \$borr string that was computed but never included in the message - overdue_notices.pl: remove %already_queued hash which was populated in _enact_trigger but never read; dead code since the refactor - C4/Overdues.pm GetBranchcodesWithOverdueRules: guard the global-rule expansion with @branchcodes check so an empty result (no rules at all) correctly returns an empty list instead of all branches
Created attachment 193901 [details] [review] Bug 10190: remove has_rules sentinel from circulation_rules table has_rules was a purely frontend marker stored as a circulation rule to signal that a context had explicitly-saved overdue trigger rules. The backend never reads it; the information is fully derivable from whether any of delay/notice/mtt/restrict are non-null for a given context. Replace all has_rules DB reads/writes with a hasExplicitRulesForTrigger() helper that checks the four real rule suffixes directly. has_rules remains as a computed in-memory concept in the Vue store (derived in findEffectiveRule) so templates require no changes. Also adds an atomicupdate to delete existing has_rules rows from the DB, and removes the has_rules insert from the bug_10190 migration script.
Created attachment 193902 [details] [review] Bug 10190: fix NaN trigger number when adding first rule for a library When no circulation rules exist for a library, triggerCounts[library_id] is undefined, causing undefined + 1 = NaN. Use nullish coalescing to default to 0 so the first trigger correctly gets number 1.
Created attachment 193903 [details] [review] Bug 10190: warn when no default triggers exist and show libraries with rules When viewing the default library context with no triggers defined, show a warning message explaining defaults apply to all libraries. If any libraries have specific triggers, list them as clickable links that switch the filter to that library. If no rules exist anywhere, show a prompt to add the first trigger.
Created attachment 193904 [details] [review] Bug 10190: Fix test count in overdues.t Patch from commit cd7c2cf
Created attachment 193905 [details] [review] Bug 10190: fix failing Permissions.t and overhaul CirculationTriggers Cypress spec Permissions.t: - Remove edit_notice_status_triggers from userpermissions.sql so fresh installs no longer seed this deprecated permission (existing installs are handled by the atomic update) - Add a defensive DELETE in the test to cover DBs that pre-date the atomic update CirculationTriggers_spec.ts: - Fix getMockCirculationRule: the helper was returning the old {rule_name, rule_value, branchcode, ...} format; update to the current API format {context: {...}, overdue_N_delay: ..., ...} - Remove assertions for UI elements that do not exist (success/error flash messages, "Existing triggers" heading, "rule set will be removed" text) - Fix reset confirmation text to match actual component output - Fix duplicate patron_category_id "ST" in mock patron categories - Remove aspirational tests that checked non-implemented behaviour - Add describe block for the no-default-triggers warning (no rules anywhere, rules only at specific libraries, library link switches the filter) - Add regression test for NaN trigger number when adding first rule for a library
Created attachment 193979 [details] [review] Bug 10190: Patron categories fetch shouldn't require full manage permission Patch from commit 2727bf7
General: Please revise the test plan, as it is no longer up to date. For example, overdue_X_has_rules no longer exists (Part 11.5) , and the permission is no longer available either (Part 10.1). We believe that this part is not testable at all: Part 13: Comparison with Legacy Interface 3. Verify it appears correctly in the old interface (if applicable) -- Strong contrasts and consistent color schemes for better understanding: Please use the same blue color as on circulation rules page due to poor contrast. Please use : rgb(218.1, 236.4, 251.4) = #DAECFB instead of : rgba(132, 193, 243, 1) = #84c1f3 ( this one is tooo dark) AND please use yellow color for Error Messages, as used elsewhere in Koha. -- Here are all the issues we found while going through the test plan. 1.) Still permission issues : Something went wrong: Error: Authorization failure. Missing required permission(s). --> Martin is working on this To fully use the CirculationTriggers app a user needs all of: circulate: circulate_remaining_permissions (read + write circ rules) parameters: manage_circ_rules (load the config/UI) catalogue: 1 (load item types and libraries) parameters: catalogue (load patron categories) 2.) Letter context it is not possible to select Letters with branchcode See Testplan - Part 3: Adding New Triggers - 4. Step 3 - Configure New Trigger - Verify only templates for the selected library context appear - Part 7: Context-Specific Behavior 1. Test library-specific letter filtering: - Create/edit a trigger for a specific library - Verify only letter templates with matching branchcode or default templates appear 3.) Editing Existing Triggers is not always possible editing the delay is only possible for all / all rules. have several rules for trigger 1 e.g. a all/all/all rule plus a all/Kids/Books with a different delay value (but same letter code as all/all/all rule) Now try to change the delay value. As soon as you press the submit button, you receive the following error message: "Your changes could not be saved as this circulation trigger was updated elsewhere. Please see the updated trigger below." See Testplan Part 4: Editing Existing Triggers 4.) Deletion of triggers does not delete all rules that are listed to reproduce: have several rules for trigger 2 e.g. a all/all/all rule plus a all/Kids/Books with a different delay value (but same letter code as all/all/all rule) --> Delete trigger, than the all/all/all rule is deleted and the entry for all/Kids/Books remains See Testplan: Part 5: Deleting Triggers 5.) No rules are displayed and syspref doesn't matter As soon as you access the page /cgi-bin/koha/admin/circulation_triggers, no rule sets are displayed; the list is empty. In addition, the system preference DefaultToLoggedInLibraryOverdueTriggers is not taken into account. See Testplan 3. Test DefaultToLoggedInLibraryOverdueTriggers preference: - Set preference to "Yes" - Reload circulation triggers page - Verify it defaults to your logged-in library - Set preference to "No" - Verify it defaults to "All libraries"
Created attachment 194045 [details] [review] Bug 10190: DB migration for overduerules This patch adds the data migration for overduerules to circulation rules. We attempt to create the default rules based on most commonly occuring rule values and then add overrides for all other rule values we find. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194046 [details] [review] Bug 10190: DBIC Schema update Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194047 [details] [review] Bug 10190: Remove OverdueRule Koha Objects Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194048 [details] [review] Bug 10190: Fix unit test to exclude _X_ rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194049 [details] [review] Bug 10190: Add support for incrementable circulation rules This patch adds support for _X_ grouped circulation rules to both the internal and external API's for handling circulation rules. This allows us to group overdue_X_rulename rules by an 'X' integer so we can have triggers incremented and grouped together. We also add the new overdue_x_* rule definition here for overdue trigger rules. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194050 [details] [review] Bug 10190: Update GetBranchcodesWithOverdueRules to use CirculationRules This patch updates the C4::Overdues method GetBranchecodesWithOverdueRules to refer to the new location of these rules in the circulation rules table. We also update the unit tests to move the mocked data into the right location. We could perhaps migrate this method in Koha::CirculationRules at a later date? Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194051 [details] [review] Bug 10190: Rework logic of overdue_notices This patch enhances the overdue_notices script to understand the newly introduced itemtype level for overdue rules. The original logic for digesting overdues in the overdue notices script was flawed given the additional itemtype handling introduced in this patchset and the way in which it handles effective fallbacks for rules. This patch tidies the file and reworks the logic so that we fetch overdues by patron category and branch and then iterate through those comparing to the correct effective rules and combing them into digest groups based on notice + trigger level. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194052 [details] [review] Bug 10190: Let the circulation_rules endpoint accept query parameters Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194053 [details] [review] Bug 10190: Complete Vue.js rewrite of Circulation Triggers administration interface This patch introduces a modern Vue.js-based interface for managing circulation triggers (overdue notice/status triggers), replacing the legacy Tools-based interface with a more intuitive, feature-rich administration page. New Features Enhanced Visibility & Navigation - Exhaustive effective rule lists showing all rules that apply to each context - Toggle between "all exhaustive rules" and "explicitly set rules" views - Advanced filtering by patron category and/or item type - Clear display of fallback/default rules for each context Rule Management Actions - Delete triggers (last trigger in a sequence for any given library) - Reset entire rule sets for a given context (with safeguards) - Edit existing triggers with improved context awareness - Add new triggers with intelligent delay constraints Improved User Experience - Multi-step form wizard with clear workflow progression - Context-aware letter template filtering (only shows applicable templates) - Placeholder values showing fallback rules when clearing inputs - Dynamic min/max constraints for delay values based on neighboring triggers - Real-time form validation and feedback - Partial page reloads (only reload affected sections, not entire page) - Specific loading messages for different operations Technical Improvements - Centralized Vuex store for state management - Reusable CirculationTriggersForm component - API clients for libraries, circulation rules, item types, and patron categories - Consistent async/await pattern throughout - Separation of repositories, utilities, and services - Improved naming consistency and code maintainability - "Dumb" TriggersTable component for better separation of concerns Test Plan Prerequisites 1. Log in to staff interface with appropriate permissions (tools_edit_notice_status_triggers) 2. Ensure you have test data: multiple libraries, patron categories, item types, and notice templates 3. Navigate to Administration > Circulation triggers (new interface) Part 1: Basic Navigation & Interface 1. Verify the new "Circulation triggers" link appears in Administration home under the Circulation section 2. Access the new interface and confirm it loads successfully 3. Verify the default view shows either "All libraries" or your logged-in library based on DefaultToLoggedInLibraryOverdueTriggers system preference 4. Confirm the interface displays a list of triggers organized by context (library/patron category/item type combinations) Part 2: Viewing and Filtering Rules 1. Toggle between "All exhaustive rules" and "Explicitly set rules" views - Verify "All exhaustive rules" shows inherited/fallback rules - Verify "Explicitly set rules" shows only rules you've specifically configured 2. Test the patron category filter: - Select a specific patron category - Confirm only rules for that category are displayed 3. Test the item type filter: - Select a specific item type - Confirm only rules for that item type are displayed 4. Test combined filters (patron category + item type simultaneously) 5. Clear filters and verify all rules display again Part 3: Adding New Triggers 1. Click "Add trigger" button 2. Step 1 - Confirm Context: - Select Library (required) - Select Patron category (required) - Select Item type (required) - Click "Confirm context" 3. Step 2 - Review Existing Triggers: - Verify existing triggers for this context are displayed - Note the trigger numbers and delays 4. Step 3 - Configure New Trigger: - Enter a delay value (should be constrained by neighboring triggers) - Try entering an invalid delay (too low or too high) - verify validation - Set "Restricts checkouts" (Yes/No/Fallback to default) - Note the placeholder showing the fallback value - Select a letter template - Verify only templates for the selected library context appear - Set "Debarring" options if desired - Select "Charged notice fees" if applicable 5. Submit the form 6. Verify the new trigger appears in the list without a full page reload 7. Verify success message is displayed Part 4: Editing Existing Triggers 1. Locate an existing trigger in the list 2. Click the edit icon/button 3. Verify the form pre-populates with existing values 4. Test delay modification: - Try changing the delay within valid bounds - should succeed - Try changing delay beyond neighboring triggers - should show validation error - Use the increment/decrement buttons to adjust delay 5. Test clearing values: - Clear a field (e.g., delay or restrict checkouts) - Verify placeholder shows the fallback/default value - Submit with cleared value - should save as null and inherit fallback 6. Change the letter template to a different valid template 7. Submit changes 8. Verify the trigger updates in the list without full page reload 9. Verify the changes persist after refreshing the page Part 5: Deleting Triggers 1. Identify a trigger that is the last in its sequence 2. Click the delete button/icon 3. Verify a confirmation dialog appears 4. Confirm deletion 5. Verify the trigger is removed from the list without page reload 6. Verify the deletion persists after page refresh 7. Test delete constraints: - Try to delete a trigger that's not the last in sequence - Verify appropriate error/warning message Part 6: Resetting Rule Sets 1. Locate a context with explicitly set rules (not just inherited) 2. Ensure this is not the only rule set for this trigger sequence 3. Click the "Reset" button for that context 4. Verify confirmation dialog explains what will be reset 5. Confirm the reset 6. Verify the explicit rules are removed and context now shows inherited rules 7. Verify changes persist after page refresh 8. Test reset constraints: - Try to reset when it would leave no rules for that trigger - Verify appropriate warning/prevention Part 7: Context-Specific Behavior 1. Test library-specific letter filtering: - Create/edit a trigger for a specific library - Verify only letter templates with matching branchcode or default templates appear - Switch to a different library context - Verify the letter list updates accordingly 2. Test default rules: - View rules for "All libraries" / "All patron categories" / "All item types" - Verify these are labeled as "Default rule for [context]" - Confirm these serve as fallback for more specific rules 3. Test DefaultToLoggedInLibraryOverdueTriggers preference: - Set preference to "Yes" - Reload circulation triggers page - Verify it defaults to your logged-in library - Set preference to "No" - Verify it defaults to "All libraries" Part 8: Form Workflow & Validation 1. Test required fields: - Try submitting add/edit form without selecting library - should fail - Try submitting without patron category - should fail - Try submitting without item type - should fail 2. Test stepper navigation: - Start adding a trigger - Verify you must confirm context before proceeding - Verify you see existing triggers before editing/adding - Verify trigger form only appears after context confirmation 3. Test cancellation: - Start adding/editing a trigger - Click Cancel - Verify you return to the list without changes - Verify no partial data was saved Part 9: Loading States & Messages 1. On slow connections or with many rules: - Verify "Loading context..." appears while fetching context data - Verify "Loading rule set information..." appears while fetching rules - Verify section-specific loading messages appear during updates 2. After form submission: - Verify appropriate success/error messages display - Verify loading indicators during async operations Part 10: Permissions 1. Log in as a user without tools_edit_notice_status_triggers permission 2. Verify the "Circulation triggers" link does not appear in Administration 3. Try accessing /cgi-bin/koha/admin/circulation_triggers directly 4. Verify access is denied Part 11: Data Integrity & Persistence 1. Create a complex set of rules with multiple triggers across different contexts 2. Refresh the page and verify all rules persist 3. Restart the Koha services 4. Verify all rules still display correctly 5. Check the database circulation_rules table: - Verify rules are stored with correct rule_name pattern (overdue_1_delay, etc.) - Verify null values are properly stored for inheritance - Verify the overdue_X_has_rules rule kind is present Part 12: Edge Cases 1. Test with no existing triggers - verify empty state message 2. Test with maximum trigger sequences (e.g., trigger 1, 2, 3) 3. Test rapid successive edits 4. Test with special characters in letter template names 5. Test browser back/forward buttons during form workflow Part 13: Comparison with Legacy Interface 1. Access the old interface if still available (via Tools menu) 2. Create a trigger using the new interface 3. Verify it appears correctly in the old interface (if applicable) 4. Create a trigger using the old interface (if still available) 5. Verify it appears correctly in the new interface
Created attachment 194054 [details] [review] Bug 10190: Update has_restricting_overdues to refer to circulation rules This patch updates the Koha::Patron->has_restriction_overdues method to refer to the migrated overdue rules. We also correct an oversight in the method prior to this patch that meant that not all overdues were being correctly checked for a restriction.. we were incorrectly filtering to only overdues at the current checkin branch. Finally, we also take account of the itemtype level addition here too. NOTE: There weren't any unit tests for this Koha::Patron method.. we should add some here! Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194055 [details] [review] Bug 10190: Migrate MarkIssueReturned test to circulation rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194056 [details] [review] Bug 10190: Remove old overduerules interface This patch updates the link from the tools homepage to open the new circulation triggers administration page. We then remove the old overduerules controller and template, and finally we drop the no longer referenced C4::Overdues::GetOverdueMessageTransportTypes method and associated unit tests. Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194057 [details] [review] Bug 10190: Update itiva script to reflect move to circ_rules Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk> Signed-off-by: George Harkins <George.Harkins@cityofglasgowcollege.ac.uk>
Created attachment 194058 [details] [review] Bug 10190: Comprehensive Cypress tests for Circulation Triggers UI This patch significantly enhances the Cypress test coverage for the new Circulation Triggers administration interface, expanding from basic breadcrumb checks to comprehensive end-to-end testing. Test Coverage: - Initial page load and API error handling - Context filtering (library, patron category, item type) - View toggling between explicit and all applicable rules - Tab navigation between multiple triggers - Full CRUD operations (Create, Read, Update, Delete) - Rule set reset functionality - Letter template filtering by library context - Form validation and constraint checking - Loading states and empty states - Error handling for all operations - Browser navigation (back/forward buttons) - Complex scenarios with inheritance hierarchies New Features: - Mock data helper functions for test fixtures - Comprehensive API interception for all endpoints - Tests for both success and error paths - Placeholder value testing for fallback rules - Increment/decrement button testing - Multi-step form workflow validation - Confirmation dialog testing for destructive operations The tests follow Koha's Cypress testing patterns established in the ERM module, using proper API mocking, clear test descriptions, and organized test suites.
Created attachment 194059 [details] [review] Bug 10190: (QA follow-up): Fix removal of tables Patch from commit 41a7bb8
Created attachment 194060 [details] [review] Bug 10190: (follow-up) Complete Vue.js rewrite of Circulation Triggers administration interface This patch aims to resolve the following issues as reported by comment 193: - missing atomic update for overde_X_has_rules (amended to account for comment 227) - existing delays being overwritten by the client when a rule set is displayed - inability to save circulation overdue rule sets for context for which circulations rules where already set - address bug_10190.pl db script warnings (undef values were being concatenated with strings) - add a guard clause to bug_101090.pl: check that the tables the script pulls the data from do still exist, otherwise exit. - remove duplicate Letters options in the circulation triggers add / edit form
Created attachment 194061 [details] [review] Bug 10190: fix permissions - add a config API endpoint to Circulations Triggers so permissions can be fetched - update the permissions in the Vue App so that manage_circ_rules is required - update the permissions in the Circulation Rules API so that manage_circ_rules is required for PUT operations - update the circulation-rules store to so that it is compatible with recently intorduced compostables - implement a redirect into the route so that permission requirements are honoured
Created attachment 194062 [details] [review] Bug 10190: remove circ triggers link from tools menu Patch from commit 02262e0
Created attachment 194063 [details] [review] Bug 10190: refactor to remove redundant try..catch Patch from commit ac42b62
Created attachment 194064 [details] [review] Bug 10190: (QA follow-up) fix unix permissions on bug_10190.pl Patch from commit 2618fd6
Created attachment 194065 [details] [review] Bug 10190: (QA follow-up) fix unix permissions on circulation_triggers.pl Patch from commit b9a2864
Created attachment 194066 [details] [review] Bug 10190: (QA follow-up) add new line at end of MessageTransportType.pm Patch from commit 29a428a
Created attachment 194067 [details] [review] Bug 10190: (QA follow-up) amend copyright statement Patch from commit bf6b82a
Created attachment 194068 [details] [review] Bug 10190: (QA follow-up) typo Patch from commit 8bca91f
Created attachment 194069 [details] [review] Bug 10190: enhance trigger context display in modal - improve modal scroll behaviour - improve modal dimensions - display contexts as a horizontal list to make better use of the available space - fully contain dropdowns within the modal body
Created attachment 194070 [details] [review] Bug 10190: (QA follow-up) add missing filter Patch from commit c1cde49
Created attachment 194071 [details] [review] Bug 10190: trigger add/edit form UI/UX enhancements - scroll to the add/edit form on button click - prevent redundant full page reload upon the user swapping between adding and editing while the modal is open - ensure robust scroll behaviour, handling: - the URL being accessed directly - the edit button in the table in the main page being used to access the form directly - the stepper workflow being used - the user swapping between add and edit once already looking at the form - dynamic display of 'Transport Type' consistency: only show if a 'Notice' has been selected - if a notice has been selected, prevent form submission if transport type isn't set and display an alert on submission attempt
Created attachment 194072 [details] [review] Bug 10190: (QA follow-up) swap undef for all in update db output Patch from commit 877458a
Created attachment 194073 [details] [review] Bug 10190: prevent duplicate ruleset load - prevent the initial API call and subsequent exhaustive applied ruleset list generation logic from running twice
Created attachment 194074 [details] [review] Bug 10190: Deprecate permission user_permissions rows do not need manual cleanup because the FK on user_permissions(module_bit, code) references permissions(module_bit, code) ON DELETE CASCADE, so deleting from permissions automatically removes per-patron rows.
Created attachment 194075 [details] [review] Bug 10190: rewrite atomicupdate with correct 1:1 rule migration The previous migration had two fundamental errors: 1. It manufactured phantom empty rows for all patron categories that had no rule, which corrupted the "most frequent" calculation. 2. It created categorycode=NULL (all-categories) default rules for every branchcode seen, silently extending overdue notice coverage to patron categories that had no configured rule in the old system. The backend (overdue_notices.pl) does not consult has_rules, so these invented defaults would cause notices to be sent to patrons that were deliberately uncovered. The rewrite migrates only explicit (branchcode, categorycode) pairs that had a positive delay configured. No default rules are created. branchcode='' maps to NULL as before. exit is replaced with return to avoid killing subsequent atomicupdate scripts.
Created attachment 194076 [details] [review] Bug 10190: fix three regressions in overdue_notices.pl - Store patron name/address fields in \$borrower_overdues so prepare_letter_for_printing has data for printed/CSV/text notices - Fix email->print fallback: look up items under original MTT key, not the effective (fallback) key - Set \$admin_email_address from \$library unconditionally in _enact_trigger; previously undef in non-patron_homelibrary mode, stripping the from_address on all queued notices
Created attachment 194077 [details] [review] Bug 10190: fix four more issues spotted in the overdues script - Patron.pm has_restricting_overdues: sort ASC not DESC so the oldest (most overdue) item is checked per branch/itemtype; DESC caused the deduplication skip to discard the very items most likely to trigger a restriction - overdue_notices.pl: fix --itemtypesout option name in GetOptions (was 'itemtypeouts', never matched the documented --itemtypesout flag) - overdue_notices.pl: fix typo 'catagorycode' -> 'categorycode' in --borcatout search, which silently broke category exclusion - circulation_triggers.pl: add flagsrequired => parameters/manage_circ_rules so the admin page enforces the correct permission; also add missing use Koha::Notice::Templates
Created attachment 194078 [details] [review] Bug 10190: correction to x-koha-authorization on /circulation_rules/config The config endpoint is used exclusively by the Circulation Triggers Vue app, which is gated on parameters/manage_circ_rules. Using acquisition/'*' and erm/1 was inconsistent copy-paste from an unrelated endpoint and would 403 circ-rules admins while granting access to unrelated module users.
Created attachment 194079 [details] [review] Bug 10190: three minor issues - overdue_notices.pl: fix verbose sprintf - remove spurious \$triggered argument (CLI 0/1 flag had replaced delay value in output) and use the \$borr string that was computed but never included in the message - overdue_notices.pl: remove %already_queued hash which was populated in _enact_trigger but never read; dead code since the refactor - C4/Overdues.pm GetBranchcodesWithOverdueRules: guard the global-rule expansion with @branchcodes check so an empty result (no rules at all) correctly returns an empty list instead of all branches
Created attachment 194080 [details] [review] Bug 10190: remove has_rules sentinel from circulation_rules table has_rules was a purely frontend marker stored as a circulation rule to signal that a context had explicitly-saved overdue trigger rules. The backend never reads it; the information is fully derivable from whether any of delay/notice/mtt/restrict are non-null for a given context. Replace all has_rules DB reads/writes with a hasExplicitRulesForTrigger() helper that checks the four real rule suffixes directly. has_rules remains as a computed in-memory concept in the Vue store (derived in findEffectiveRule) so templates require no changes. Also adds an atomicupdate to delete existing has_rules rows from the DB, and removes the has_rules insert from the bug_10190 migration script.
Created attachment 194081 [details] [review] Bug 10190: fix NaN trigger number when adding first rule for a library When no circulation rules exist for a library, triggerCounts[library_id] is undefined, causing undefined + 1 = NaN. Use nullish coalescing to default to 0 so the first trigger correctly gets number 1.
Created attachment 194082 [details] [review] Bug 10190: warn when no default triggers exist and show libraries with rules When viewing the default library context with no triggers defined, show a warning message explaining defaults apply to all libraries. If any libraries have specific triggers, list them as clickable links that switch the filter to that library. If no rules exist anywhere, show a prompt to add the first trigger.
Created attachment 194083 [details] [review] Bug 10190: Fix test count in overdues.t Patch from commit 9e5163c
Created attachment 194084 [details] [review] Bug 10190: fix failing Permissions.t and overhaul CirculationTriggers Cypress spec Permissions.t: - Remove edit_notice_status_triggers from userpermissions.sql so fresh installs no longer seed this deprecated permission (existing installs are handled by the atomic update) - Add a defensive DELETE in the test to cover DBs that pre-date the atomic update CirculationTriggers_spec.ts: - Fix getMockCirculationRule: the helper was returning the old {rule_name, rule_value, branchcode, ...} format; update to the current API format {context: {...}, overdue_N_delay: ..., ...} - Remove assertions for UI elements that do not exist (success/error flash messages, "Existing triggers" heading, "rule set will be removed" text) - Fix reset confirmation text to match actual component output - Fix duplicate patron_category_id "ST" in mock patron categories - Remove aspirational tests that checked non-implemented behaviour - Add describe block for the no-default-triggers warning (no rules anywhere, rules only at specific libraries, library link switches the filter) - Add regression test for NaN trigger number when adding first rule for a library
Created attachment 194085 [details] [review] Bug 10190: Patron categories fetch shouldn't require full manage permission Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk>
Created attachment 194086 [details] [review] Bug 10190: Fix permission mismatch on /circulation_rules/config endpoint The config endpoint was gated on parameters: manage_circ_rules, while the actual GET/PUT /circulation_rules endpoints require circulate: circulate_remaining_permissions. Any user who can read/write circulation rules should also be able to load the config that drives the UI.
Created attachment 194087 [details] [review] Bug 10190: (QA follow-up) Fix six issues reported during QA 1. Letter context: setFilteredLetters() was reading library_id from currentRuleSet (which has no top-level library_id property) instead of this.context.library_id, causing library-specific letter templates to never match. 2. False conflict on edit: conflict check fetched rule set from DB with effective=false (raw), but the in-memory snapshot was fetched with effective=true (with fallbacks). Comparing raw vs effective always triggered a conflict when a context inherited any value from a more general rule. Fixed by using effective=true for the DB fetch too. 3. Incomplete deletion: updateCircRuleSets() inside deleteRuleSet() was called without await, creating a race condition where the page refresh could complete before the deletion API call, leaving stale data visible. Also added a guard to skip rule sets with no explicit rules for the trigger being deleted, and simplified rulesForDeletion construction. 4. No rules displayed on initial load: the route watcher in CirculationTriggersList fired immediately before init() had loaded patron categories and item types, causing setAllExhaustiveEffectiveRuleSets() to produce an empty result. Fixed with a metaInitialized flag that the watcher waits for before proceeding. 5. DefaultToLoggedInLibraryOverdueTriggers syspref not respected: the default_view template variable was computed but never applied to the store's currentLibraryId. Fixed by passing it to init(). 6. UI colors: info box background changed from #84c1f3 to #DAECFB (lighter, matching the circulation rules page). Error alert messages changed from alert-info (blue) to alert-warning (yellow) to match Koha conventions.
Created attachment 194088 [details] [review] Bug 10190: Enforce manage_circ_rules_from_any_libraries permission - Add OR-type x-koha-authorization to /circulation_rules, /circulation_rules/kinds, and /circulation_rules/config endpoints so users with either circulate:circulate_remaining_permissions OR parameters:manage_circ_rules can access them - Enforce manage_circ_rules_from_any_libraries in the set_rules controller: users without this permission may only set rules for their own library and cannot set default (*) rules - Add explicit use C4::Auth qw(haspermission) import to CirculationRules.pm - Lock the library selector in the Vue UI for users who lack manage_circ_rules_from_any_libraries, defaulting them to their own library - Update set_rules() tests to cover the new permission restrictions
New living test plan: https://docs.google.com/document/d/1NnVo8T1CK5456ec0WvMu2sHiQVL3nSHJI95t5yjb1I4/edit?usp=sharing
Created attachment 194090 [details] [review] Bug 10190: Center tables in add modal Patch from commit 841e3fc
Created attachment 194094 [details] [review] Bug 10190: Fix patron_categories permission format The x-koha-authorization was using 'parameters: catalogue' which the Koha auth system interprets as a 'catalogue' subpermission within the 'parameters' module (which doesn't exist), causing a 403 for any user. The correct format for the top-level 'catalogue' permission is 'catalogue: "1"', consistent with all other read-only API endpoints.
We are making good progress; however, I unfortunately need to set the status back to Failed QA. The most significant current issue is that triggers cannot be edited if all/all/all rules are configured. This was Number 3 in Comment 316: 3.) Editing Existing Triggers is not always possible editing the delay is only possible for all / all rules. have several rules for trigger 1 e.g. a all/all/all rule plus a all/Kids/Books with a different delay value (but same letter code as all/all/all rule) Now try to change the delay value of the all/Kids/Books rule. As soon as you press the submit button, you receive the following error message: "Your changes could not be saved as this circulation trigger was updated elsewhere. Please see the updated trigger below." --- The good news is : Permission error is solved ! :-) Letter context is solved Incomplete trigger deletion is solved DefaultToLoggedInLibraryOverdueTriggers is solved UI colors consistent now First run for cronjob overdue_notices.pl was successful. TBC next week