Bug 16268 - Add confirm message when deleting circ and fines rules
Summary: Add confirm message when deleting circ and fines rules
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on: 16267
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-14 22:44 UTC by Aleisha Amohia
Modified: 2016-12-05 21:27 UTC (History)
5 users (show)

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


Attachments
Bug 16268: Add confirm message for deleting circ and fines rules (4.49 KB, patch)
2016-04-14 23:00 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 16268: Add confirm message for deleting circ and fines rules (4.84 KB, patch)
2016-04-17 22:36 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 16268: Add confirm message for deleting circ and fines rules (4.99 KB, patch)
2016-04-21 12:39 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 16268: Add confirm message for deleting circ and fines rules (5.05 KB, patch)
2016-04-24 21:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16268: Add confirm message for deleting circ and fines rules (4.66 KB, patch)
2016-04-25 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 16268: Add confirm message for deleting circ and fines rules (4.79 KB, patch)
2016-04-26 04:23 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16268: Add confirm message for deleting circ and fines rules (4.86 KB, patch)
2016-04-26 16:11 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 Aleisha Amohia 2016-04-14 22:44:14 UTC

    
Comment 1 Aleisha Amohia 2016-04-14 23:00:58 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2016-04-15 13:32:49 UTC
This patch violates the newly approved and newly added to the wiki (as of yesterday) coding guideline JS9: "Avoid the use of event attributes like "onclick" to attach events."

https://wiki.koha-community.org/wiki/Coding_Guidelines#JS9:_Avoid_the_use_of_event_attributes_like_.22onclick.22_to_attach_events

There is a simple function in staff-global.js which seems to be seldom used, but it could be in this case:

function confirmDelete(message) {
    return (confirm(message) ? true : false);
}

---

$(document).ready(function(){
    $(".delete_rule").on("click",function(){
        return confirmDelete(MSG_CONFIRM_DELETE);
    });
});

I have started the process of tracking down and cleaning up use of "onclick" (See Bug 16262) but there are many templates to fix.
Comment 3 Aleisha Amohia 2016-04-17 22:09:17 UTC
(In reply to Owen Leonard from comment #2)
> This patch violates the newly approved and newly added to the wiki (as of
> yesterday) coding guideline JS9: "Avoid the use of event attributes like
> "onclick" to attach events."
> 
> https://wiki.koha-community.org/wiki/Coding_Guidelines#JS9:
> _Avoid_the_use_of_event_attributes_like_.22onclick.22_to_attach_events
> 
> There is a simple function in staff-global.js which seems to be seldom used,
> but it could be in this case:
> 
> function confirmDelete(message) {
>     return (confirm(message) ? true : false);
> }
> 
> ---
> 
> $(document).ready(function(){
>     $(".delete_rule").on("click",function(){
>         return confirmDelete(MSG_CONFIRM_DELETE);
>     });
> });
> 
> I have started the process of tracking down and cleaning up use of "onclick"
> (See Bug 16262) but there are many templates to fix.

Ah thanks for bringing this to my attention. I recently had a few patches pass QA where I was using the onclick attribute for confirm messages.
Comment 4 Aleisha Amohia 2016-04-17 22:36:08 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2016-04-21 12:39:09 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-04-23 06:59:51 UTC
I don't think the confirmUnsetDel function is useful.
We do not need a refactoring such trivial code.
Comment 7 Katrin Fischer 2016-04-24 21:57:20 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2016-04-24 22:03:03 UTC
Sorry, I had missed Jonathan's comment - Aleisha, could you take a look?
Comment 9 Aleisha Amohia 2016-04-25 21:40:20 UTC Comment hidden (obsolete)
Comment 10 Marc Véron 2016-04-26 04:23:57 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-04-26 16:11:58 UTC
Created attachment 50727 [details] [review]
Bug 16268: Add confirm message for deleting circ and fines rules

EDIT: Removes unnecessary code and uses confirm message in staff-global.js

To test:
1) Apply 16267
2) Go to Admin -> Circ and fines rules
3) Try clicking on Delete and Unset buttons
4) Confirm a confirm message pops up and works as expected

Sponsored-by: Catalyst IT

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Kyle M Hall 2016-04-29 10:57:32 UTC
Pushed to master! Will be in the May 2016 release! Kiitos Aleisha!