Bug 19029 - Implement a security question for cloning circulation conditions
Summary: Implement a security question for cloning circulation conditions
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Buckley
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
Depends on: 19027
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-03 06:10 UTC by Alex Buckley
Modified: 2017-10-27 17:15 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19029 - Added JavaScript security question for cloning circ rules (4.40 KB, patch)
2017-08-27 23:35 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 19029 - Added JavaScript security question for cloning circ rules (4.46 KB, patch)
2017-09-22 18:20 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19029 - Followup - Implemented .format() to ease translation (1.37 KB, patch)
2017-10-02 19:34 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 19029 - Followup: Make security question appear on clone operations from one library to another (1.96 KB, patch)
2017-10-11 22:48 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 19029 - Followup: Make security question appear on clone operations from one library to another (2.14 KB, patch)
2017-10-11 23:56 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 19029 - Added JavaScript security question for cloning circ rules (4.46 KB, patch)
2017-10-27 12:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19029 - Followup - Implemented .format() to ease translation (1.37 KB, patch)
2017-10-27 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19029 - Followup: Make security question appear on clone operations from one library to another (2.14 KB, patch)
2017-10-27 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-08-03 06:10:15 UTC
When users create a circulation rule which they want to be applied to multiple library branches they will click "Standard rules to all libraries" (from bug 19027)

However they may accidentally hit the 'clone' button, to clone the rule to a specific library.

When the rule is being applied to all libraries with the 'standard' option then the only use case for clicking the 'clone' button would be to then adapt the rule in that specific library.

This is not a common use case and users may accidentally hit the 'clone' button without understanding it fully.

Therefore this bug report suggests that a security question be displayed to be ask the user if they are sure about cloning the rule to a specific library. Additionally the clone button should be moved to a slightly different placement on the page.

Can we please have some feedback on this idea
Comment 1 Marc Véron 2017-08-03 08:15:45 UTC
+1   :-)
Comment 2 Katrin Fischer 2017-08-11 09:31:25 UTC
+1
Comment 3 Katrin Fischer 2017-08-11 09:32:20 UTC
We usually have 'do you really want to...' questions for deleting data. As cloning overwrites the existing rules completely and currently without warning, I think the alert would fit in our patterns and be really helpful.
Comment 4 Alex Buckley 2017-08-27 23:35:08 UTC
Created attachment 66519 [details] [review]
Bug 19029 - Added JavaScript security question for cloning circ rules

This patch introduces a Javascript security question which is displayed
to the user when they try to clone a circulation rule to a specific
branch when the rule is a 'Standard rule for all libraries"

The rationale for this patch is when the cloning takes place it
overwrites the existing rules of the destination branch and there is no
notification of this to the user. Therefore by implementing this patch
the user is asked if they want to clone the rule (if the rule is
standard accross all libraries) and are told that it
will overwrite the rules in the destination branch.

Test plan:
1. Create a circulation rule for all libraries

2. Make sure the 'select a library' option is set to 'Standard rules for all
libraries"

3. Click the 'Clone' button and notice that the cloning takes place
without any warning that it will overwrite the rules of the destination
branch

4. Apply patch

5. Return to the circulation and fine rules page

6. Repeat step 2

7. Click the clone button and notice a alert box appears asking if you
are sure you want to clone the standard rule to the destination branch.

Note: The name of the destination branch is included in the alert.
Also note that the user is informed of the consequences of performing
the action, i.e. that it will overwrite the existing rules in the
destination branch

8. Click 'Cancel' and notice that no cloning occurs

9. Click the clone button again and this time click 'OK' and notice
that the cloning takes place

10. Return to the Circulation and fine rules page and set the 'Select a
library' option to the name of an individual branch

11. Click the clone button and notice that the clone action takes place

Sponsored-By: Catalyst IT
Comment 5 Alex Buckley 2017-08-28 02:38:28 UTC
In the patch I have attach I implemented a security question but have not moved the clone button, please let me know if you think I need to move the clone button as well, because I personally think the security question is a great improvement on the previous behaviour and the moving of the clone button is not needed.
Comment 6 Marc Véron 2017-08-28 05:29:21 UTC
While testing I get:
'Cloning of circulation and fine rules failed!'
Plack error log says:
DBD::mysql::st execute failed: Duplicate entry '5' for key 'PRIMARY' [for Statement "INSERT INTO issuingrules SELECT * FROM tmpissuingrules WHERE branchcode=?" with ParamValues: 0="MPL"] at /home/marc/koha/admin/clone-rules.pl line 86.

