Bug 18656 - Require confirmation of deletion of files from patron record
Summary: Require confirmation of deletion of files from patron record
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-23 11:30 UTC by BASE Library Consortium
Modified: 2017-07-31 11:22 UTC (History)
9 users (show)

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


Attachments
Bug 18656 - Require confirmation of deletion of files from patron record (2.50 KB, patch)
2017-05-24 13:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 18571: Add default ES configuration to koha-conf-site.xml.in (2.00 KB, patch)
2017-05-25 18:34 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 18571: Add default ES configuration to koha-conf-site.xml.in (2.06 KB, patch)
2017-05-25 18:35 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 18656 - Require confirmation of deletion of files from patron record (2.56 KB, patch)
2017-05-25 22:19 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 18656 - Require confirmation of deletion of files from patron record (2.63 KB, patch)
2017-06-09 16:03 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 18656 - Require confirmation of deletion of files from patron record (2.71 KB, patch)
2017-06-12 14:12 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 BASE Library Consortium 2017-05-23 11:30:48 UTC
On the borrower record in Files, we store the patron registration form.
 
If a member of staff accidentally hits the delete link, we would need to trace the originally scanned form at its' originating library to recover the file. 
 
To make user error more difficult, could Koha have a dialog box, 'Are you sure you want to delete this file', on the borrower record and file?
Comment 1 Owen Leonard 2017-05-24 13:52:22 UTC
Created attachment 63691 [details] [review]
Bug 18656 - Require confirmation of deletion of files from patron record

This patch adds a JavaScript confirm to the deletion of files. It also
styles the file deletion link as a Bootstrap button.

To test, apply the patch and open the "Files" page for a patron who has
files attached.

- Verify that clicking any "delete" link prompts for a confirmation.
- Verify that confirming the dialog triggers the deletion of the correct
  file.
- Verify that canceling the confirmation dialog aborts the deletion.
- Confirm that the "Delete" button looks correct.
Comment 2 PTFS Europe Sandboxes 2017-05-25 18:33:58 UTC
Patch tested with a sandbox, by David Roberts <david.roberts@ptfs-europe.com>
Comment 3 PTFS Europe Sandboxes 2017-05-25 18:34:19 UTC
Created attachment 63715 [details] [review]
Bug 18571: Add default ES configuration to koha-conf-site.xml.in

This patch adds a default configuration entry for elasticsearch. It will
add localhost:9200 to the server subsection, and koha_instance (replacing instance
for the corresponding instance name) for the namespace.

To test:
- Apply the patch
- Copy the file to the /etc/koha dir:
  $ sudo cp kohaclone/debian/templates/koha-conf-site.xml.in /etc/koha
- Create a new instance:
  $ sudo koha-create --create-db test
=> SUCCESS: /etc/koha/sites/test/koha-conf.xml includes the mentioned section:

 <elasticsearch>
     <server>localhost:9200</server>
     <index_name>koha_test</index_name>
 </elasticsearch>

- Sign off :-D

Note: As the use of ES is syspref driven, this default entry doesn't have any use
until ES is installed and SearchEngine set to Elasticsearch. So it doesn't hurt
but will help end users test the ES integration. Advanced users will take care of
this config entry manually (pointing to external servers/clusters, etc).

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: David Roberts <david.roberts@ptfs-europe.com>
Comment 4 PTFS Europe Sandboxes 2017-05-25 18:34:58 UTC
Patch tested with a sandbox, by David Roberts <david.roberts@ptfs-europe.com>
Comment 5 PTFS Europe Sandboxes 2017-05-25 18:35:20 UTC
Created attachment 63716 [details] [review]
Bug 18571: Add default ES configuration to koha-conf-site.xml.in

This patch adds a default configuration entry for elasticsearch. It will
add localhost:9200 to the server subsection, and koha_instance (replacing instance
for the corresponding instance name) for the namespace.

To test:
- Apply the patch
- Copy the file to the /etc/koha dir:
  $ sudo cp kohaclone/debian/templates/koha-conf-site.xml.in /etc/koha
- Create a new instance:
  $ sudo koha-create --create-db test
=> SUCCESS: /etc/koha/sites/test/koha-conf.xml includes the mentioned section:

 <elasticsearch>
     <server>localhost:9200</server>
     <index_name>koha_test</index_name>
 </elasticsearch>

- Sign off :-D

