Bug 20467 - Add ability to batch add items to a course
Summary: Add ability to batch add items to a course
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords: NeedsMoreQA
Depends on:
Blocks:
 
Reported: 2018-03-23 17:05 UTC by Kyle M Hall
Modified: 2018-07-03 10:05 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20467: Add ability to batch add items to a course (13.27 KB, patch)
2018-03-23 17:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20467: Add ability to batch add items to a course (13.19 KB, patch)
2018-03-23 17:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20467: Add ability to batch add items to a course (13.20 KB, patch)
2018-03-23 17:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20467: Add ability to batch add items to a course (13.30 KB, patch)
2018-03-23 18:50 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20467: Add ability to batch add items to a course (13.35 KB, patch)
2018-03-23 19:21 UTC, Maksim Sen
Details | Diff | Splinter Review
Bug 20467: Add ability to batch add items to a course (13.46 KB, patch)
2018-03-25 15:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20467 [QA Followup]: Add Branches plugin to template (907 bytes, patch)
2018-03-29 11:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20467: (QA follow-up) Add Branches plugin to template (950 bytes, patch)
2018-03-30 08:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20467 [QA Followup]: Add Branches plugin to template (965 bytes, patch)
2018-04-03 11:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20467 [QA Followup] - Filter barcodes, Remove selector code as batch changes cannot have a previous selection, use template plugins (6.27 KB, patch)
2018-04-03 11:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20467: Remove useless use statements (760 bytes, patch)
2018-04-05 16:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20467 [QA Followup]: Display error if no or invalid course id is passed (4.19 KB, patch)
2018-04-06 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20467: Add ability to batch add items to a course (13.52 KB, patch)
2018-06-26 21:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20467: (QA follow-up) Add Branches plugin to template (1001 bytes, patch)
2018-06-26 21:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20467: (QA follow-up) Filter barcodes, remove selector code, use template plugins (6.32 KB, patch)
2018-06-26 21:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20467: Remove useless use statements (810 bytes, patch)
2018-06-26 21:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20467: (QA follow-up) Display error if no or invalid course id is passed (4.22 KB, patch)
2018-06-26 21:08 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-03-23 17:05:27 UTC
Some librarians have requested the ability to batch add items to a course using the same settings for all items.
Comment 1 Kyle M Hall 2018-03-23 17:08:10 UTC
Created attachment 73209 [details] [review]
Bug 20467: Add ability to batch add items to a course

Some librarians have requested the ability to batch add items to a
course using the same settings for all items.

Test Plan:
1) Apply this patch
2) Create a new course
3) Click the new 'Batch add reserves' button
4) Enter some valid and invalid barcodes in the text box, add some settings
5) Submit the batch, note the results list added items and invalid barcodes
6) Note the valid items were added to the course
Comment 2 Kyle M Hall 2018-03-23 17:13:27 UTC
Created attachment 73213 [details] [review]
Bug 20467: Add ability to batch add items to a course

Some librarians have requested the ability to batch add items to a
course using the same settings for all items.

Test Plan:
1) Apply this patch
2) Create a new course
3) Click the new 'Batch add reserves' button
4) Enter some valid and invalid barcodes in the text box, add some settings
5) Submit the batch, note the results list added items and invalid barcodes
6) Note the valid items were added to the course
Comment 3 Kyle M Hall 2018-03-23 17:23:43 UTC
Created attachment 73214 [details] [review]
Bug 20467: Add ability to batch add items to a course

Some librarians have requested the ability to batch add items to a
course using the same settings for all items.

Test Plan:
1) Apply this patch
2) Create a new course
3) Click the new 'Batch add reserves' button
4) Enter some valid and invalid barcodes in the text box, add some settings
5) Submit the batch, note the results list added items and invalid barcodes
6) Note the valid items were added to the course
Comment 4 Séverine Queune 2018-03-23 18:50:26 UTC
Created attachment 73219 [details] [review]
Bug 20467: Add ability to batch add items to a course

Some librarians have requested the ability to batch add items to a
course using the same settings for all items.

