Description
Tomás Cohen Arazi (tcohen)
2021-05-18 11:50:38 UTC
Created attachment 121105 [details] [review] Bug 28369: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 121106 [details] [review] Bug 28369: Set additionalProperties: false and fix things This patch performs a cleanup on the holds routes: - It adds additionalProperties: false to all missing routes - It syncs the spec with the guidelines - additionalProperties: false highlighted some attributes read in the controlled weren't declared on the spec, adding them - PUT /holds/:hold_id is moved to PATCH, as it makes more sense and gathered opinions lean towards that To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Lots of things are wrong with the API 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Could we please add first a deprecation notice and create the PATCH route in addition to the PUT instead of simply renaming the PUT? We have already code using this API (at least https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/ILS/Driver/KohaRest.php) and I'm thinking there might also other people using this API we are not aware of, so things might unexpectedly break on the next update if we simply remove the PUT method without any migration period. (In reply to Joonas Kylmälä from comment #3) > Could we please add first a deprecation notice and create the PATCH route in > addition to the PUT instead of simply renaming the PUT? We have already code > using this API (at least > https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/ILS/ > Driver/KohaRest.php) and I'm thinking there might also other people using > this API we are not aware of, so things might unexpectedly break on the next > update if we simply remove the PUT method without any migration period. Yes! Can certainly do! But as I mentioned, the PUT route is using (in the code) different attribute names than the API spec, so you need to check if it isn't broken already (20.11). I couldn't find the references to the API routes on that PHP code. (In reply to Tomás Cohen Arazi from comment #4) > Yes! Can certainly do! > > But as I mentioned, the PUT route is using (in the code) different attribute > names than the API spec, so you need to check if it isn't broken already > (20.11). I couldn't find the references to the API routes on that PHP code. The code is in the function changePickupLocation. The code in changePickupLocation uses the attribute names that work (instead of broken ones). (In reply to Joonas Kylmälä from comment #5) > (In reply to Tomás Cohen Arazi from comment #4) > > Yes! Can certainly do! > > > > But as I mentioned, the PUT route is using (in the code) different attribute > > names than the API spec, so you need to check if it isn't broken already > > (20.11). I couldn't find the references to the API routes on that PHP code. > > The code is in the function changePickupLocation. The code in > changePickupLocation uses the attribute names that work (instead of broken > ones). Ok, I'm fine with fixing the spec and adding a deprecation warning, and adding PATCH as a duplicate for PUT. Do you agree? Created attachment 121119 [details] [review] Bug 28369: (QA follow-up) Restore PUT with deprecation message This patch restores the PUT route, but adding a deprecation message. The controller method is the same, and relevant patch tests are duplicated but calling PUT, so it still gets tested. To test: 1. Apply this patch 2. Reload plack 3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route => SUCCESS: There's a deprecation wargning! 4. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 121131 [details] [review] Bug 28369: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 121132 [details] [review] Bug 28369: Set additionalProperties: false and fix things This patch performs a cleanup on the holds routes: - It adds additionalProperties: false to all missing routes - It syncs the spec with the guidelines - additionalProperties: false highlighted some attributes read in the controlled weren't declared on the spec, adding them - PUT /holds/:hold_id is moved to PATCH, as it makes more sense and gathered opinions lean towards that To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Lots of things are wrong with the API 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 121133 [details] [review] Bug 28369: (QA follow-up) Restore PUT with deprecation message This patch restores the PUT route, but adding a deprecation message. The controller method is the same, and relevant patch tests are duplicated but calling PUT, so it still gets tested. To test: 1. Apply this patch 2. Reload plack 3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route => SUCCESS: There's a deprecation wargning! 4. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> (In reply to Tomás Cohen Arazi from comment #6) > (In reply to Joonas Kylmälä from comment #5) > > (In reply to Tomás Cohen Arazi from comment #4) > > > Yes! Can certainly do! > > > > > > But as I mentioned, the PUT route is using (in the code) different attribute > > > names than the API spec, so you need to check if it isn't broken already > > > (20.11). I couldn't find the references to the API routes on that PHP code. > > > > The code is in the function changePickupLocation. The code in > > changePickupLocation uses the attribute names that work (instead of broken > > ones). > > Ok, I'm fine with fixing the spec and adding a deprecation warning, and > adding PATCH as a duplicate for PUT. > > Do you agree? Works for me (I'm responsible for the KohaRest PHP module). As a side note, it would be really nice if PATCH was able to modify priority, suspension and suspension end date as well as pickup location. From an API user's standpoint modifying several attributes of a hold is typically a single operation, and having to make several API calls makes using it convoluted. (In reply to Ere Maijala from comment #11) > (In reply to Tomás Cohen Arazi from comment #6) > > (In reply to Joonas Kylmälä from comment #5) > > > (In reply to Tomás Cohen Arazi from comment #4) > > > > Yes! Can certainly do! > > > > > > > > But as I mentioned, the PUT route is using (in the code) different attribute > > > > names than the API spec, so you need to check if it isn't broken already > > > > (20.11). I couldn't find the references to the API routes on that PHP code. > > > > > > The code is in the function changePickupLocation. The code in > > > changePickupLocation uses the attribute names that work (instead of broken > > > ones). > > > > Ok, I'm fine with fixing the spec and adding a deprecation warning, and > > adding PATCH as a duplicate for PUT. > > > > Do you agree? > > Works for me (I'm responsible for the KohaRest PHP module). As a side note, > it would be really nice if PATCH was able to modify priority, suspension and > suspension end date as well as pickup location. From an API user's > standpoint modifying several attributes of a hold is typically a single > operation, and having to make several API calls makes using it convoluted. Luckily, it works like that! This all looks good to me, but I'm not one of the existing consumers of this API endpoint, so I think it worth getting a final QA signoff from Joonas or Ere.. I'll add my SO lines to varify I've tested and it works as expected from my perspective however. Created attachment 121233 [details] [review] Bug 28369: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121234 [details] [review] Bug 28369: Set additionalProperties: false and fix things This patch performs a cleanup on the holds routes: - It adds additionalProperties: false to all missing routes - It syncs the spec with the guidelines - additionalProperties: false highlighted some attributes read in the controlled weren't declared on the spec, adding them - PUT /holds/:hold_id is moved to PATCH, as it makes more sense and gathered opinions lean towards that To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Lots of things are wrong with the API 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: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121235 [details] [review] Bug 28369: (QA follow-up) Restore PUT with deprecation message This patch restores the PUT route, but adding a deprecation message. The controller method is the same, and relevant patch tests are duplicated but calling PUT, so it still gets tested. To test: 1. Apply this patch 2. Reload plack 3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route => SUCCESS: There's a deprecation wargning! 4. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => 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: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121236 [details] [review] Bug 28369: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 121237 [details] [review] Bug 28369: Set additionalProperties: false and fix things This patch performs a cleanup on the holds routes: - It adds additionalProperties: false to all missing routes - It syncs the spec with the guidelines - additionalProperties: false highlighted some attributes read in the controlled weren't declared on the spec, adding them - PUT /holds/:hold_id is moved to PATCH, as it makes more sense and gathered opinions lean towards that To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Lots of things are wrong with the API 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: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 121238 [details] [review] Bug 28369: (QA follow-up) Restore PUT with deprecation message This patch restores the PUT route, but adding a deprecation message. The controller method is the same, and relevant patch tests are duplicated but calling PUT, so it still gets tested. To test: 1. Apply this patch 2. Reload plack 3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route => SUCCESS: There's a deprecation wargning! 4. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => 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: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Pushed to master for 21.05, thanks to everybody involved! Depends on Bug 28272 not in 20.11.x |