|
Description
Pedro Amorim
2024-09-11 10:58:18 UTC
Created attachment 171300 [details] [review] Bug 37891: Submit SMS related data even if SMSSendDriver is empty. The bug is caused by the SMS data not being submitted as its fieldset is disabled if SMSSendDriver is empty. This causes the SMS instance of the notice to not be updated when the edit happens, becoming out of sync. Once it becomes out of sync with the other notices of same code but different transport_type, the 'GROUP BY branchcode,module,code,name,branchname' in letter.pl will fail, as the name no longer matches, listing the same notice twice on the notices table. This patch updates the logic on the template to have the SMS input elements be disabled/readonly instead of having disabled='disabled' on the SMS fieldset. This keeps the same functionality i.e. staff member is unable to edit SMS notice if SMSSendDriver is disabled, but ensures that SMS related data is submitted to the controller when the notice edit save happens. Test plan: 1) Visit notices and slips: <staff_url>/cgi-bin/koha/tools/letter.pl 2) Search for a notice code e.g. 'ILL_REQUEST_UPDATE'. 3) Notice only one result is returned. Click 'Edit'. 4) Edit the 'Name'. Click 'Save'. 5) Repeat 2. Notice two results are now returned. 6) Apply patch. Repeat test plan. Notice only one result is returned at the end. *** Bug 35029 has been marked as a duplicate of this bug. *** Just finished the test plan for bug 37891. I can confirm that the bug doesn’t show up in the UI. However, the discussion in bug 35029 suggests there’s the same issue when updating the database manually. (In reply to Sukhmandeep from comment #3) > Just finished the test plan for bug 37891. I can confirm that the bug > doesn’t show up in the UI. However, the discussion in bug 35029 suggests > there’s the same issue when updating the database manually. Yes, this bug 37891 prevents the bug from happening off of regular UI usage e.g. a staff member editing a template name. The underlying problem (reproduceable by manually updating the database through SQL queries) in relation to the database table design and respective GROUP BY logic in letter.pl should be discussed/handled in bug 35029. Created attachment 172037 [details] [review] Bug 37891: Submit SMS related data even if SMSSendDriver is empty. The bug is caused by the SMS data not being submitted as its fieldset is disabled if SMSSendDriver is empty. This causes the SMS instance of the notice to not be updated when the edit happens, becoming out of sync. Once it becomes out of sync with the other notices of same code but different transport_type, the 'GROUP BY branchcode,module,code,name,branchname' in letter.pl will fail, as the name no longer matches, listing the same notice twice on the notices table. This patch updates the logic on the template to have the SMS input elements be disabled/readonly instead of having disabled='disabled' on the SMS fieldset. This keeps the same functionality i.e. staff member is unable to edit SMS notice if SMSSendDriver is disabled, but ensures that SMS related data is submitted to the controller when the notice edit save happens. Test plan: 1) Visit notices and slips: <staff_url>/cgi-bin/koha/tools/letter.pl 2) Search for a notice code e.g. 'ILL_REQUEST_UPDATE'. 3) Notice only one result is returned. Click 'Edit'. 4) Edit the 'Name'. Click 'Save'. 5) Repeat 2. Notice two results are now returned. 6) Apply patch. Repeat test plan. Notice only one result is returned at the end. Signed-off-by: Jason Robb <jrobb@sekls.org> Putting this here even though it's not been PQAd as to not forget: Adding 23.11 and 24.05 release candidate keywords as we need this for 23.11 specifically. But can/should possibly be backported further down. Let me know if help is required rebasing. FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt
FAIL tt_valid
lines 643, 645, 650, 668, 675, 678
TT directives are not allowed inside html tags. Created attachment 172222 [details] [review] Bug 37891: (QA follow-up): Fix tt_valid QA test 1) Apply both patches 2) $ qa -c 2 -v 2 Created attachment 172224 [details] [review] Bug 37891: Submit SMS related data even if SMSSendDriver is empty. The bug is caused by the SMS data not being submitted as its fieldset is disabled if SMSSendDriver is empty. This causes the SMS instance of the notice to not be updated when the edit happens, becoming out of sync. Once it becomes out of sync with the other notices of same code but different transport_type, the 'GROUP BY branchcode,module,code,name,branchname' in letter.pl will fail, as the name no longer matches, listing the same notice twice on the notices table. This patch updates the logic on the template to have the SMS input elements be disabled/readonly instead of having disabled='disabled' on the SMS fieldset. This keeps the same functionality i.e. staff member is unable to edit SMS notice if SMSSendDriver is disabled, but ensures that SMS related data is submitted to the controller when the notice edit save happens. Test plan: 1) Visit notices and slips: <staff_url>/cgi-bin/koha/tools/letter.pl 2) Search for a notice code e.g. 'ILL_REQUEST_UPDATE'. 3) Notice only one result is returned. Click 'Edit'. 4) Edit the 'Name'. Click 'Save'. 5) Repeat 2. Notice two results are now returned. 6) Apply patch. Repeat test plan. Notice only one result is returned at the end. Signed-off-by: Jason Robb <jrobb@sekls.org> Amended-by: Jonathan Druart Align Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 172225 [details] [review] Bug 37891: (QA follow-up): Fix tt_valid QA test 1) Apply both patches 2) $ qa -c 2 -v 2 Amended-by: Jonathan Druart Add the different attribute at the end of the node so that we can easily see what's different between the two lines Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> (In reply to Jonathan Druart from comment #11) > Add the different attribute at the end of the node so that we can easily > see what's different between the two lines Fair. Thanks! Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant doesnt apply cleanly to 24.05.x, please rebase if needed Created attachment 172812 [details] [review] [24.05]Bug 37891: Submit SMS related data even if SMSSendDriver is empty. The bug is caused by the SMS data not being submitted as its fieldset is disabled if SMSSendDriver is empty. This causes the SMS instance of the notice to not be updated when the edit happens, becoming out of sync. Once it becomes out of sync with the other notices of same code but different transport_type, the 'GROUP BY branchcode,module,code,name,branchname' in letter.pl will fail, as the name no longer matches, listing the same notice twice on the notices table. This patch updates the logic on the template to have the SMS input elements be disabled/readonly instead of having disabled='disabled' on the SMS fieldset. This keeps the same functionality i.e. staff member is unable to edit SMS notice if SMSSendDriver is disabled, but ensures that SMS related data is submitted to the controller when the notice edit save happens. Test plan: 1) Visit notices and slips: <staff_url>/cgi-bin/koha/tools/letter.pl 2) Search for a notice code e.g. 'ILL_REQUEST_UPDATE'. 3) Notice only one result is returned. Click 'Edit'. 4) Edit the 'Name'. Click 'Save'. 5) Repeat 2. Notice two results are now returned. 6) Apply patch. Repeat test plan. Notice only one result is returned at the end. Signed-off-by: Jason Robb <jrobb@sekls.org> Amended-by: Jonathan Druart Align Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 172813 [details] [review] [24.05]Bug 37891: (QA follow-up): Fix tt_valid QA test 1) Apply both patches 2) $ qa -c 2 -v 2 Amended-by: Jonathan Druart Add the different attribute at the end of the node so that we can easily see what's different between the two lines Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> (In reply to Lucas Gass (lukeg) from comment #14) > doesnt apply cleanly to 24.05.x, please rebase if needed Thanks Lucas. I've rebased for 24.05.x and re-tested. I'm now looking into 23.11.x. That rebase is not as trivial because bug 35882 is missing in 23.11.x Created attachment 172818 [details] [review] [23.11]Bug 37891: Submit SMS related data even if SMSSendDriver is empty This is a squash of both patches for 23.11.x. The bug is caused by the SMS data not being submitted as its fieldset is disabled if SMSSendDriver is empty. This causes the SMS instance of the notice to not be updated when the edit happens, becoming out of sync. Once it becomes out of sync with the other notices of same code but different transport_type, the 'GROUP BY branchcode,module,code,name,branchname' in letter.pl will fail, as the name no longer matches, listing the same notice twice on the notices table. This patch updates the logic on the template to have the SMS input elements be disabled/readonly instead of having disabled='disabled' on the SMS fieldset. This keeps the same functionality i.e. staff member is unable to edit SMS notice if SMSSendDriver is disabled, but ensures that SMS related data is submitted to the controller when the notice edit save happens. Test plan: 1) Visit notices and slips: <staff_url>/cgi-bin/koha/tools/letter.pl 2) Search for a notice code e.g. 'ILL_REQUEST_UPDATE'. 3) Notice only one result is returned. Click 'Edit'. 4) Edit the 'Name'. Click 'Save'. 5) Repeat 2. Notice two results are now returned. 6) Apply patch. Repeat test plan. Notice only one result is returned at the end. Signed-off-by: Jason Robb <jrobb@sekls.org> Amended-by: Jonathan Druart Align Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> [24.05]Bug 37891: (QA follow-up): Fix tt_valid QA test 1) Apply both patches 2) $ qa -c 2 -v 2 Amended-by: Jonathan Druart Add the different attribute at the end of the node so that we can easily see what's different between the two lines Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Rebased and re-tested for 23.11.x. Pedro, I appreciate all of your hard work with the different re-bases. Thanks! Backported to 24.05.x for upcoming 24.05.05 Pushed to 23.11.x for 23.11.11 Thanks a lot for rebased patch ;) |