Test Plan:
1) Apply this patch
2) Create a new course
3) Click the new 'Batch add reserves' button
4) Enter some valid and invalid barcodes in the text box, add some settings
5) Submit the batch, note the results list added items and invalid barcodes
6) Note the valid items were added to the course

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 5 Maksim Sen 2018-03-23 19:21:39 UTC
Created attachment 73220 [details] [review]
Bug 20467: Add ability to batch add items to a course

Some librarians have requested the ability to batch add items to a
course using the same settings for all items.

Test Plan:
1) Apply this patch
2) Create a new course
3) Click the new 'Batch add reserves' button
4) Enter some valid and invalid barcodes in the text box, add some settings
5) Submit the batch, note the results list added items and invalid barcodes
6) Note the valid items were added to the course

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Comment 6 Katrin Fischer 2018-03-25 15:15:30 UTC
Created attachment 73239 [details] [review]
Bug 20467: Add ability to batch add items to a course

Some librarians have requested the ability to batch add items to a
course using the same settings for all items.

Test Plan:
1) Apply this patch
2) Create a new course
3) Click the new 'Batch add reserves' button
4) Enter some valid and invalid barcodes in the text box, add some settings
5) Submit the batch, note the results list added items and invalid barcodes
6) Note the valid items were added to the course

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2018-03-26 12:31:59 UTC
QA: Have to check something, please don't push yet!
Comment 8 Katrin Fischer 2018-03-28 05:58:21 UTC
The holding library pull down is empty in the batch add form, please fix!
Thx Owen for noticing!
Comment 9 Kyle M Hall 2018-03-29 11:33:31 UTC
Created attachment 73419 [details] [review]
Bug 20467 [QA Followup]: Add Branches plugin to template
Comment 10 Katrin Fischer 2018-03-30 08:40:45 UTC
Created attachment 73453 [details] [review]
Bug 20467: (QA follow-up) Add Branches plugin to template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Jonathan Druart 2018-04-02 20:59:49 UTC
1. Need to be patched with changes on bug 20282

2. I think you should use the TT plugin AuthorisedValues (and include file options_for_authorised_value_categories) for CCODE and LOC

