Created attachment 61122 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface.
Created attachment 61123 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch
Created attachment 61124 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author)
Created attachment 61125 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial
Created attachment 61126 [details] [review] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine
While testing first patch I get an Internal Server Error. Plack error log says: Template process failed: undef error - The method ccode is not covered by tests! at (...)/C4/Templates.pm line 121.
Created attachment 61177 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface.
Created attachment 61178 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch
Created attachment 61179 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author)
Created attachment 61180 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial
Created attachment 61181 [details] [review] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine
Merge conflicts in 1st patch: koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/add_items-step2.tt course_reserves/add_items.pl Maybe a merge conflict in 3rd patch as well (other patches applied when i skipped 1st and 3rd patch)
Created attachment 62081 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface.
Created attachment 62082 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch
Created attachment 62083 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author)
Created attachment 62084 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial
Created attachment 62085 [details] [review] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine
Bug 18276: Remove GetBiblioFromItemNumber - Course reserves: Internal server error while displaying course detail in OPAC. Plack error log says: Template process failed: undef error - The method datedue is not covered by tests! at .../koha/C4/Templates.pm line 121 It must be related with line 76 of opac-course-details.tt rsp. an error inside item-status.inc <td>[% INCLUDE 'item-status.inc' item=cr.item issue=cr.issue %]</td> (No error if I remove the template directive)
Kyle, could you take a look please? On current master opac-course-details.tt includes item-status.inc. But to be displayed correctly the status need info from the checkout. These info are never sent to the template. What were you expecting to display under this "status" column?
(In reply to Jonathan Druart from comment #19) > Kyle, could you take a look please? > On current master opac-course-details.tt includes item-status.inc. > But to be displayed correctly the status need info from the checkout. > These info are never sent to the template. > What were you expecting to display under this "status" column? Each element in the course_reserves loop should have a key 'issue' if it has an open issue. This data is pulled by GetCourseReserves using GetOpenIssue. It's meant to show what the item's general status is ( available, lost, damaged, checked out, etc ). Does that make sense?
Kyle, What I meant is that item-status.inc needs item.waiting, item.itemnotforloan, item.transfertwhen, item.datedue and more to display the status of the items. These flags are set from C4::Search::searchResults, not GetBiblioFromItemNumber (which fills the item values sent to the include).
Created attachment 62476 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface.
Created attachment 62477 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch
Created attachment 62478 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author)
Created attachment 62479 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial
Created attachment 62480 [details] [review] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine
Created attachment 62481 [details] [review] Bug 18276: FIX status display for course reserves This include file is terribly wrong, it's called from different places that do not set the same flag. The status from detail and result page might be different from the one display on the course reserve table.
I've got this error on course reserve detail page in opac: Template process failed: undef error - The method onsite_checkout is not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121. Also there are some problems reported by qa tools: FAIL C4/SIP/ILS/Item.pm FAIL forbidden patterns forbidden pattern: tab char (line 74) forbidden pattern: tab char (line 99) forbidden pattern: tab char (line 100) forbidden pattern: tab char (line 90) FAIL C4/SIP/ILS/Transaction/Hold.pm FAIL forbidden patterns forbidden pattern: tab char (line 108) FAIL circ/returns.pl FAIL valid Global symbol "$biblio" requires explicit package name circ/returns.pl had compilation errors. FAIL circ/transferstoreceive.pl FAIL valid Global symbol "$num" requires explicit package name Global symbol "$getransf" requires explicit package name Global symbol "%getransf" requires explicit package name syntax error Global symbol "$biblio" requires explicit package name Global symbol "%branchloop" requires explicit package name circ/transferstoreceive.pl has too many errors. FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/moveitem.tt FAIL forbidden patterns forbidden pattern: tab char (line 39) forbidden pattern: tab char (line 46) FAIL offline_circ/process_koc.pl FAIL valid Global symbol "$biblio" requires explicit package name offline_circ/process_koc.pl had compilation errors.
Created attachment 63121 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface.
Created attachment 63122 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch
Created attachment 63123 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author)
Created attachment 63124 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial
Created attachment 63125 [details] [review] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine
Created attachment 63126 [details] [review] Bug 18276: FIX status display for course reserves This include file is terribly wrong, it's called from different places that do not set the same flag. The status from detail and result page might be different from the one display on the course reserve table.
I would suggest to not make this change a priority for 17.05
(In reply to Jonathan Druart from comment #30) > Created attachment 63122 [details] [review] [review] > Bug 18276: Remove GetBiblioFromItemNumber - ILSDI > > Test plan: > Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 > The results should be the same before and after this patch Works good for item level holds, with title level holds I see: "Can't call method "biblio" on an undefined value at /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 422"
All other looks good for me
Created attachment 63166 [details] [review] Bug 18276: (follow-up) Remove GetBiblioFromItemNumber - ILSDI Take into account biblio level holds
(In reply to Josef Moravec from comment #36) > Works good for item level holds, with title level holds I see: > > "Can't call method "biblio" on an undefined value at > /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 422" Great catch Josef!
Created attachment 63294 [details] [review] [SIGNED-OFF] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63295 [details] [review] [SIGNED-OFF] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63296 [details] [review] [SIGNED-OFF] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author) Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63297 [details] [review] [SIGNED-OFF] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63298 [details] [review] [SIGNED-OFF] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63299 [details] [review] [SIGNED-OFF] Bug 18276: FIX status display for course reserves This include file is terribly wrong, it's called from different places that do not set the same flag. The status from detail and result page might be different from the one display on the course reserve table. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63300 [details] [review] [SIGNED-OFF] Bug 18276: (follow-up) Remove GetBiblioFromItemNumber - ILSDI Take into account biblio level holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
QA: Looking here now
Created attachment 64748 [details] [review] Bug 18276: [QA Follow-up] Two newlines to make qa tools happy FAIL pod Apparent command =cut not preceded by blank line in file misc/cronjobs/advance_notices.pl FAIL pod Apparent command =cut not preceded by blank line in file C4/SIP/ILS/Item.pm
Add checks for find please. Can't call method "biblio" on an undefined value at circ/circulation.pl line 334. Before this patch set you get a warning on an unknown barcode, now an Internal server error..
Created attachment 64809 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 64810 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 64811 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author) Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 64812 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 64813 [details] [review] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 64814 [details] [review] Bug 18276: FIX status display for course reserves This include file is terribly wrong, it's called from different places that do not set the same flag. The status from detail and result page might be different from the one display on the course reserve table. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 64815 [details] [review] Bug 18276: (follow-up) Remove GetBiblioFromItemNumber - ILSDI Take into account biblio level holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 64816 [details] [review] Bug 18276: [QA Follow-up] Two newlines to make qa tools happy FAIL pod Apparent command =cut not preceded by blank line in file misc/cronjobs/advance_notices.pl FAIL pod Apparent command =cut not preceded by blank line in file C4/SIP/ILS/Item.pm
Fixed conflicts with bug 17843 and bug 17738.
Created attachment 64817 [details] [review] Bug 18276: barcode may be nonexistent when checking out/in
(In reply to Marcel de Rooy from comment #49) > Add checks for find please. > > Can't call method "biblio" on an undefined value at circ/circulation.pl line > 334. > > Before this patch set you get a warning on an unknown barcode, now an > Internal server error.. Thanks for catching that Marcel. Must be fixed with the last patch. The other calls look safe.
Created attachment 64878 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Course reserves Values from the items, biblio and biblioitems tables are used in the template, so we need to pass all of them to the template, but separately. That way we easily see which field from which table we are displaying. Test plan: Create a course reserve and add items. Correct information must be displayed on the detail page of the course reserve, on staff and OPAC interface. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64879 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - ILSDI Test plan: Hit /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_holds=1 The results should be the same before and after this patch Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64880 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - circulation pages Test plan: 1. Checkout items in a batch The information displayed should be ok (title, subtitles, author, itemnotes, barcode) 2. Checkout an item using the standard checkout page You should correctly see the information of the item that has been checked out (title and author) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64881 [details] [review] Bug 18276: Remove GetBiblioFromItemNumber - Easy ones The subroutine C4::Biblio::GetBiblioFromItemNumber was wrong for several reasons: - badly named, we can get biblio info from a barcode - SELECT * from items, biblio and biblioitems makes things hard to follow and debug, we never know where do come from the value we display - sometimes called only for trivial information such as biblionumber, author or title This patchset suggests to replace it with calls to: - Koha::Items->find for item's info - $item->biblio for biblio's info - $item->biblio->biblioitem for biblioitem's info Test plan: Item's info should correctly be displayed on the following pages: - circulation history - transfer book - checkin - waiting holds QA will check the other changes reading the code, it's trivial Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64882 [details] [review] Bug 18276: Remove the C4::Biblio::GetBiblioFromItemNumber subroutine Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64883 [details] [review] Bug 18276: FIX status display for course reserves This include file is terribly wrong, it's called from different places that do not set the same flag. The status from detail and result page might be different from the one display on the course reserve table. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64884 [details] [review] Bug 18276: (follow-up) Remove GetBiblioFromItemNumber - ILSDI Take into account biblio level holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64885 [details] [review] Bug 18276: [QA Follow-up] Two newlines to make qa tools happy FAIL pod Apparent command =cut not preceded by blank line in file misc/cronjobs/advance_notices.pl FAIL pod Apparent command =cut not preceded by blank line in file C4/SIP/ILS/Item.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64886 [details] [review] Bug 18276: barcode may be nonexistent when checking out/in Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64887 [details] [review] Bug 18276: [QA Follow-up] Fix two new calls Found two calls in opac-issue-note.pl and svc/patron_notes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64974 [details] [review] Bug 18276: (follow-up) Remove GetBiblioFromItemNumber - Easy ones
Created attachment 64975 [details] [review] Bug 18276: (follow-up) Remove GetBiblioFromItemNumber - Easy ones - Item does not have a title attribute, it comes from biblio - There is an additional call to effective_itemtype done on AddReturn, so we need to catch both warnings
Pushed to master for 17.11, thanks to everybody involved!
Enhancement not pushed to 17.05.x