Outside of the UI, the not for loan status is coded to fallback to biblio level not for loan settings if the item is found to be loanable at the item level. We should respect that in the API too.
As discussed on bug 29105 I opted instead to always add an effective_not_for_loan_status field rather than outright replace it... But, I'm really not sure about this approach.. it feels like we're seeking internal processes a lot here.
Created attachment 137507 [details] [review] Bug 31128: Unit tests This patch adds unit tests for the new effective_not_for_loan_status which I add to the items api responses in this patchset. Test plan 1) Run the unit test without applying the next commit, it should fail 2) RUn the test again after applying the next commit, it should pass
Created attachment 137508 [details] [review] Bug 31128: Add effective_not_for_loan_status into items response
Created attachment 137574 [details] [review] Bug 31128: Unit tests This patch adds unit tests for the new effective_not_for_loan_status which I add to the items api responses in this patchset. Test plan 1) Run the unit test without applying the next commit, it should fail 2) RUn the test again after applying the next commit, it should pass
Created attachment 137575 [details] [review] Bug 31128: Add effective_not_for_loan_status into items response
Created attachment 137576 [details] [review] Bug 31128: Unit tests This patch adds unit tests for the new effective_not_for_loan_status which I add to the items api responses in this patchset. Test plan 1) Run the unit test without applying the next commit, it should fail 2) RUn the test again after applying the next commit, it should pass Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 137577 [details] [review] Bug 31128: Add effective_not_for_loan_status into items response Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
> ->status_is( 200, 'SWAGGER3.2.2' ) How can we have Swagger version hardcoded in tests and it still works? Searching for "SWAGGER" shows multiple versions allover the tests and yet it still work :o
Created attachment 137593 [details] [review] Bug 31128: Unit tests This patch adds unit tests for the new effective_not_for_loan_status which I add to the items api responses in this patchset. Test plan 1) Run the unit test without applying the next commit, it should fail 2) RUn the test again after applying the next commit, it should pass Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137594 [details] [review] Bug 31128: Add effective_not_for_loan_status into items response Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com>
The bit you highlight Victor is actually just a comment rather than test condition. If I'm honest I'm not sure why that was used as the comment in the first place, I just used the president set in the rest of the test.
> The bit you highlight Victor is actually just a comment rather than test condition. Ah, it's just string that pops when the test fails.
Created attachment 138023 [details] [review] Bug 31128: Unit tests This patch adds unit tests for the new effective_not_for_loan_status which I add to the items api responses in this patchset. Test plan 1) Run the unit test without applying the next commit, it should fail 2) RUn the test again after applying the next commit, it should pass Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138024 [details] [review] Bug 31128: Add effective_not_for_loan_status into items response Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Pushed to master for 22.11. Nice work everyone, thanks!
In 22.11 getting 500 errors with the message {"message":"Expected integer - got null.","path":"\/body\/0\/effective_not_for_loan_status"} Going to keep exploring...
(In reply to David Cook from comment #16) > In 22.11 getting 500 errors with the message > {"message":"Expected integer - got > null.","path":"\/body\/0\/effective_not_for_loan_status"} > > Going to keep exploring... Specfically when I'm going to /api/v1/public/biblios/16/items
Working fine in koha-testing-docker though so maybe something else is going on... :|
Yep never mind me. Looks like something broken in an instance rather than the Koha version... probably a red herring error message...
(In reply to David Cook from comment #19) > Yep never mind me. Looks like something broken in an instance rather than > the Koha version... probably a red herring error message... Looks like a number of item types had NULL instead of 0 in the itemtypes table. Ahhhhhhhh.
(In reply to Victor Grousset/tuxayo from comment #8) > > ->status_is( 200, 'SWAGGER3.2.2' ) > > How can we have Swagger version hardcoded in tests and it still works? > Searching for "SWAGGER" shows multiple versions allover the tests and yet it > still work :o We put a reference to the API coding guidelines described behavior we are testing. So 3.2.2 is this: https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER3.2.2_GET
(In reply to David Cook from comment #16) > In 22.11 getting 500 errors with the message > {"message":"Expected integer - got > null.","path":"\/body\/0\/effective_not_for_loan_status"} > > Going to keep exploring... We are hitting this in production and I filed bug 35167 to discuss it on its own place. Please join me there.
(In reply to Tomás Cohen Arazi from comment #22) > (In reply to David Cook from comment #16) > > In 22.11 getting 500 errors with the message > > {"message":"Expected integer - got > > null.","path":"\/body\/0\/effective_not_for_loan_status"} > > > > Going to keep exploring... > > We are hitting this in production and I filed bug 35167 to discuss it on its > own place. Please join me there. Seeing it too.