Bug 11490 - MaxItemsForBatch should be split into two new prefs
Summary: MaxItemsForBatch should be split into two new prefs
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 10163
  Show dependency treegraph
 
Reported: 2014-01-07 23:07 UTC by Christopher Brannon
Modified: 2019-10-14 19:56 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11490: Add prefs MaxItemsToProcessForBatchMod and MaxItemsToDisplayForBatchDel (3.79 KB, patch)
2016-05-11 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11490: Split MaxItemsForBatch into 2 prefs to clarify things (4.18 KB, patch)
2016-05-11 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11490: Add prefs MaxItemsToProcessForBatchMod and MaxItemsToDisplayForBatchDel (3.85 KB, patch)
2016-06-03 13:15 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 11490: Split MaxItemsForBatch into 2 prefs to clarify things (4.24 KB, patch)
2016-06-03 13:15 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 11490: Add prefs MaxItemsToProcessForBatchMod and MaxItemsToDisplayForBatchDel (3.90 KB, patch)
2016-06-17 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11490: Split MaxItemsForBatch into 2 prefs to clarify things (4.29 KB, patch)
2016-06-17 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2014-01-07 23:07:14 UTC
This discovery is made based off an enhancement being worked on in bug 11343.

It has been discovered that when patch 11343 is in place, if I set the new syspref MaxItemsForBatch to 2, and then try to do a batch delete of 3 items, it won't show the 3 items because of the limit, but it will still allow me to delete beyond the limit.

I have concern that even without this patch, staff can delete beyond the hard coded 1000 item limit.

Can anyone confirm this issue?

Christopher
Comment 1 Jonathan Druart 2015-04-23 14:42:55 UTC
Nicole, Katrin,
Could I get your opinion on this one?
Bug 8246 has been pushed a couple of weeks ago and there is inconsistency between these 2 bug reports.
According to the syspref description it should be considered as a bug.

"MaxItemsForBatch: Process up to XXX items in a single modification or deletion batch."
Comment 2 Katrin Fischer 2015-05-15 22:35:48 UTC
Hi Jonathan, 

I'd agree that the behaviour described doesn't match the syspref description.
Comment 3 Jonathan Druart 2015-05-26 08:38:49 UTC
Not sure to know what you should do here.
MaxItemsForBatch has been added because "1000" was hardcoded in the code.

In the batch item deletion tool it does not block the deletion, but does not display the item info.
In the batch item mod tool it completely blocks the modification.

This behavior seems to exist previous to bug 11343.
Comment 4 Katrin Fischer 2015-05-31 12:05:33 UTC
I feel like we should honor the system preference. 

Wondering: Does it display a warning, that not all information is displayed? (batch item deletion) 

If we don't honor the pref to its current description, we should rephrase it and also make it clear to the user that not all information is displayed.
Comment 5 Jonathan Druart 2015-06-03 08:41:41 UTC
(In reply to Katrin Fischer from comment #4)
> I feel like we should honor the system preference. 
> 
> Wondering: Does it display a warning, that not all information is displayed?
> (batch item deletion) 
> 
> If we don't honor the pref to its current description, we should rephrase it
> and also make it clear to the user that not all information is displayed.

I am not sure. In this case, people won't be able to modify more than "MaxItemsForBatch" items.
I think the limit has been introduced in order not to freeze the browser if there is too much info to display.
Comment 6 Jonathan Druart 2016-05-11 13:19:43 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-05-11 13:19:47 UTC Comment hidden (obsolete)
Comment 8 Laurence Rault 2016-06-03 13:13:28 UTC
Tested on sandbox, so the updatedatabase is made automatically
Both sysprefs are set to 1000 by default

0/ Do not execute the update DB entry
1/ Set a value != 1000 for MaxItemsForBatch
2/ Execute the update DB entry and confirm that the 2 new prefs are
initially set with the value of MaxItemsForBatch and that
MaxItemsForBatch has been deleted

Tested on sandbox, so the updatedatabase is made automatically
Both sysprefs are set to 1000 by default
MaxItemsForBatch has been deleted


3/ Set different values for these 2 prefs (2 and 3 for instance to ease
the next steps)
4/ Try to delete less than MaxItemsToDisplayForBatchDel items
=> You must see the items details
OK

5/ Try to delete more than MaxItemsToDisplayForBatchDel items
=> You must not see the items details but are allowed to delete them
OK

6/ Try to modify less than MaxItemsToProcessForBatchMod items
=> You must see the items details
OK

7/ Try to modify more than MaxItemsToProcessForBatchMod items
=> You must be restricted and not allowed to process
OK
Comment 9 Biblibre Sandboxes 2016-06-03 13:15:18 UTC
Patch tested with a sandbox, by Laurence Rault <laurence.rault@biblibre.com>
Comment 10 Biblibre Sandboxes 2016-06-03 13:15:49 UTC Comment hidden (obsolete)
Comment 11 Biblibre Sandboxes 2016-06-03 13:15:52 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2016-06-17 16:41:41 UTC
Created attachment 52496 [details] [review]
Bug 11490: Add prefs MaxItemsToProcessForBatchMod and MaxItemsToDisplayForBatchDel

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2016-06-17 16:41:48 UTC
Created attachment 52497 [details] [review]
Bug 11490: Split MaxItemsForBatch into 2 prefs to clarify things

Prior to this patch, a single pref (MaxItemsForBatch) was used to
restrict the use of the batch item modification and deletion.

On the batch modification tool, the pref is used to limit the number of
items a librarian is allowed to modify in a batch, but on the batch
deletion tool the pref is used to limit the number of items to display.

To clarify things, this patch split the pref into 2 new prefs:
MaxItemsToDisplayForBatchDel and MaxItemsToProcessForBatchMod.

Later we could add a MaxItemsToProcessForBatchDel if needed.
Note that this patch should not introduce any changes in the current behaviors.

Test plan:
0/ Do not execute the update DB entry
1/ Set a value != 1000 for MaxItemsForBatch
2/ Execute the update DB entry and confirm that the 2 new prefs are
initially set with the value of MaxItemsForBatch and that
MaxItemsForBatch has been deleted
3/ Set different values for these 2 prefs (2 and 3 for instance to ease
the next steps)
4/ Try to delete less than MaxItemsToDisplayForBatchDel items
=> You must see the items details
5/ Try to delete more than MaxItemsToDisplayForBatchDel items
=> You must not see the items details but are allowed to delete them
6/ Try to modify less than MaxItemsToProcessForBatchMod items
=> You must see the items details
7/ Try to modify more than MaxItemsToProcessForBatchMod items
=> You must be restricted and not allowed to process

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2016-06-24 11:53:34 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 15 Frédéric Demians 2016-08-05 06:42:35 UTC
Pushed in 16.05. Will be in 16.05.03.
Comment 16 Julian Maurice 2016-08-17 13:01:46 UTC
Patch pushed to 3.22.x, will be in 3.22.10