Bug 3134 - Ability to select multiple reports to delete at once
Summary: Ability to select multiple reports to delete at once
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 10761
  Show dependency treegraph
 
Reported: 2009-04-18 07:40 UTC by Nicole C. Engard
Modified: 2014-12-07 20:02 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 3134 - Ability to selelct multiple reports to delete at once (5.09 KB, patch)
2013-01-15 15:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3134 - Ability to selelct multiple reports to delete at once (6.67 KB, patch)
2013-07-03 14:37 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3134 - Ability to selelct multiple reports to delete at once (6.21 KB, patch)
2013-07-19 19:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3134 - Ability to selelct multiple reports to delete at once (6.26 KB, patch)
2013-07-22 00:07 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3134 - Ability to selelct multiple reports to delete at once (5.20 KB, patch)
2013-08-19 13:47 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3134 [Follow-up] Ability to select multiple reports to delete at once (3.62 KB, patch)
2013-08-19 14:30 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3134 - Ability to selelct multiple reports to delete at once (5.22 KB, patch)
2013-08-20 21:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 3134 [Follow-up] Ability to select multiple reports to delete at once (3.92 KB, patch)
2013-08-20 21:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 3134 - Ability to selelct multiple reports to delete at once (5.27 KB, patch)
2013-08-21 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3134 [Follow-up] Ability to select multiple reports to delete at once (3.97 KB, patch)
2013-08-21 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3134: Add the warn class on tr elements (1.48 KB, patch)
2013-08-21 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3134: ID and Report name columns are not well sorted. (1.64 KB, patch)
2013-08-21 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3134: Reindent delete_report (1.18 KB, patch)
2013-08-21 08:47 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 Chris Cormack 2010-05-21 01:06:51 UTC


---- Reported by nengard@gmail.com 2009-04-18 19:40:40 ----

It would be nice if there were check boxes next to reports so you can select this month's reports and delete them when you run next month's ... that way you don't end up with 100s of reports.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:06 UTC  ---

This bug was previously known as _bug_ 3134 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3134

Actual time not defined. Setting to 0.0
CC member bchurch@ptfs.com does not have an account here

Comment 1 Owen Leonard 2013-01-15 15:48:32 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2013-03-21 13:44:42 UTC
Owen,

What was wrong with this patch?

Nicole
Comment 3 Owen Leonard 2013-07-03 14:37:16 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2013-07-03 14:39:27 UTC
Comment on attachment 19376 [details] [review]
Bug 3134 - Ability to selelct multiple reports to delete at once

Grrr... Another prematurely-sent version of this patch. This one breaks the report group tabs. Back to the drawing board.
Comment 5 Liz Rea 2013-07-05 07:52:11 UTC
I actually think the number serves a purpose - rather than the name, which is not unique, the number gives librarians a way to identify a specific report - I often find myself asking "which number report are you running? There are three labeled "Circ stats - TEST"" or somesuch. Showing the number saves a lot of back and forth on that. Librarians often refer to the reports by number as well, just naturally.

So please don't remove them. :)

