Bug 14805

Summary: Allow the deletion of multiple patron card batches
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: Label/patron card printingAssignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 14805: Allow the user to delete multiple batches
[Signed-off] Bug 14805: Allow the user to delete multiple batches
Bug 14805: QA follow-up
Bug 14805: (follow-up) fix a bug in previous code and change msg string
Bug 14805: Replace generic 'items' with the specific ones
Bug 14805: Replace generic 'items' with the specific ones
Bug 14805: Allow the user to delete multiple batches
Bug 14805: QA follow-up
Bug 14805: (follow-up) fix a bug in previous code and change msg string
Bug 14805: Replace generic 'items' with the specific ones
Bug 14805: Replace generic 'items' with the specific ones

Description Joonas Kylmälä 2015-09-11 08:24:57 UTC
In koha/patroncards/manage.pl?card_element=batch you cannot select multiple batches for deletion. We don't want the user to go through them one by one if the user wants to delete all the batches.
Comment 1 Joonas Kylmälä 2015-09-14 13:34:19 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-09-16 12:51:40 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-09-17 08:56:27 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-09-17 08:57:35 UTC
Work quite well, but when trying to remove more than 1 profile, the message is "Are you sure you want to delete X batches?", should be 'profiles'.
I haven't tried, but it is certainly the same for templates and layouts.
Comment 5 Joonas Kylmälä 2015-09-17 09:47:20 UTC
(In reply to Jonathan Druart from comment #4)
> Work quite well, but when trying to remove more than 1 profile, the message
> is "Are you sure you want to delete X batches?", should be 'profiles'.
> I haven't tried, but it is certainly the same for templates and layouts.

I see. I'll do a follow-up for that.
Comment 6 Joonas Kylmälä 2015-09-17 11:44:19 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-09-17 13:02:06 UTC
Comment on attachment 42647 [details] [review]
Bug 14805: (follow-up) fix a bug in previous code and change msg string

Review of attachment 42647 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/manage.tt
@@ +21,4 @@
>  
>                  var msg;
>                  if (element_ids.length > 1) {
> +                    msg = _("Are you sure you want to delete %s items?").format(element_ids.length, element_ids.length);

Is not possible to call translate_card_element here?
Comment 8 Joonas Kylmälä 2015-09-18 07:17:38 UTC
(In reply to Jonathan Druart from comment #7)
> Comment on attachment 42647 [details] [review] [review]
> Bug 14805: (follow-up) fix a bug in previous code and change msg string
> 
> Review of attachment 42647 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/manage.tt
> @@ +21,4 @@
> >  
> >                  var msg;
> >                  if (element_ids.length > 1) {
> > +                    msg = _("Are you sure you want to delete %s items?").format(element_ids.length, element_ids.length);
> 
> Is not possible to call translate_card_element here?

One way would be by adding if statements for all the different translate_card_elements. I don't know how we could otherwise conjugate the words. Any better ideas than the if statements (or case statements)?
Comment 9 Jonathan Druart 2015-09-21 14:57:55 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-09-21 14:58:41 UTC
(In reply to Joonas Kylmälä from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > Comment on attachment 42647 [details] [review] [review] [review]
> > Bug 14805: (follow-up) fix a bug in previous code and change msg string
> > 
> > Review of attachment 42647 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/manage.tt
> > @@ +21,4 @@
> > >  
> > >                  var msg;
> > >                  if (element_ids.length > 1) {
> > > +                    msg = _("Are you sure you want to delete %s items?").format(element_ids.length, element_ids.length);
> > 
> > Is not possible to call translate_card_element here?
> 
> One way would be by adding if statements for all the different
> translate_card_elements. I don't know how we could otherwise conjugate the
> words. Any better ideas than the if statements (or case statements)?

Have a look at my last patch and please sign it off if it works correctly.
Comment 11 Joonas Kylmälä 2015-09-22 07:33:47 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-09-22 09:05:29 UTC
Created attachment 42762 [details] [review]
Bug 14805: Allow the user to delete multiple batches

Allows the user to delete multiple patron card batches.

Test plan:

1. Add first some 20 batches
2. Go to the edit page of some X batch by first selecting from the
   batch list the batch you want to edit and then after that by clicking
   edit.
3. Delete one batch by selecting one batch and then clicking delete.
4. Select 0 batches and click delete, notice you cannot delete.
5. Select 0 batches and click edit, notice you cannot edit.
6. Select 2 batches and click edit, notice you cannot edit.
7. Apply patch
8. Check that steps from 2-6 work like they worked before.
   (and add more batches so that you have ~20 of them)
9. Select 2 batches and click delete. Make sure they are deleted.
10. Try to figure out some anomalies this patch might have caused.

Sponsored-by: Vaara-kirjastot

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2015-09-22 09:05:32 UTC
Created attachment 42763 [details] [review]
Bug 14805: QA follow-up

This patches remove a trailing whitespace and simplify 2 parts of code.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2015-09-22 09:05:35 UTC
Created attachment 42764 [details] [review]
Bug 14805: (follow-up) fix a bug in previous code and change msg string

Previously the code didn't work if there was only one batch available for selection.
This fixes that now. Also there was a bug with wrong string showing now it's
changed to a general one, "items". I choosed the string "items" because there was
difficulties adding the plural part of the word. One cannot add just s to the end
of the word batch like you can in the case of profile.

Sponsored-by: Vaara-kirjastot

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2015-09-22 09:05:38 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi (tcohen) 2015-11-02 15:27:30 UTC
Please rebase against master, urgently :-D
Comment 17 Joonas Kylmälä 2015-11-04 11:02:03 UTC
Created attachment 44358 [details] [review]
Bug 14805: Replace generic 'items' with the specific ones

Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Joonas Kylmälä 2015-11-04 11:03:39 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Please rebase against master, urgently :-D

Done! :)
Comment 19 Tomás Cohen Arazi (tcohen) 2015-11-04 14:31:18 UTC
Patches pushed to master.

Thanks Joonas and Jonathan!