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!