Description
Nick Clemens (kidclamp)
2017-03-14 14:19:41 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 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> I do not understand the goal of this patch, why do barcodes are removed? (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 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? Created attachment 61272 [details] [review] Bug 18264 - (QA Followup) Remove use of GetBiblioFromItemNumber Replace <blank> with (blank) (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 Created attachment 61299 [details] [review] Bug 18264 - (QA Followup) Remove use of GetBiblioFromItemNumber Replace <blank> with (blank) 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> 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> Hm, blank is not translatable? Touches some strings, will take a look at this for the next 16.11.x release. I've filed bug 18367 about the string. Hm, a little torn about pushing this one. Opinions? 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. 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! (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. These patches have been pushed to 16.11.x and will be in 16.11.09. Thx for the hint, Jonathan! |