3. You should use uniq on @barcodes (and use the same split we already do in tools/*: \s\n)
Comment 12 Kyle M Hall 2018-04-03 11:12:20 UTC
Created attachment 73550 [details] [review]
Bug 20467 [QA Followup]: Add Branches plugin to template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Kyle M Hall 2018-04-03 11:12:27 UTC
Created attachment 73551 [details] [review]
Bug 20467 [QA Followup] - Filter barcodes, Remove selector code as batch changes cannot have a previous selection, use template plugins
Comment 14 Jonathan Druart 2018-04-05 16:43:38 UTC
A small thing:
- Hit /course_reserves/batch_add_items.pl (without course_id parameter or invalid)
- add items
- submit

=> no error 

- "view course"

=> "Invalid course!"

I think we should warn before
Comment 15 Jonathan Druart 2018-04-05 16:49:25 UTC
Does it really make sense to have this new batch_add_items couple?
I would have adjusted add_items to accept a batch. Why do we need 2 different screens?
Comment 16 Jonathan Druart 2018-04-05 16:52:48 UTC
Created attachment 73705 [details] [review]
Bug 20467: Remove useless use statements
Comment 17 Kyle M Hall 2018-04-06 15:02:25 UTC
(In reply to Jonathan Druart from comment #15)
> Does it really make sense to have this new batch_add_items couple?
> I would have adjusted add_items to accept a batch. Why do we need 2
> different screens?

I was going to do that at first, but once I started writing the code I realized that there wasn't much shared code. So it made sense to keep the new feature in a separate script.
Comment 18 Jonathan Druart 2018-04-06 15:48:26 UTC
I am sorry Kyle but they really look the same to me (compare them with only your first patch applied).

Also, we you add a single item, you are redirected to the same 'add item' view, which means people were expecting to scan multiple items.

I really think we should have only 1 page to add item to a course, scan 1+ then redirect to the course detail.

I would like another QA POV.
Comment 19 Kyle M Hall 2018-04-06 17:15:45 UTC
(In reply to Jonathan Druart from comment #18)
> I am sorry Kyle but they really look the same to me (compare them with only
> your first patch applied).
> 
> Also, we you add a single item, you are redirected to the same 'add item'
> view, which means people were expecting to scan multiple items.
> 
> I really think we should have only 1 page to add item to a course, scan 1+
> then redirect to the course detail.
> 
> I would like another QA POV.

I think your issue is a matter of philosophy. I think it's better to have multiple simpler scripts, whereas you definitely prefer a monolithic approach. That is fine, as there is more than one way to do it ; )

That being said, I'm not interested in rewriting this patch for the sake of a difference in style. If you'd like to submit a followup to merge the two I'd be happy to sign it off!
Comment 20 Kyle M Hall 2018-04-06 18:50:59 UTC
Created attachment 73797 [details] [review]
Bug 20467 [QA Followup]: Display error if no or invalid course id is passed
Comment 21 Tomás Cohen Arazi 2018-04-09 14:57:51 UTC
I feel like the dev is good enough to be considered for inclusion. In the merge-patrons case, I did the rewrite because I didn't want to have code in Koha::* that diverted from what we've been doing for a couple years, but the logic itself was good too.
In this case, those controller scripts... I don't think this matter of taste should care that much.

If I was asked before this dev was written, I would've agreed with a same-page approach. And a REST endpoint to handle CRUD. But I don't think this should be rejected because of it.
Comment 22 Jonathan Druart 2018-04-09 20:29:22 UTC
When I edit an item, I get:
  "This course already has this item on reserve."
and
  "Number of courses reserving this item: 1"

Are they expected? (with and without the patch)

Also the legend of the fieldset always says "Add" (instead of edit)
Comment 23 Kyle M Hall 2018-04-10 10:53:46 UTC
(In reply to Jonathan Druart from comment #22)
> When I edit an item, I get:
>   "This course already has this item on reserve."
> and
>   "Number of courses reserving this item: 1"
> 
> Are they expected? (with and without the patch)
> 
> Also the legend of the fieldset always says "Add" (instead of edit)

Yes, that is expected though not perfect. The "Number of courses" message is exactly how it should be. We could pass an 'edit' flag to change the message and legend when purposefully editing a course item ( as opposed adding it only to find it's already on the course ).

If you would file a separate bug report I'd be happy to submit a patch for that!
Comment 24 Katrin Fischer 2018-06-26 06:29:30 UTC
Back on this, will fix patch subjects.
Comment 25 Katrin Fischer 2018-06-26 21:00:28 UTC
I've filed bug 21003 for the superfluous message about the existing item. It makes no sense when editing a reserve item.
Comment 26 Katrin Fischer 2018-06-26 21:08:04 UTC
Created attachment 76468 [details] [review]
Bug 20467: Add ability to batch add items to a course

Some librarians have requested the ability to batch add items to a
course using the same settings for all items.

Test Plan:
1) Apply this patch
2) Create a new course
3) Click the new 'Batch add reserves' button
4) Enter some valid and invalid barcodes in the text box, add some settings
5) Submit the batch, note the results list added items and invalid barcodes
6) Note the valid items were added to the course

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2018-06-26 21:08:08 UTC
Created attachment 76469 [details] [review]
Bug 20467: (QA follow-up) Add Branches plugin to template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2018-06-26 21:08:13 UTC
Created attachment 76470 [details] [review]
Bug 20467: (QA follow-up)  Filter barcodes, remove selector code, use template plugins

Removes selector code as batch changes cannot have a previous selection.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2018-06-26 21:08:17 UTC
Created attachment 76471 [details] [review]
Bug 20467: Remove useless use statements

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2018-06-26 21:08:21 UTC
Created attachment 76472 [details] [review]
Bug 20467: (QA follow-up) Display error if no or invalid course id is passed

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Nick Clemens 2018-07-02 13:17:51 UTC
Awesome work all!

Pushed to master for 18.11
Comment 32 Martin Renvoize 2018-07-03 10:05:17 UTC
Enhancement, not backporting to 18.05.x series.