Bug 16949 - Batch record deletion says success when no records have been passed in
Summary: Batch record deletion says success when no records have been passed in
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-20 22:13 UTC by Aleisha Amohia
Modified: 2017-12-07 22:17 UTC (History)
3 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 16949: Batch record deletion says success when no records have been passed in (1.88 KB, patch)
2016-07-20 22:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 16949: Batch record deletion says success when no records have been passed in (2.50 KB, patch)
2016-07-28 21:22 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 16949: Batch record deletion says success when no records have been passed in (2.69 KB, patch)
2016-08-02 04:07 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Can not proceed even if items selected (47.25 KB, image/png)
2016-08-07 19:37 UTC, Marc Véron
Details
Bug 16949: Batch record deletion says success when no records have been passed in (3.00 KB, patch)
2016-08-26 02:52 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16949: Batch record deletion says success when no records have been passed in (3.11 KB, patch)
2016-09-09 14:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16949: Batch record deletion says success when no records have been passed in (3.13 KB, patch)
2016-09-13 08:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16949: Simplify the checkbox checked condition (1.60 KB, patch)
2016-09-13 08: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 Aleisha Amohia 2016-07-20 22:13:16 UTC
To reproduce:
1) Go to Tools -> Batch record deletion
2) Put in a record number and click Continue
3) Deselect the record so that it doesn't actually delete and click Delete selected records
4) Page says All records have been deleted successfully! --> Error
Comment 1 Aleisha Amohia 2016-07-20 22:17:17 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-07-21 05:15:53 UTC
Hi Aleisha,

What do you think about having a different message if no record is selected?

Maybe something like:
(...)
  [% ELSIF op == 'report' %]
    [% IF report.total_records == 0 %]
      No records were selected for deletion!
    [% ELSIF report.total_records == report.total_success %]
      All records have been deleted successfully!
    [% ELSIF report.total_success == 0 %]
      No record has been deleted. An error occurred.
    [% ELSE %]
(...)

Marc
Comment 3 Marc Véron 2016-07-21 05:19:10 UTC
(In reply to Marc Véron from comment #2)
> Hi Aleisha,
> 
> What do you think about having a different message if no record is selected?
> 
> Maybe something like:
> (...)
>   [% ELSIF op == 'report' %]
>     [% IF report.total_records == 0 %]
>       No records were selected for deletion!
>     [% ELSIF report.total_records == report.total_success %]
>       All records have been deleted successfully!
>     [% ELSIF report.total_success == 0 %]
>       No record has been deleted. An error occurred.
>     [% ELSE %]
> (...)
> 
> Marc

...and maybe an additional link back to the previous screen to give the opportunity to select records.
Comment 4 Owen Leonard 2016-07-22 12:31:08 UTC
I think a client-side check is also in order: Don't submit the form if no titles are checked. That's standard for similar interfaces in Koha.
Comment 5 Aleisha Amohia 2016-07-28 20:59:20 UTC
(In reply to Marc Véron from comment #3)
> ...and maybe an additional link back to the previous screen to give the
> opportunity to select records.

There is already a link there that says 'New batch record deletion' after the form has been submitted.
Comment 6 Aleisha Amohia 2016-07-28 21:22:53 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2016-07-30 05:51:53 UTC
Hi ALeisha,
I applied the patch, cleared the browser cache and tested.
Now I get always "No records have been selected" (with or withot items selected in checkboxes).
Comment 8 Aleisha Amohia 2016-08-02 04:07:50 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2016-08-07 19:37:01 UTC
Created attachment 54127 [details]
Can not proceed even if items selected

Re-tested, I still can not proceed with items selected, see screenshot.
Comment 10 Aleisha Amohia 2016-08-26 02:52:49 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2016-09-09 14:24:52 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-09-13 08:07:24 UTC
Created attachment 55514 [details] [review]
Bug 16949: Batch record deletion says success when no records have been passed in

The reason this happens is because the page will say success when the
total number of records given equals the total number of successful
deletions. If you pass in no records, there are no successful deletions
--> 0 = 0 --> it thinks it has been successful. This patch adds a check
that validates if any checkboxes were selected before submitting the
final form.

I have removed the check for if any records were selected AFTER the form
has been submitted because it seemed unnecessary if the form can't be
submitted without selection of records anyway.

To test:
1) Go to Tools -> Batch record deletion
2) Put in a record number and click Continue
3) Deselect the record so that it doesn't actually delete and click
   Delete selected records
4) Page says 'All records have been deleted successfully!'
5) Apply patch. Go back and repeat step 3
6) Form should not submit and you should receive an alert saying that no
   records have been selected.
7) If you try selecting and deleting a record after this alert, it
   should still work

Note: Have also changed the wording of error in Step 1 when you are
entering record numbers to delete.

Sponsored-by: Catalyst IT

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2016-09-13 08:12:50 UTC
Created attachment 55515 [details] [review]
Bug 16949: Simplify the checkbox checked condition

It's easier to use jQuery selector to know if checkboxes are checked.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Kyle M Hall 2016-09-13 18:23:34 UTC
Pushed to master for 16.11, thanks Aleisha, Jonathan!