Bug 18264 - Course reserves - use itemnumber for editing existing reserve items
Summary: Course reserves - use itemnumber for editing existing reserve items
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17974
Blocks: 18367
  Show dependency treegraph
 
Reported: 2017-03-14 14:19 UTC by Nick Clemens
Modified: 2017-06-05 13:45 UTC (History)
4 users (show)

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


Attachments
Bug 18264 - Course reserves - use itemnumber for editing existing reserve items (3.53 KB, patch)
2017-03-14 14:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18264 - Course reserves - use itemnumber for editing existing reserve items (3.70 KB, patch)
2017-03-15 10:13 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18264 - (QA Followup) Remove use of GetBiblioFromItemNumber Replace <blank> with (blank) (3.68 KB, patch)
2017-03-20 11:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18264 - (QA Followup) Remove use of GetBiblioFromItemNumber Replace <blank> with (blank) (3.71 KB, patch)
2017-03-20 13:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18264 - Course reserves - use itemnumber for editing existing reserve items (3.77 KB, patch)
2017-03-20 14:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18264 - (QA Followup) Remove use of GetBiblioFromItemNumber Replace <blank> with (blank) (3.78 KB, patch)
2017-03-20 14:22 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 Nick Clemens 2017-03-14 14:19:41 UTC
Some libraries have found that when using course reserves an item will have the barcode removed. If this happens the edit links break. We should use the itemnumber for links.
Comment 1 Nick Clemens 2017-03-14 14:21:44 UTC
Created attachment 61067 [details] [review]
Bug 18264 - Course reserves - use itemnumber for editing existing reserve items

This patch alters the crouse reserves module to use itemnumber as the
lookup method for existing reserves as opposed to barcode

To test:
1 - Apply patch
2 - Add some items to a course reserves - all should work as expected
3 - Delete the barcode for an item on reserve
4 - Note the edit link still resolves to the correct item
Comment 2 Marc Véron 2017-03-15 10:13:44 UTC
Created attachment 61095 [details] [review]
Bug 18264 - Course reserves - use itemnumber for editing existing reserve items

This patch alters the crouse reserves module to use itemnumber as the
lookup method for existing reserves as opposed to barcode

To test:
1 - Apply patch
2 - Add some items to a course reserves - all should work as expected
3 - Delete the barcode for an item on reserve
4 - Note the edit link still resolves to the correct item

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 3 Jonathan Druart 2017-03-15 12:18:39 UTC
I do not understand the goal of this patch, why do barcodes are removed?
Comment 4 Nick Clemens 2017-03-15 13:25:43 UTC
(In reply to Jonathan Druart from comment #3)
> I do not understand the goal of this patch, why do barcodes are removed?

Some workflows at libraries cause this, I think electronic items or copies may only be assigned temporary barcodes and deleted when the items are not in use - in these cases things can no longer be edited in the reserves. 

While having barcodes is expected for adding items to course reserves currently I think this change opens up more possibilities and doesn't hurt anything and solves a potential issue.
Comment 5 Jonathan Druart 2017-03-17 14:18:08 UTC
Comment on attachment 61095 [details] [review]
Bug 18264 - Course reserves - use itemnumber for editing existing reserve items

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

::: course_reserves/add_items.pl
@@ +39,2 @@
>  
> +my $item = GetBiblioFromItemNumber( $itemnumber, $barcode );

This is wrong, this subroutine uses only one parameter (also see bug 18276).

@@ +50,4 @@
>          flagsrequired   => { coursereserves => 'add_reserves' },
>      }
>  );
> +my $inumber = $itemnumber ? "<blank> (itemnumber:$itemnumber)" : "";

hum? <blank>?

@@ +51,5 @@
>      }
>  );
> +my $inumber = $itemnumber ? "<blank> (itemnumber:$itemnumber)" : "";
> +$template->param( ERROR_BARCODE_NOT_FOUND => $barcode . $inumber )
> +  unless ( $barcode && !$itemnumber && $item && $action eq 'lookup' );

How do we reach that?
Comment 6 Nick Clemens 2017-03-20 11:24:03 UTC
Created attachment 61272 [details] [review]
Bug 18264 - (QA Followup)     Remove use of GetBiblioFromItemNumber     Replace <blank> with (blank)
Comment 7 Nick Clemens 2017-03-20 11:26:33 UTC
(In reply to Jonathan Druart from comment #5)
 > > +my $item = GetBiblioFromItemNumber( $itemnumber, $barcode ); 
> This is wrong, this subroutine uses only one parameter (also see bug 18276).
Removed
 
> hum? <blank>?
In case of itemnumber lookup (no barcode) we display '(blank)' for barcode and itemnumber after


> How do we reach that?
As above case - it shouldn't happen, but makes for a nice error if someone messes up the link example with bad itemnumber:
http://localhost:8081/cgi-bin/koha/course_reserves/add_items.pl?course_id=11&itemnumber=123456789123456798&action=lookup&return=11
Comment 8 Nick Clemens 2017-03-20 13:37:42 UTC
Created attachment 61299 [details] [review]
Bug 18264 - (QA Followup) Remove use of GetBiblioFromItemNumber Replace <blank> with (blank)
Comment 9 Jonathan Druart 2017-03-20 14:22:34 UTC
Created attachment 61304 [details] [review]
Bug 18264 - Course reserves - use itemnumber for editing existing reserve items

This patch alters the crouse reserves module to use itemnumber as the
lookup method for existing reserves as opposed to barcode

To test:
1 - Apply patch
2 - Add some items to a course reserves - all should work as expected
3 - Delete the barcode for an item on reserve
4 - Note the edit link still resolves to the correct item

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 10 Jonathan Druart 2017-03-20 14:22:40 UTC
Created attachment 61305 [details] [review]
Bug 18264 - (QA Followup) Remove use of GetBiblioFromItemNumber Replace <blank> with (blank)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Katrin Fischer 2017-03-20 20:50:32 UTC
Hm, blank is not translatable?
Comment 13 Katrin Fischer 2017-03-26 20:46:34 UTC
Touches some strings, will take a look at this for the next 16.11.x release.
Comment 14 Katrin Fischer 2017-04-02 16:40:51 UTC
I've filed bug 18367 about the string.
Comment 15 Katrin Fischer 2017-04-10 21:43:53 UTC
Hm, a little torn about pushing this one. Opinions?
Comment 16 Nick Clemens 2017-04-11 19:27:53 UTC
The 'bug' part is reproducible on 16.11, I filed as a bug because it can cause issues in those limited cases where barcodes are removed.
Comment 17 Katrin Fischer 2017-05-14 11:55:03 UTC
Tried to backport these patches, but with the patches applied, I have an error under Plack when adding an item to a course by barcode:
The method biblio is not covered by tests!
Comment 18 Jonathan Druart 2017-05-15 13:50:04 UTC
(In reply to Katrin Fischer from comment #17)
> Tried to backport these patches, but with the patches applied, I have an
> error under Plack when adding an item to a course by barcode:
> The method biblio is not covered by tests!

Koha::Item->biblio is added by bug 17974.
Comment 19 Katrin Fischer 2017-06-05 13:45:55 UTC
These patches have been pushed to 16.11.x and will be in 16.11.09.

Thx for the hint, Jonathan!