Bug 37891 - Editing a notice's name having SMSSendDriver disabled causes notice to be listed twice
Summary: Editing a notice's name having SMSSendDriver disabled causes notice to be lis...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Pedro Amorim
QA Contact: Jonathan Druart
URL:
Keywords: rel_23_11_candidate, rel_24_05_candidate
Depends on:
Blocks:
 
Reported: 2024-09-11 10:58 UTC by Pedro Amorim
Modified: 2024-10-09 20:43 UTC (History)
10 users (show)

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


Attachments
Bug 37891: Submit SMS related data even if SMSSendDriver is empty. (8.82 KB, patch)
2024-09-11 11:08 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37891: Submit SMS related data even if SMSSendDriver is empty. (8.87 KB, patch)
2024-09-26 19:21 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 37891: (QA follow-up): Fix tt_valid QA test (7.58 KB, patch)
2024-09-30 13:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37891: Submit SMS related data even if SMSSendDriver is empty. (9.04 KB, patch)
2024-09-30 14:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37891: (QA follow-up): Fix tt_valid QA test (7.80 KB, patch)
2024-09-30 14:27 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-09-11 10:58:18 UTC
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.
Comment 1 Pedro Amorim 2024-09-11 11:08:58 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.
Comment 2 Pedro Amorim 2024-09-11 11:11:34 UTC
*** Bug 35029 has been marked as a duplicate of this bug. ***
Comment 3 Sukhmandeep 2024-09-12 12:43:31 UTC
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.
Comment 4 Pedro Amorim 2024-09-12 13:07:49 UTC
(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.
Comment 5 ByWater Sandboxes 2024-09-26 19:21:02 UTC
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>
Comment 6 Pedro Amorim 2024-09-27 10:07:49 UTC
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.
Comment 7 Jonathan Druart 2024-09-30 13:14:43 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt
   FAIL   tt_valid
                lines 643, 645, 650, 668, 675, 678
Comment 8 Jonathan Druart 2024-09-30 13:15:40 UTC
TT directives are not allowed inside html tags.
Comment 9 Pedro Amorim 2024-09-30 13:51:00 UTC
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
Comment 10 Jonathan Druart 2024-09-30 14:27:25 UTC
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>
Comment 11 Jonathan Druart 2024-09-30 14:27:28 UTC
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>
Comment 12 Pedro Amorim 2024-09-30 15:51:33 UTC
(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!
Comment 13 Martin Renvoize (ashimema) 2024-10-02 09:56:32 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 14 Lucas Gass (lukeg) 2024-10-09 20:43:20 UTC
doesnt apply cleanly to 24.05.x, please rebase if needed