[Bug 19004] New: Patrons.t fails when item-level_itypes is not set
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Bug ID: 19004 Summary: Patrons.t fails when item-level_itypes is not set Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Test Suite Assignee: chris@bigballofwax.co.nz Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org Problem is that TestBuilder generates two biblio records, one via item.biblionumber and one via items.biblioitemnumber => biblioitem.biblionumber. (Which actually is bad design; the biblionumber should not be in the items table.) The biblio generated via items.biblionumber has no biblioitem. AddReturn is called in Patrons.t. When item-level_itypes is false, it gets the itemtype from $biblio->biblioitem->itemtype. Which generates an error on the biblio since it has not biblioitem. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Patrons.t fails when |Koha/Patrons.t fails when |item-level_itypes is not |item-level_itypes is not |set |set -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|chris@bigballofwax.co.nz |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65789 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65789&action=edit Bug 19004: Patrons.t should create its own data for enrollment fees. If the patron categories J, K, YA would not exist, Patrons.t would fail. Test plan: [1] Remove one of these patron categories. [2] Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65790 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65790&action=edit Bug 19004: Adjust AddReturn for retrieving item type In the regular situation, you can get itemtype via biblio and then biblioitem as well as via biblioitem (at least when item-level_itypes is set to biblio). But since Koha unfortunately defined two relations in item, one for biblioitemnumber (the good one) and one for biblionumber (redundant), TestBuilder (correctly) builds one biblioitem and two biblios. If you item-level_itypes to biblio record, this will result in failing tests when calling AddReturn (in this case Koha/Patrons.t). It will crash on: Can't call method "itemtype" on an undefined value at C4/Circulation.pm line 1826. Cause: AddReturn goes via the biblionumber to biblio and than to biblioitems, and it does not find a biblioitem. (Not a fault from TestBuilder but a database design problem.) This patch makes a small change in AddReturn to retrieve the itemtype via biblioitem. It actually is a shorter road than items->biblio->biblioitems. Note: I do not test the Biblioitems->find call, since we already checked the GetItem call before and we have a foreign key constraint. I did not call $item->effective_itemtype since we still use GetItem; this could be done later. Adjusted Circulation/Returns.t too: If we add an item with TestBuilder and we called AddBiblio before, we should link biblioitemnumber as well. Test plan: [1] Do not apply this patch yet. [2] Set item-level_itypes to biblio record. [3] Run t/db_dependent/Koha/Patrons.t. (It should fail.) [4] Apply this patch. [5] Run t/db_dependent/Koha/Patrons.t again. [6] Run t/db_dependent/Circulation/Returns.t [7] Git grep on AddReturn and run a few other tests calling it. Note: Bugs 19070/19071 address three tests that call AddReturn too. [8] In the interface, check in a book. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Note: The design problem around items.biblionumber is a hard one since it is widely used field. Fixing the issue by adjusting TestBuilder is a workaround I thought about, but I did not want to introduce that change here. The change on AddReturn is a tiny improvement and fixes some tests calling it in combination with biblio level itemtypes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- vagrant@kohadevbox:kohaclone(bug_19004)$ sudo koha-shell -c bash kohadev kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove `grep AddReturn \`find . -type f -name "*.t"\` | cut -f1 -d: | sort -u` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65789|0 |1 is obsolete| | --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 66037 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66037&action=edit [SIGNED-OFF] Bug 19004: Patrons.t should create its own data for enrollment fees. If the patron categories J, K, YA would not exist, Patrons.t would fail. Test plan: [1] Remove one of these patron categories. [2] Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 66038 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66038&action=edit [SIGNED-OFF] [SIGNED-OFF] Bug 19004: Adjust AddReturn for retrieving item type In the regular situation, you can get itemtype via biblio and then biblioitem as well as via biblioitem (at least when item-level_itypes is set to biblio). But since Koha unfortunately defined two relations in item, one for biblioitemnumber (the good one) and one for biblionumber (redundant), TestBuilder (correctly) builds one biblioitem and two biblios. If you item-level_itypes to biblio record, this will result in failing tests when calling AddReturn (in this case Koha/Patrons.t). It will crash on: Can't call method "itemtype" on an undefined value at C4/Circulation.pm line 1826. Cause: AddReturn goes via the biblionumber to biblio and than to biblioitems, and it does not find a biblioitem. (Not a fault from TestBuilder but a database design problem.) This patch makes a small change in AddReturn to retrieve the itemtype via biblioitem. It actually is a shorter road than items->biblio->biblioitems. Note: I do not test the Biblioitems->find call, since we already checked the GetItem call before and we have a foreign key constraint. I did not call $item->effective_itemtype since we still use GetItem; this could be done later. Adjusted Circulation/Returns.t too: If we add an item with TestBuilder and we called AddBiblio before, we should link biblioitemnumber as well. Test plan: [1] Do not apply this patch yet. [2] Set item-level_itypes to biblio record. [3] Run t/db_dependent/Koha/Patrons.t. (It should fail.) [4] Apply this patch. [5] Run t/db_dependent/Koha/Patrons.t again. [6] Run t/db_dependent/Circulation/Returns.t [7] Git grep on AddReturn and run a few other tests calling it. Note: Bugs 19070/19071 address three tests that call AddReturn too. [8] In the interface, check in a book. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Note: Bugs 19070 and 19071 are already pushed. The command in comment #4 has all the tests successful. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65790|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to M. Tompsett from comment #6)
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Thanks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 66486 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66486&action=edit Bug 19004: build_object takes care of existing data Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 66038 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66038 [SIGNED-OFF] [SIGNED-OFF] Bug 19004: Adjust AddReturn for retrieving item type Review of attachment 66038: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=19004&attachment=66038) ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +1822,5 @@
my $itemnumber = $item->{ itemnumber }; + my $itemtype = C4::Context->preference("item-level_itypes") + ? $item->{itype} + : Koha::Biblioitems->find( $item->{biblioitemnumber} )->itemtype;
Was this code correct? Sorry if this is silly, but I am lacking time at the moment to take a look correctly: Why do not we just get $item->{itype} as the item-level_itypes is checked in GetItem (via Koha::Item->effective_itemtype)? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Actually either the previous version was good (to handle specific cases, i.e. wrong data) and yours is wrong, or we could just use $item->{itype}. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66037|[SIGNED-OFF] Bug 19004: |Bug 19004: Patrons.t should description|Patrons.t should create its |create its own data for |own data for enrollment |enrollment fees. |fees. | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66038|[SIGNED-OFF] [SIGNED-OFF] |Bug 19004: Adjust AddReturn description|Bug 19004: Adjust AddReturn |for retrieving item type |for retrieving item type | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #8)
Created attachment 66486 [details] [review] Bug 19004: build_object takes care of existing data
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
It works, but it will trigger a "unique violation" warning from build. Which is valid since build should create data not find it. (I once proposed a flag to suppress this warning, but it was not welcomed.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #10)
Actually either the previous version was good (to handle specific cases, i.e. wrong data) and yours is wrong, or we could just use $item->{itype}.
We can just use $item->{itype}. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66546 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66546&action=edit Bug 19004: [QA Follow-up] No need to check item-level_itypes again As Jonathan pointed out, GetItem already called effective_itemtype. So we can just use $item->{itype} here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #11)
(In reply to Jonathan Druart from comment #8)
Created attachment 66486 [details] [review] [review] Bug 19004: build_object takes care of existing data
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
It works, but it will trigger a "unique violation" warning from build. Which is valid since build should create data not find it. (I once proposed a flag to suppress this warning, but it was not welcomed.)
It does not work: # Subtest: add_enrolment_fee_if_needed 1..4 Violation of unique constraint in Category at /usr/share/koha/masterclone/t/lib/ TestBuilder.pm line 233. # No tests run! not ok 14 - No tests run for subtest "add_enrolment_fee_if_needed" # Failed test 'No tests run for subtest "add_enrolment_fee_if_needed"' # at t/db_dependent/Koha/Patrons.t line 426. Can't call method "enrolmentfee" on an undefined value at t/db_dependent/Koha/Pa trons.t line 381. # Looks like your test exited with 255 just after 14. Obsoleting this patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66486|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66037|0 |1 is obsolete| | Attachment #66038|0 |1 is obsolete| | Attachment #66546|0 |1 is obsolete| | --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 66721 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66721&action=edit Bug 19004: Patrons.t should create its own data for enrollment fees. If the patron categories J, K, YA would not exist, Patrons.t would fail. Test plan: [1] Remove one of these patron categories. [2] Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 66722 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66722&action=edit Bug 19004: Adjust AddReturn for retrieving item type In the regular situation, you can get itemtype via biblio and then biblioitem as well as via biblioitem (at least when item-level_itypes is set to biblio). But since Koha unfortunately defined two relations in item, one for biblioitemnumber (the good one) and one for biblionumber (redundant), TestBuilder (correctly) builds one biblioitem and two biblios. If you item-level_itypes to biblio record, this will result in failing tests when calling AddReturn (in this case Koha/Patrons.t). It will crash on: Can't call method "itemtype" on an undefined value at C4/Circulation.pm line 1826. Cause: AddReturn goes via the biblionumber to biblio and than to biblioitems, and it does not find a biblioitem. (Not a fault from TestBuilder but a database design problem.) This patch makes a small change in AddReturn to retrieve the itemtype via biblioitem. It actually is a shorter road than items->biblio->biblioitems. Note: I do not test the Biblioitems->find call, since we already checked the GetItem call before and we have a foreign key constraint. I did not call $item->effective_itemtype since we still use GetItem; this could be done later. Adjusted Circulation/Returns.t too: If we add an item with TestBuilder and we called AddBiblio before, we should link biblioitemnumber as well. Test plan: [1] Do not apply this patch yet. [2] Set item-level_itypes to biblio record. [3] Run t/db_dependent/Koha/Patrons.t. (It should fail.) [4] Apply this patch. [5] Run t/db_dependent/Koha/Patrons.t again. [6] Run t/db_dependent/Circulation/Returns.t [7] Git grep on AddReturn and run a few other tests calling it. Note: Bugs 19070/19071 address three tests that call AddReturn too. [8] In the interface, check in a book. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Note: Bugs 19070 and 19071 are already pushed. The command in comment #4 has all the tests successful. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 66723 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66723&action=edit Bug 19004: [QA Follow-up] No need to check item-level_itypes again As Jonathan pointed out, GetItem already called effective_itemtype. So we can just use $item->{itype} here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Note to myself: - my $item_level_itypes = C4::Context->preference("item-level_itypes"); - my $biblio = $item_level_itypes ? undef : Koha::Biblios->find( $item->{ biblionumber } ); # don't get bib data unless we need it - my $itemtype = $item_level_itypes ? $item->{itype} : $biblio->biblioitem->itemtype; + my $itemtype = $item->{itype}; # GetItem called effective_itemtype This is not equal, but the previous code was buggy. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolin.somers@biblibre.co | |m --- Comment #20 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.05.x, will be in 17.05.05. effective_itemtype method is awsome, it should be used everywhere. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19004 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #21 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- These patches have been pushed to 16.11.x and will be in 16.11.13. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org