Seems to be a separate bug, but it makes impossible to test here.
Comment 7 Marc Véron 2017-08-28 05:55:48 UTC
(In reply to Marc Véron from comment #6)
> While testing I get:
> 'Cloning of circulation and fine rules failed!'
> Plack error log says:
> DBD::mysql::st execute failed: Duplicate entry '5' for key 'PRIMARY' [for
> Statement "INSERT INTO issuingrules SELECT * FROM tmpissuingrules WHERE
> branchcode=?" with ParamValues: 0="MPL"] at
> /home/marc/koha/admin/clone-rules.pl line 86.
> 
> Seems to be a separate bug, but it makes impossible to test here.

Could be related to Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures
Comment 8 David Bourgault 2017-09-22 18:20:57 UTC
Created attachment 67323 [details] [review]
Bug 19029 - Added JavaScript security question for cloning circ rules

This patch introduces a Javascript security question which is displayed
to the user when they try to clone a circulation rule to a specific
branch when the rule is a 'Standard rule for all libraries"

The rationale for this patch is when the cloning takes place it
overwrites the existing rules of the destination branch and there is no
notification of this to the user. Therefore by implementing this patch
the user is asked if they want to clone the rule (if the rule is
standard accross all libraries) and are told that it
will overwrite the rules in the destination branch.

Test plan:
1. Create a circulation rule for all libraries

2. Make sure the 'select a library' option is set to 'Standard rules for all
libraries"

3. Click the 'Clone' button and notice that the cloning takes place
without any warning that it will overwrite the rules of the destination
branch

4. Apply patch

5. Return to the circulation and fine rules page

6. Repeat step 2

7. Click the clone button and notice a alert box appears asking if you
are sure you want to clone the standard rule to the destination branch.

Note: The name of the destination branch is included in the alert.
Also note that the user is informed of the consequences of performing
the action, i.e. that it will overwrite the existing rules in the
destination branch

8. Click 'Cancel' and notice that no cloning occurs

9. Click the clone button again and this time click 'OK' and notice
that the cloning takes place

10. Return to the Circulation and fine rules page and set the 'Select a
library' option to the name of an individual branch

11. Click the clone button and notice that the clone action takes place

Sponsored-By: Catalyst IT

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 9 David Bourgault 2017-09-22 18:23:41 UTC
The patch works as planned - the security question only shows when cloning from standard rules.

However I feel like it would be better to apply the security question to all cloning operations. Saving users the trouble of an accidental cloning is better than having 1 less click to do.
Comment 10 Jonathan Druart 2017-10-02 15:39:14 UTC
Alex, you should use .format() to ease translation.
Comment 11 Alex Buckley 2017-10-02 19:34:27 UTC
Created attachment 67529 [details] [review]
Bug 19029 - Followup - Implemented .format() to ease translation

Sponosred by: Catalyst IT
Comment 12 Katrin Fischer 2017-10-02 23:54:01 UTC
There seems to be no security question for cloning conditions from one library to another. I think it would be good to be consistent and always ask, as cloning accidentally can create quite a chaos.
Comment 13 Alex Buckley 2017-10-11 22:48:15 UTC
Created attachment 67980 [details] [review]
Bug 19029 - Followup: Make security question appear on clone operations from one library to another

Sponsored-By: Catalyst IT
Comment 14 Alex Buckley 2017-10-11 22:51:39 UTC
Hi all

I have just attached a patch which implements the security question on clone operations from one library to another which Katrin suggested in comment 12. Could the 2 follow-up patches be tested please
Comment 15 Dilan Johnpullé 2017-10-11 23:56:16 UTC
Created attachment 67983 [details] [review]
Bug 19029 - Followup: Make security question appear on clone operations from one library to another

Message pops up for all instances of cloning now.
Works as expected.

Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 16 Kyle M Hall 2017-10-27 12:55:58 UTC
Created attachment 68688 [details] [review]
Bug 19029 - Added JavaScript security question for cloning circ rules

This patch introduces a Javascript security question which is displayed
to the user when they try to clone a circulation rule to a specific
branch when the rule is a 'Standard rule for all libraries"

The rationale for this patch is when the cloning takes place it
overwrites the existing rules of the destination branch and there is no
notification of this to the user. Therefore by implementing this patch
the user is asked if they want to clone the rule (if the rule is
standard accross all libraries) and are told that it
will overwrite the rules in the destination branch.

Test plan:
1. Create a circulation rule for all libraries

2. Make sure the 'select a library' option is set to 'Standard rules for all
libraries"

3. Click the 'Clone' button and notice that the cloning takes place
without any warning that it will overwrite the rules of the destination
branch

4. Apply patch

5. Return to the circulation and fine rules page

6. Repeat step 2

7. Click the clone button and notice a alert box appears asking if you
are sure you want to clone the standard rule to the destination branch.

Note: The name of the destination branch is included in the alert.
Also note that the user is informed of the consequences of performing
the action, i.e. that it will overwrite the existing rules in the
destination branch

8. Click 'Cancel' and notice that no cloning occurs

9. Click the clone button again and this time click 'OK' and notice
that the cloning takes place

10. Return to the Circulation and fine rules page and set the 'Select a
library' option to the name of an individual branch

11. Click the clone button and notice that the clone action takes place

Sponsored-By: Catalyst IT

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 17 Kyle M Hall 2017-10-27 12:56:06 UTC
Created attachment 68689 [details] [review]
Bug 19029 - Followup - Implemented .format() to ease translation

Sponosred by: Catalyst IT
Comment 18 Kyle M Hall 2017-10-27 12:56:08 UTC
Created attachment 68690 [details] [review]
Bug 19029 - Followup: Make security question appear on clone operations from one library to another

Message pops up for all instances of cloning now.
Works as expected.

Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 19 Jonathan Druart 2017-10-27 17:15:09 UTC
Pushed to master for 17.11, thanks to everybody involved!