items.stack is not included at /api/v1/items/ I think it should be
Created attachment 170299 [details] [review] Bug 37639: Add items.stack to items endpoint To test: 1. Apply patch and restart_all 2. Check the items endpoint for items.stack ( http://localhost:8081/api/v1/items/144 ) 3. The data is there, and correct.
Created attachment 170328 [details] [review] Bug 37639: Add items.stack to items endpoint To test: 1. Apply patch and restart_all, and yarn api:bundle 2. Check the items endpoint for items.stack ( http://localhost:8081/api/v1/items/144 ) 3. The data is there, and correct.
Created attachment 170329 [details] [review] Bug 37639: (follow-up) Remove stack from API mapping Shouldn't need to define stack in to_api_mapping because the name isn't changing.
Created attachment 170396 [details] [review] Bug 37639: Add items.stack to items endpoint To test: 1. Apply patch and restart_all, and yarn api:bundle 2. Check the items endpoint for items.stack ( http://localhost:8081/api/v1/items/144 ) 3. The data is there, and correct. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170397 [details] [review] Bug 37639: (follow-up) Remove stack from API mapping Shouldn't need to define stack in to_api_mapping because the name isn't changing. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Why is items.stack the only column in items that doesn't have a comment in kohastructure.pl?
(In reply to Paul Derscheid from comment #6) > Why is items.stack the only column in items that doesn't have a comment in > kohastructure.pl? It's a good question. There should be more info. on what it is. I had to ask around to find out. It is also one of the only ones without POD in Koha/Schema/Result/Item.pm
(In reply to Paul Derscheid from comment #6) > Why is items.stack the only column in items that doesn't have a comment in > kohastructure.pl? https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37653
Just feeling that it would be better to list it (skip 2nd patch). Also note: withdrawn => 'withdrawn', timestamp => 'timestamp', location => 'location', uri => 'uri', Is it possible to add a tiny test to prove the result of this change?
Created attachment 170458 [details] [review] Bug 37639: Add unit tests
> Is it possible to add a tiny test to prove the result of this change? I don't know how to write a test for my change only, so I added some for all the to_api_mappiings in Item.pm
OK.. we add it to the mapping... As people are confused about 'stack'.. should we not actually map it to something more meaningful for the API field? control_number perhaps?
(In reply to Martin Renvoize from comment #12) > OK.. we add it to the mapping... As people are confused about 'stack'.. > should we not actually map it to something more meaningful for the API field? > > control_number perhaps? I think control_number might also be a bit difficult as it reminds of the MARC 001 label. shelving_control_number maybe?
(In reply to Katrin Fischer from comment #13) > (In reply to Martin Renvoize from comment #12) > > OK.. we add it to the mapping... As people are confused about 'stack'.. > > should we not actually map it to something more meaningful for the API field? > > > > control_number perhaps? > > I think control_number might also be a bit difficult as it reminds of the > MARC 001 label. shelving_control_number maybe? I'm happy to change the patch. Is 'shelving_control_number' agreed upon?
I think Martin is not around for a few days. If you agree, maybe move on with it.
Sorry, but what exactly is this referring to?
Exposing items.stacks as shelving_control_number in the API (I believe).
Alright, I also think shelving_control_number sounds reasonable.
Sounds great to me.. it's a tiny ask but I think that would poke it into PQA for me [U+1F600]
Created attachment 170782 [details] [review] Bug 37639: Map items.stack as shelving_control_number in API
WARN t/db_dependent/api/v1/items.t WARN tidiness The file is less tidy than before (bad/messy lines before: 180, now: 225)
# Failed test 'items.stack is mapped as stack in to_api_mapping' # at /usr/share/koha/t/db_dependent/api/v1/items.t line 63. # got: 'shelving_control_number' # expected: 'stack' # Looks like you failed 1 test of 46. # Failed test 'SWAGGER3.2.2' # at /usr/share/koha/t/db_dependent/api/v1/items.t line 143. # got: '500' # expected: '200' # Looks like you planned 15 tests but ran 5. # Looks like you failed 1 test of 5 run.
(In reply to Marcel de Rooy from comment #21) > WARN t/db_dependent/api/v1/items.t > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 180, now: 225) My patchset doesn't touch these lines so I won't tidy.
Created attachment 172980 [details] [review] Bug 37639: Adjust tests
(In reply to Lucas Gass (lukeg) from comment #23) > (In reply to Marcel de Rooy from comment #21) > > WARN t/db_dependent/api/v1/items.t > > WARN tidiness > > The file is less tidy than before (bad/messy lines before: > > 180, now: 225) > > My patchset doesn't touch these lines so I won't tidy. What do you mean with these lines btw? Qa tools counts messy lines and tell us that your patch introduces 225-180 = 45 messy lines. The second patch adds 60+ lines to the test file. I suspect that all lines that are considered too long have been counted. So why are you stating that you did not touch these lines?
Created attachment 172988 [details] [review] Bug 37639: Adjust tests
(In reply to Marcel de Rooy from comment #25) > (In reply to Lucas Gass (lukeg) from comment #23) > > (In reply to Marcel de Rooy from comment #21) > > > WARN t/db_dependent/api/v1/items.t > > > WARN tidiness > > > The file is less tidy than before (bad/messy lines before: > > > 180, now: 225) > > > > My patchset doesn't touch these lines so I won't tidy. > > What do you mean with these lines btw? > Qa tools counts messy lines and tell us that your patch introduces 225-180 = > 45 messy lines. > The second patch adds 60+ lines to the test file. I suspect that all lines > that are considered too long have been counted. > > So why are you stating that you did not touch these lines? I tidied them.
Looking here
(In reply to Marcel de Rooy from comment #22) > # Failed test 'SWAGGER3.2.2' > # at /usr/share/koha/t/db_dependent/api/v1/items.t line 143. > # got: '500' > # expected: '200' > # Looks like you planned 15 tests but ran 5. > # Looks like you failed 1 test of 5 run. Thanks for tidying. This test still fails with me with new line numbers: # Failed test 'SWAGGER3.2.2' # at /usr/share/koha/t/db_dependent/api/v1/items.t line 191. # got: '500' # expected: '200' # Looks like you planned 15 tests but ran 5. # Looks like you failed 1 test of 5 run. Seeing warns in the log like: [WARN] OpenAPI >>> GET /api/v1/items [{"message":"Properties not allowed: shelving_control_number.","path":"\/body\/0"} etc
(In reply to Marcel de Rooy from comment #29) > (In reply to Marcel de Rooy from comment #22) > > > # Failed test 'SWAGGER3.2.2' > > # at /usr/share/koha/t/db_dependent/api/v1/items.t line 143. > > # got: '500' > > # expected: '200' > > # Looks like you planned 15 tests but ran 5. > > # Looks like you failed 1 test of 5 run. > > Thanks for tidying. > This test still fails with me with new line numbers: > > # Failed test 'SWAGGER3.2.2' > # at /usr/share/koha/t/db_dependent/api/v1/items.t line 191. > # got: '500' > # expected: '200' > # Looks like you planned 15 tests but ran 5. > # Looks like you failed 1 test of 5 run. > > Seeing warns in the log like: > [WARN] OpenAPI >>> GET /api/v1/items [{"message":"Properties not allowed: > shelving_control_number.","path":"\/body\/0"} > etc Hmmm, I cannot make this failure happen locally.
(In reply to Lucas Gass (lukeg) from comment #30) > (In reply to Marcel de Rooy from comment #29) > > (In reply to Marcel de Rooy from comment #22) > > > > > # Failed test 'SWAGGER3.2.2' > > > # at /usr/share/koha/t/db_dependent/api/v1/items.t line 143. > > > # got: '500' > > > # expected: '200' > > > # Looks like you planned 15 tests but ran 5. > > > # Looks like you failed 1 test of 5 run. > > > > Thanks for tidying. > > This test still fails with me with new line numbers: > > > > # Failed test 'SWAGGER3.2.2' > > # at /usr/share/koha/t/db_dependent/api/v1/items.t line 191. > > # got: '500' > > # expected: '200' > > # Looks like you planned 15 tests but ran 5. > > # Looks like you failed 1 test of 5 run. > > > > Seeing warns in the log like: > > [WARN] OpenAPI >>> GET /api/v1/items [{"message":"Properties not allowed: > > shelving_control_number.","path":"\/body\/0"} > > etc > > Hmmm, I cannot make this failure happen locally. Maybe someone else can check too ?
(In reply to Marcel de Rooy from comment #29) > This test still fails with me with new line numbers: > > # Failed test 'SWAGGER3.2.2' > # at /usr/share/koha/t/db_dependent/api/v1/items.t line 191. > # got: '500' > # expected: '200' > # Looks like you planned 15 tests but ran 5. > # Looks like you failed 1 test of 5 run. > > Seeing warns in the log like: > [WARN] OpenAPI >>> GET /api/v1/items [{"message":"Properties not allowed: > shelving_control_number.","path":"\/body\/0"} You were missing the API spec bundle step: ```shell yarn api:bundle ```
Lucas, I think there's been some miscommunication regarding this comment: (In reply to Marcel de Rooy from comment #9) > Just feeling that it would be better to list it (skip 2nd patch). > Also note: > withdrawn => 'withdrawn', > timestamp => 'timestamp', > location => 'location', > uri => 'uri', > > Is it possible to add a tiny test to prove the result of this change? We never add tests for `to_api_mapping()` and my interpretation of Marcel's comment is that he doubts we need those specific mappings (which are dated a few years ago). I think we need to remove the tests and squash the patches.
Created attachment 173615 [details] [review] Bug 37639: Add items.stack to items endpoint To test: 1. Apply patch and restart_all, and yarn api:bundle 2. Check the items endpoint for items.stack ( http://localhost:8081/api/v1/items/144 ) 3. The data is there, and correct. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 173616 [details] [review] Bug 37639: (follow-up) Remove redundant mappings from Koha::Item::to_api_mapping() Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed for 24.11! Well done everyone, thank you!