I found `POST /biblios` is not returning it. Opening this bug to track and fix such problems.
Created attachment 176832 [details] [review] Bug 38929: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 176833 [details] [review] Bug 38929: Make POST endpoints return the `Location` header This patch makes the POST endpoints missing the Location header return it. Some endpoints I skipped: * /patrons/account/credits (doesn't have a GET route to point to) * /patrons/account/debits (doesn't have a GET route to point to) To test: 1. Apply the regression tests 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/authorities.t \ t/db_dependent/api/v1/biblios.t \ t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/api/v1/holds.t \ t/db_dependent/api/v1/ill_batchstatuses.t \ t/db_dependent/api/v1/import_batch_profiles.t \ t/db_dependent/api/v1/transfer_limits.t => FAIL: Tests fail! The endpoints don't return the header! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 176834 [details] [review] Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests The wiki now uses `REST*` for the rule names instead of `SWAGGER*` some tests have their descriptions correct, but most don't. This is a follow-up to an editorial decision made on the Wiki (probably at a dev meeting too, I don't remember). QA tests will probably not pass, but I refrain from tidying it because there's a major tidy on its way to main. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I hadn't thought about this use case but it makes sense to return the Location header
Do we want a test to enforce this? Should we ALWAYS find $c->res->headers->location before the render call?
(In reply to Jonathan Druart from comment #5) > Do we want a test to enforce this? > > Should we ALWAYS find $c->res->headers->location before the render call? Well, we have a long time voted coding guideline https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST3.4.1_POST
Created attachment 176901 [details] [review] Bug 38929: Add xt test to enforce REST3.4.1 POST
(In reply to Tomás Cohen Arazi (tcohen) from comment #6) > (In reply to Jonathan Druart from comment #5) > > Do we want a test to enforce this? > > > > Should we ALWAYS find $c->res->headers->location before the render call? > > Well, we have a long time voted coding guideline > > https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST3.4.1_POST Yes, and it's not enough. Here is a test to enforce it, we have 3 failures. # Failed test 'Koha/REST/V1/Auth/Password.pm does not contain the location header' # at xt/api.t line 186. # Failed test 'Koha/REST/V1/Patrons/Account.pm does not contain the location header' # at xt/api.t line 186. # Failed test 'Koha/REST/V1/Preservation/WaitingList.pm does not contain the location header' # at xt/api.t line 186. # Looks like you failed 3 tests of 55.
(In reply to Jonathan Druart from comment #8) > (In reply to Tomás Cohen Arazi (tcohen) from comment #6) > > > > Well, we have a long time voted coding guideline > > > > https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST3.4.1_POST > > Yes, and it's not enough. Here is a test to enforce it, we have 3 failures. I know about them. Answering inline: > # Failed test 'Koha/REST/V1/Auth/Password.pm does not contain the > location header' > # at xt/api.t line 186. I'm not sure what location it should return. Maybe the same was used for the request. > # Failed test 'Koha/REST/V1/Patrons/Account.pm does not contain the > location header' > # at xt/api.t line 186. This is certainly handled by bug 38932. > # Failed test 'Koha/REST/V1/Preservation/WaitingList.pm does not > contain the location header' > # at xt/api.t line 186. > # Looks like you failed 3 tests of 55. A complex one. The POST receives a list of items. I'm also not sure on this one. Ideas?
(In reply to Tomás Cohen Arazi (tcohen) from comment #9) > (In reply to Jonathan Druart from comment #8) > > (In reply to Tomás Cohen Arazi (tcohen) from comment #6) > > # Failed test 'Koha/REST/V1/Auth/Password.pm does not contain the > > location header' > > # at xt/api.t line 186. > > I'm not sure what location it should return. Maybe the same was used for the > request. This one could be skip I guess. > > # Failed test 'Koha/REST/V1/Preservation/WaitingList.pm does not > > contain the location header' > > # at xt/api.t line 186. > > # Looks like you failed 3 tests of 55. > > A complex one. The POST receives a list of items. I'm also not sure on this > one. Ideas? Point to the first one? Of we skip this specific file from the test. Exceptions are not bad, if they prevent us from new missing occurrences.
Created attachment 176906 [details] [review] Bug 38929: Add 2 exceptions
Testing notes (using KTD): 1. xt tests now fail: prove xt/api.t 2. Other tests pass after pacthes applied: - Tests in step 2 of the test plan - All the API tests: prove t/db_dependent/api/v1/* prove xt/api.t xt/api.t .. 3/6 # Failed test 'Koha/REST/V1/Patrons/Account.pm does not contain the location header' # at xt/api.t line 193. # Looks like you failed 1 test of 55. # Failed test 'POST (201) have location header' # at xt/api.t line 196. # Looks like you failed 1 test of 6. xt/api.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/6 subtests Test Summary Report ------------------- xt/api.t (Wstat: 256 (exited 1) Tests: 6 Failed: 1) Failed test: 6 Non-zero exit status: 1 Files=1, Tests=6, 4 wallclock secs ( 0.01 usr 0.01 sys + 4.39 cusr 0.32 csys = 4.73 CPU) Result: FAIL
David, this failure should be fixed with the patches from bug 38932 applied.
@RM - Push along with bug 38932.
Created attachment 177068 [details] [review] Bug 38929: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 177069 [details] [review] Bug 38929: Make POST endpoints return the `Location` header This patch makes the POST endpoints missing the Location header return it. Some endpoints I skipped: * /patrons/account/credits (doesn't have a GET route to point to) * /patrons/account/debits (doesn't have a GET route to point to) To test: 1. Apply the regression tests 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/authorities.t \ t/db_dependent/api/v1/biblios.t \ t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/api/v1/holds.t \ t/db_dependent/api/v1/ill_batchstatuses.t \ t/db_dependent/api/v1/import_batch_profiles.t \ t/db_dependent/api/v1/transfer_limits.t => FAIL: Tests fail! The endpoints don't return the header! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 177070 [details] [review] Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests The wiki now uses `REST*` for the rule names instead of `SWAGGER*` some tests have their descriptions correct, but most don't. This is a follow-up to an editorial decision made on the Wiki (probably at a dev meeting too, I don't remember). QA tests will probably not pass, but I refrain from tidying it because there's a major tidy on its way to main. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 177071 [details] [review] Bug 38929: Add xt test to enforce REST3.4.1 POST Signed-off-by: David Nind <david@davidnind.com>
Created attachment 177072 [details] [review] Bug 38929: Add 2 exceptions Signed-off-by: David Nind <david@davidnind.com>
(In reply to Jonathan Druart from comment #13) > David, this failure should be fixed with the patches from bug 38932 applied. Thanks Jonathan! Testing notes (using KTD): 1. Apply 38926. 2. Apply 38932. 3. Run: yarn api:bundle 4. Apply 38929. 5. Restart everything: restart_all 6. Check that these tests pass (step 2 of the test plan): prove t/db_dependent/api/v1/authorities.t \ t/db_dependent/api/v1/biblios.t \ t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/api/v1/holds.t \ t/db_dependent/api/v1/ill_batchstatuses.t \ t/db_dependent/api/v1/import_batch_profiles.t \ t/db_dependent/api/v1/transfer_limits.t 7. Check that these tests pass: prove xt/api.t 8. Check that all the API tests pass: prove t/db_dependent/api/v1/*
Created attachment 177106 [details] [review] Bug 38929: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 177107 [details] [review] Bug 38929: Make POST endpoints return the `Location` header This patch makes the POST endpoints missing the Location header return it. Some endpoints I skipped: * /patrons/account/credits (doesn't have a GET route to point to) * /patrons/account/debits (doesn't have a GET route to point to) To test: 1. Apply the regression tests 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/authorities.t \ t/db_dependent/api/v1/biblios.t \ t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/api/v1/holds.t \ t/db_dependent/api/v1/ill_batchstatuses.t \ t/db_dependent/api/v1/import_batch_profiles.t \ t/db_dependent/api/v1/transfer_limits.t => FAIL: Tests fail! The endpoints don't return the header! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 177108 [details] [review] Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests The wiki now uses `REST*` for the rule names instead of `SWAGGER*` some tests have their descriptions correct, but most don't. This is a follow-up to an editorial decision made on the Wiki (probably at a dev meeting too, I don't remember). QA tests will probably not pass, but I refrain from tidying it because there's a major tidy on its way to main. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 177109 [details] [review] Bug 38929: Add xt test to enforce REST3.4.1 POST Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 177110 [details] [review] Bug 38929: Add 2 exceptions Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Please note: xt/api.t will fail without Bug 38932.
Pushed for 25.05! Well done everyone, thank you!
Nice work everyone! Pushed to 24.11.x for 24.11.04
Not backporting to 22.11.x as it wasn't backported to 24.05.x