Bug 31128 - Add effective_not_for_loan_status to API item objects
Summary: Add effective_not_for_loan_status to API item objects
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 29105
Blocks: 35167
  Show dependency treegraph
 
Reported: 2022-07-11 12:17 UTC by Martin Renvoize
Modified: 2023-11-06 09:50 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31128: Unit tests (3.63 KB, patch)
2022-07-11 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31128: Add effective_not_for_loan_status into items response (1.48 KB, patch)
2022-07-11 12:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31128: Unit tests (3.63 KB, patch)
2022-07-11 16:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31128: Add effective_not_for_loan_status into items response (730 bytes, patch)
2022-07-11 16:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31128: Unit tests (3.69 KB, patch)
2022-07-11 16:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31128: Add effective_not_for_loan_status into items response (1.43 KB, patch)
2022-07-11 16:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31128: Unit tests (3.74 KB, patch)
2022-07-11 22:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 31128: Add effective_not_for_loan_status into items response (1.47 KB, patch)
2022-07-11 22:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 31128: Unit tests (3.79 KB, patch)
2022-07-22 13:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31128: Add effective_not_for_loan_status into items response (1.53 KB, patch)
2022-07-22 13:29 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-07-11 12:17:11 UTC
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.
Comment 1 Martin Renvoize 2022-07-11 12:18:29 UTC
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.
Comment 2 Martin Renvoize 2022-07-11 12:47:58 UTC
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
Comment 3 Martin Renvoize 2022-07-11 12:48:04 UTC
Created attachment 137508 [details] [review]
Bug 31128: Add effective_not_for_loan_status into items response
Comment 4 Nick Clemens 2022-07-11 16:37:09 UTC
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
Comment 5 Nick Clemens 2022-07-11 16:37:14 UTC
Created attachment 137575 [details] [review]
Bug 31128: Add effective_not_for_loan_status into items response
Comment 6 Nick Clemens 2022-07-11 16:39:31 UTC
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>
Comment 7 Nick Clemens 2022-07-11 16:39:34 UTC
Created attachment 137577 [details] [review]
Bug 31128: Add effective_not_for_loan_status into items response

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Victor Grousset/tuxayo 2022-07-11 22:13:07 UTC
> ->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
Comment 9 David Nind 2022-07-11 22:59:08 UTC
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>
Comment 10 David Nind 2022-07-11 22:59:12 UTC
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>
Comment 11 Martin Renvoize 2022-07-12 07:20:47 UTC
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.
Comment 12 Victor Grousset/tuxayo 2022-07-13 23:07:18 UTC
> 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.
Comment 13 Katrin Fischer 2022-07-22 13:29:19 UTC
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>
Comment 14 Katrin Fischer 2022-07-22 13:29:24 UTC
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>
Comment 15 Tomás Cohen Arazi 2022-07-22 15:29:42 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 16 David Cook 2023-05-16 02:09:05 UTC
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...
Comment 17 David Cook 2023-05-16 02:11:09 UTC
(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
Comment 18 David Cook 2023-05-16 02:18:55 UTC
Working fine in koha-testing-docker though so maybe something else is going on... :|
Comment 19 David Cook 2023-05-16 02:21:17 UTC
Yep never mind me. Looks like something broken in an instance rather than the Koha version... probably a red herring error message...
Comment 20 David Cook 2023-05-16 02:25:24 UTC
(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.
Comment 21 Tomás Cohen Arazi 2023-10-26 14:34:30 UTC
(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
Comment 22 Tomás Cohen Arazi 2023-10-26 14:35:28 UTC
(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.
Comment 23 Marcel de Rooy 2023-11-06 09:50:18 UTC
(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.