Liz
Comment 6 Katrin Fischer 2013-07-07 08:02:19 UTC
I'd like to keep the report numbers too - apart from what Liz said it's an easy way to find the newest reports.
Comment 7 Owen Leonard 2013-07-19 19:50:10 UTC Comment hidden (obsolete)
Comment 8 Liz Rea 2013-07-22 00:07:34 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-08-18 19:20:06 UTC
Hi Owen,
could you rebase your patch please? I tried but might have gone wrong somewhere. Would it be possible that you provide a unit test for the changed sub delete_report?
Comment 10 kenza 2013-08-19 12:16:44 UTC
(In reply to Katrin Fischer from comment #9)
> Hi Owen,
> could you rebase your patch please? I tried but might have gone wrong
> somewhere. Would it be possible that you provide a unit test for the changed
> sub delete_report?

I can provide the unit test for delete_report if necessary.
Comment 11 kenza 2013-08-19 13:36:27 UTC
(In reply to kenza from comment #10)
> (In reply to Katrin Fischer from comment #9)
> > Hi Owen,
> > could you rebase your patch please? I tried but might have gone wrong
> > somewhere. Would it be possible that you provide a unit test for the changed
> > sub delete_report?
> 
> I can provide the unit test for delete_report if necessary.

Bug 10761 - UT: saved_report and delete_report in C4::Reports::Report_Guided.pm need unit tests.
Comment 12 Owen Leonard 2013-08-19 13:37:04 UTC
(In reply to kenza from comment #10)
> I can provide the unit test for delete_report if necessary.

I would appreciate that very much kenza.
Comment 13 Owen Leonard 2013-08-19 13:47:56 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2013-08-19 14:01:06 UTC
QA comment:

The first column contains the report id *and* a checkbox => checkboxes are not aligned. I suspect a wrong rebase. I think we want only the checkbox.

Marked as Failed QA.
Comment 15 Jonathan Druart 2013-08-19 14:02:01 UTC
(In reply to Jonathan Druart from comment #14)
> QA comment:
> 
> The first column contains the report id *and* a checkbox => checkboxes are
> not aligned. I suspect a wrong rebase. I think we want only the checkbox.
> 
> Marked as Failed QA.

Or maybe the checkbox should be before the id. Like that, checkboxes will be aligned.
Comment 16 Owen Leonard 2013-08-19 14:30:28 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-08-20 21:43:27 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2013-08-20 21:43:41 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2013-08-21 08:46:31 UTC
QA comment:

These patches add the ability to delete multiple reports.
The second one fixes the ergonomic issue (described in comment 14).

I will provide 3 followups:
1/ Fix the following issue:
(In reply to Katrin Fischer from comment #18)
> There is one little thing to note: If you delete a single report
> using the option from the menu, the line you want to delete
> is highlighted in red. It would be nice to do that also for
> deleting multiple reports.

2/ Fix the sort on id and report name

3/ Reindent the delete_report routine (shouldn't it be delete_reports?) and remove the $sth->finish statement.

Note for RM: feel free to push only the 2 first patches and to ask for a new report for the 3 followups.

Marked as Passed QA.
Comment 20 Jonathan Druart 2013-08-21 08:47:02 UTC
Created attachment 20524 [details] [review]
Bug 3134 - Ability to selelct multiple reports to delete at once

This patch adds the option to select multiple saved reports for
deletion.

To test you must have two or more saved reports to delete. Deletion
should work properly when:

- Selecting one report for deletion by checking the box.
- Selecting more than one report for deletion by checking boxes.
- Clicking the old "Delete" link

Clicking the delete button should prompt you to confirm. Clicking cancel
should cancel.

Clicking the delete button when no boxes are checked should trigger an
alert asking you to select reports for deletion.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Functional tests pass, template tests pass.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 21 Jonathan Druart 2013-08-21 08:47:08 UTC
Created attachment 20525 [details] [review]
Bug 3134 [Follow-up] Ability to select multiple reports to delete at once

This follow-up moves the checkboxes into a separate column from the
report ID in order to prevent visual inconsistencies.

To test, repeat the test plan from the original patch:

You must have two or more saved reports to delete. Deletion
should work properly when:

- Selecting one report for deletion by checking the box.
- Selecting more than one report for deletion by checking boxes.
- Clicking the old "Delete" link

Clicking the delete button should prompt you to confirm. Clicking cancel
should cancel.

Clicking the delete button when no boxes are checked should trigger an
alert asking you to select reports for deletion.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
This works really nicely, passes all tests and the QA script.

There is one little thing to note: If you delete a single report
using the option from the menu, the line you want to delete
is highlighted in red. It would be nice to do that also for
deleting multiple reports.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 22 Jonathan Druart 2013-08-21 08:47:13 UTC
Created attachment 20526 [details] [review]
Bug 3134: Add the warn class on tr elements

This patch adds the same behavior when deleting 1 or more reports.
When checkboxes are checked and the "delete selected" button is pressed,
checked lines are hightlighted in red.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 23 Jonathan Druart 2013-08-21 08:47:17 UTC
Created attachment 20527 [details] [review]
Bug 3134: ID and Report name columns are not well sorted.

The sort for ID is alpha instead of numerical, same for the report
names.

To test:
create id=1, name=Report 1
create id=2, name=Report 2
create id=10, name=Report 10
create id=11, name=Report 11

Sorting by id:
without this patch 1, 10, 11, 2
with this patch: 1, 2, 10, 11

Sorting by name:
without this patch: Report 1, Report 10, Report 11, Report 2
with this patch: Report 1, Report 2, Report 10, Report 11

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 24 Jonathan Druart 2013-08-21 08:47:22 UTC
Created attachment 20528 [details] [review]
Bug 3134: Reindent delete_report

The first patch add a bad indentation for this routine. This patch fixes
that.
The $sth->finish statement is useless and should be removed.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 25 Galen Charlton 2013-08-21 14:48:35 UTC
Pushed to master.  Thanks, Owen and Jonathan!