Bug 7813

Summary: Add ability to delete local cover images
Product: Koha Reporter: Magnus Enger <magnus>
Component: OPACAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P3 CC: chris, dcook, gmcharlt, jonathan.druart, keinequal, liz, melia, nengard, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 7813: Ability to delete local cover images
Bug 7813: Ability to delete local cover images
Bug 7813: Followup Ability to delete local cover images
Bug 7813: Followup Ability to delete local cover images
Bug 7813: Ability to delete local cover images
Bug 7813: Followup Ability to delete local cover images

Description Magnus Enger 2012-03-23 13:56:22 UTC
Bug 1633 introduced the possibility of local cover images, but without the means to delete images once they have been added.
Comment 1 Magnus Enger 2012-11-20 18:36:09 UTC
*** Bug 9119 has been marked as a duplicate of this bug. ***
Comment 2 Melia Meggs 2013-01-04 20:29:49 UTC
So if you upload an image that you don't like, you have no means of getting rid of it except by replacing it with something else?  Seems like there really should be an option to delete an image.  I'm going to bump up the importance a little.
Comment 3 Liz Rea 2013-02-02 06:20:19 UTC
I agree - we need to be able to remove them.
Comment 4 keinequal 2013-03-02 18:37:40 UTC
So, how is the ability to delete the local cover image different/similar from the ability to delete the image of the patron? There should not be that much difference in code.
Comment 5 Chris Cormack 2013-03-02 18:52:58 UTC
(In reply to comment #4)
> So, how is the ability to delete the local cover image different/similar
> from the ability to delete the image of the patron? There should not be that
> much difference in code.

A great learning exercise would be to look .. then send a patch implementing the feature :)
Comment 6 Tomás Cohen Arazi 2013-09-02 18:20:06 UTC Comment hidden (obsolete)
Comment 7 David Cook 2013-09-03 03:10:53 UTC Comment hidden (obsolete)
Comment 8 David Cook 2013-09-03 03:58:50 UTC
Alas, when I try in 3.8.0 (after manually merging the patch with the code), I get this error:

Uncaught Syntax error, unrecognized expression: [{"imagenumber":"13","deleted":1}] jquery.js:19

F.filter jquery.js:19
F jquery.js:19
F jquery.js:19
o.fn.o.find jquery.js:12
jQuery.fn.find jquery.hotkeys.min.js:1
o.fn.o.init jquery.js:12
l.jQuery.l.$ jquery.js:12
$.ajax.success detail.pl?biblionumber=179:102
I jquery.js:19
N jquery.js:19

Maybe I made a mistake doing the merge? Or maybe an older version of jQuery has a harder time processing JSON but that seems strange...
Comment 9 Tomás Cohen Arazi 2013-09-03 16:29:49 UTC
(In reply to David Cook from comment #8)
> Alas, when I try in 3.8.0 (after manually merging the patch with the code),
> I get this error:
> 
> Uncaught Syntax error, unrecognized expression:
> [{"imagenumber":"13","deleted":1}] jquery.js:19
> 
> Maybe I made a mistake doing the merge? Or maybe an older version of jQuery
> has a harder time processing JSON but that seems strange...

I'm not sure about that one, but it might be due to:
- wrong jquery version (incorrectly processing the application/json content-type)
- wrong JSON Perl library (not outputting the right format)

I'd like to note that I used bootstrap on this patch, and there are some visual gli-glitches when merging into 3.8.x.
Comment 10 Jonathan Druart 2013-09-20 11:12:59 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-09-20 11:14:36 UTC
Hi Tomás,
I think this followup improves yours. Could you have a look at it and add a signoff if you agree?
Comment 12 Tomás Cohen Arazi 2013-09-20 13:32:37 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2013-09-20 13:56:36 UTC
Marked as Passed QA.
Comment 14 Jonathan Druart 2013-09-20 13:57:33 UTC
Created attachment 21276 [details] [review]
Bug 7813: Ability to delete local cover images

This patch adds the ability to delete local cover images from the detail page. It adds a 'x' button to trigger the deletion. It occurs using a new SVC script called cover_images that accepts the following parameters:
 - action
 - biblionumber (used for checking imagenumber validity)
 - imagenumber (repeatable)
It then deletes all valid images passed and returns a JSON object containing the imagenumbers and the deletion status (1/0). The operation is triggered using jQuery.ajax, and with the response the corresponding images get deleted.

Note: currently one image is deleted at a time, but the code is there to be used.

To test:
- Open the detail page fr a biblionumber containing one or more cover images, go to its images tab.
- Apply the patch, reload the page, go to the images tab.
- Thumbnails should look better than before ;-)
- An 'X' button should show below each cover.
- Clicking the 'X' button should trigger an alert message asking for confirmation.
 a) Cancel: nothing happens, reload the page and check nothing got deleted.
 b) Accept: the deleted image should be gone, reload the page to check it doesn't exist anymore and is not a browser/DOM manipulation trick.
- Signoff

Note: check having more cover images than those that fit the width and see it wraps fine.

Any comments are welcome, this is WIP.
Thanks
To+

P.S. I chose not to implement every possible action on the svc script, but another bug could be filled for more improvements.

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 15 Jonathan Druart 2013-09-20 13:57:39 UTC
Created attachment 21277 [details] [review]
Bug 7813: Followup Ability to delete local cover images

This patch:
- displays a js alert if a problem occurred on deleting the
image on the server.
- adds a class for the new span (in order to avoid future problem, if
  someone adds a new span).
- adds a title for the [x] link.
- deal with the upload_local_cover_images permission (don't display the
  delete link if the logged in user does not have it).
- changes the license version.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
I like the ehancements to the original patch and work as expected.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 16 Galen Charlton 2013-09-21 19:22:51 UTC
Pushed to master, along with a follow-up that replaces the "&times;" symbol with more descriptive text to make it easier for users and translators.

Thanks, Tomás and Jonathan!
Comment 17 Tomás Cohen Arazi 2013-11-13 16:22:13 UTC
This patch has been pushed to 3.12.x, will be in 3.12.7.
Long standing usability bug solved :-D