Note: As the use of ES is syspref driven, this default entry doesn't have any use
until ES is installed and SearchEngine set to Elasticsearch. So it doesn't hurt
but will help end users test the ES integration. Advanced users will take care of
this config entry manually (pointing to external servers/clusters, etc).

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: David Roberts <david.roberts@ptfs-europe.com>

https://bugs.koha-community.org/show_bug.cgi?id=18656

Signed-off-by: David Roberts <david.roberts@ptfs-europe.com>
Comment 6 Katrin Fischer 2017-05-25 18:35:45 UTC
Hi David, the wrong patch got attached, I am reverting this. Do you want to try again?
Comment 7 Katrin Fischer 2017-05-25 18:36:38 UTC
If it's not working from the sandboxes I can attach a patch with your sign-off line, just let me know.
Comment 8 PTFS Europe Sandboxes 2017-05-25 22:18:59 UTC
Patch tested with a sandbox, by David Roberts <david.roberts@ptfs-europe.com>
Comment 9 PTFS Europe Sandboxes 2017-05-25 22:19:20 UTC
Created attachment 63723 [details] [review]
Bug 18656 - Require confirmation of deletion of files from patron record

This patch adds a JavaScript confirm to the deletion of files. It also
styles the file deletion link as a Bootstrap button.

To test, apply the patch and open the "Files" page for a patron who has
files attached.

- Verify that clicking any "delete" link prompts for a confirmation.
- Verify that confirming the dialog triggers the deletion of the correct
  file.
- Verify that canceling the confirmation dialog aborts the deletion.
- Confirm that the "Delete" button looks correct.

Signed-off-by: David Roberts <david.roberts@ptfs-europe.com>
Comment 10 David Roberts 2017-05-25 22:20:43 UTC
Katrin, I hope that's working better now. No idea what went wrong last time.
Comment 11 Marcel de Rooy 2017-05-26 09:50:22 UTC
String changes.
Comment 12 Jonathan Druart 2017-06-09 15:55:38 UTC
Owen, should not we prefer
  $(elt).on('click', function(){});
?
Comment 13 Owen Leonard 2017-06-09 16:03:22 UTC
Created attachment 64159 [details] [review]
Bug 18656 - Require confirmation of deletion of files from patron record

This patch adds a JavaScript confirm to the deletion of files. It also
styles the file deletion link as a Bootstrap button.

To test, apply the patch and open the "Files" page for a patron who has
files attached.

- Verify that clicking any "delete" link prompts for a confirmation.
- Verify that confirming the dialog triggers the deletion of the correct
  file.
- Verify that canceling the confirmation dialog aborts the deletion.
- Confirm that the "Delete" button looks correct.

Signed-off-by: David Roberts <david.roberts@ptfs-europe.com>

Revision for QA: Replaced '.click(function' with '.on("click",function'
Comment 14 Owen Leonard 2017-06-09 16:05:05 UTC
(In reply to Jonathan Druart from comment #12)
> Owen, should not we prefer
>   $(elt).on('click', function(){});

Unless you need the additional parameters offered by on() it's mostly a stylistic question because they are functionally equivalent. However, I do prefer the on() syntax so it is worth correcting my cut-and-paste.
Comment 15 Jonathan Druart 2017-06-12 14:12:49 UTC
Created attachment 64201 [details] [review]
Bug 18656 - Require confirmation of deletion of files from patron record

This patch adds a JavaScript confirm to the deletion of files. It also
styles the file deletion link as a Bootstrap button.

To test, apply the patch and open the "Files" page for a patron who has
files attached.

- Verify that clicking any "delete" link prompts for a confirmation.
- Verify that confirming the dialog triggers the deletion of the correct
  file.
- Verify that canceling the confirmation dialog aborts the deletion.
- Confirm that the "Delete" button looks correct.

Signed-off-by: David Roberts <david.roberts@ptfs-europe.com>

Revision for QA: Replaced '.click(function' with '.on("click",function'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2017-06-12 14:27:11 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 17 Fridolin SOMERS 2017-06-13 07:06:36 UTC
Pushed to 17.05.x, will be in 17.05.01

Enable EnableBorrowerFiles to test.

Looks like an enhancement but since it protects from loosing datas its a real bug.
Comment 18 Katrin Fischer 2017-06-13 20:41:05 UTC
This patch has been pushed to 16.11.x and will be in 16.11.09.

Agree with Fridolin - this one makes sense in stable.
Comment 19 Mason James 2017-07-31 11:22:16 UTC
Pushed to 16.05.x, for 16.05.15 release