Description
Pedro Amorim
2023-10-31 11:53:01 UTC
Created attachment 158112 [details] [review] Bug 35197: Add additional_fields REST API endpoint Test plan: 1) Apply patch, restart plack 'koha-plack --restart kohadev' 2) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice its empty 3) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqbasket and add a new additional field 4) Do step 2) again - Notice the newly created additional field is there 5) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqinvoices and add a new additional field for invoices 6) Do step 2) again - Notice both additional fields are there 7) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice only the additional field for aqinvoices is listed Created attachment 158511 [details] [review] Bug 35197: Add additional_fields REST API endpoint Test plan: 1) Apply patch, restart plack 'koha-plack --restart kohadev' 2) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice its empty 3) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqbasket and add a new additional field 4) Do step 2) again - Notice the newly created additional field is there 5) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqinvoices and add a new additional field for invoices 6) Do step 2) again - Notice both additional fields are there 7) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice only the additional field for aqinvoices is listed Created attachment 158512 [details] [review] Bug 35197: Add tests prove t/db_dependent/api/v1/additional_fields.t Created attachment 158514 [details] [review] Bug 35197: Add additional_fields REST API endpoint Test plan: 1) Apply patch, restart plack 'koha-plack --restart kohadev' 2) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice its empty 3) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqbasket and add a new additional field 4) Do step 2) again - Notice the newly created additional field is there 5) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqinvoices and add a new additional field for invoices 6) Do step 2) again - Notice both additional fields are there 7) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice only the additional field for aqinvoices is listed Created attachment 158515 [details] [review] Bug 35197: Add tests prove t/db_dependent/api/v1/additional_fields.t Created attachment 158577 [details] [review] Bug 35197: Add additional_fields REST API endpoint Test plan: 1) Apply patch, restart plack 'koha-plack --restart kohadev' 2) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice its empty 3) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqbasket and add a new additional field 4) Do step 2) again - Notice the newly created additional field is there 5) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqinvoices and add a new additional field for invoices 6) Do step 2) again - Notice both additional fields are there 7) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice only the additional field for aqinvoices is listed Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 158578 [details] [review] Bug 35197: Add tests prove t/db_dependent/api/v1/additional_fields.t Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 159648 [details] [review] Bug 35197: Add additional_fields REST API endpoint Test plan: 1) Apply patch, restart plack 'koha-plack --restart kohadev' 2) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice its empty 3) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqbasket and add a new additional field 4) Do step 2) again - Notice the newly created additional field is there 5) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqinvoices and add a new additional field for invoices 6) Do step 2) again - Notice both additional fields are there 7) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice only the additional field for aqinvoices is listed Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 159649 [details] [review] Bug 35197: Add tests prove t/db_dependent/api/v1/additional_fields.t Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 159650 [details] [review] Bug 35197: Field name consistency This patch adds a to_api_mapping and updates the yaml definitions to be ensure we are consistent with modern best practice for our api field names. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I updated the mappings to be more consistent with the communities agreed guidelines.. though it raised lots of little questions for me.. I'm wondering how often our embeded extended_attributes are going to really look like the spec here. Sending Tomas's way for final QA Ping Tomas... This is now the oldest bug waiting for QA according to the dashboard - can we please have QA? It also blocks a real nice enh for the ERM module that would be shiny in our release notes :) Partial QA (not sure enough on the functional side): Magic number here for the permission flag: > my $librarian = $builder->build_object( > { > class => 'Koha::Patrons', > value => { flags => 2**13 } > } > ); --- https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER1.3.2:_required > SWAGGER1.3.2: required > All resources should have a list of required fields specified Is it just that nullable fields should be defined with "null" in the type list? If so, it's ok. --- That's quite sus ↓↓↓↓↓ ↓↓↓↓↓ > # Filtering works, two agreements sharing vendor_id > $t->get_ok( "//$userid:$password@/api/v1/additional_fields?tablename=" . $additional_field->tablename ) > ->status_is(200)->json_is( [ $additional_field->to_api, $another_additional_field->to_api ] ); --- It seems the `503:` section isn't necessary. After putting Koha in maintenance mode: `UPDATE systempreferences SET value = 1 WHERE variable = "version";` And removing the section and restarting. I still get the expected {"error":"System is under maintenance."} and the response 503 code. Sabotage confirmed that if I break that file and restart, it's taken into account. So it's not that my removal wasn't taken into account. Same with `500: ` section. I put a `die;` in Koha/REST/V1/AdditionalFields.pm#list And I still get {"errors":[{"message":"Internal Server Error.","path":"\/"}],"status":500} Looks like the same for 400 and 403, tests still pass after removing them. All these are in many other API routes. Maybe they are useful or maybe we have been adding dead API code for a while. There is no 404 case, it returns [ ] if tablename doesn't match an existing one. Leaving that info for someone QAing the functional side to tell if that's expected here. --- The param tablename should be table_name I think. That's what is shown from the examples here: https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER4.2:_Using_query_parameters_to_find_the_required_objects They diverge from the DB and for city_id, it's not for terminology reason, it seems to be just to follow snake_case. And the JSON of the response has the key `table_name` --- Otherwise all seems conformant to the guidelines. --- Is the enhancement sponsored BTW? (QA script asks this now) Created attachment 165726 [details] [review] Bug 35197: (QA follow-up): Update tablename prove t/db_dependent/api/v1/additional_fields.t Created attachment 165727 [details] [review] Bug 35197: (QA follow-up): Fix pasted comment Created attachment 165728 [details] [review] Bug 35197: (QA follow-up): Fix permissions specification Sponsored-by: UKHSA - UK Health Security Agency Sponsored-by: PTFS Europe Ltd Thanks for the review, Victor. I believe my follow-ups address the issues you mentioned. Additional comments: I believe sub under at REST/V1/Auth.pm handles the use cases you mentioned, this is why removing e.g. HTTP code 400 from the additional_fields route spec specifically does not prevent a 400 response from that specific route, as that is "called before every request to API" as in the sub description. I believe we add this "dead API code" for every new endpoint for openAPI documentation purposes i.e. https://api.koha-community.org/ The above is only my interpretation of it, Martin and/or Tomas will be better suited to confirm or deny it. Looking today. Created attachment 165769 [details] [review] Bug 35197: (QA follow-up) Add 'additional_fields' tag description in docs Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 165780 [details] [review] Bug 35197: Rename to 'Extended attribute types' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Hi all, I invested a fair amount of time today on this, and I'm not sure how to move it forward without this follow-ups. I submitted a follow-up that explains my thoughts on how this should be done. It is not 100% polished (e.g. we should find a better place for the mapping, etc). But it implements it as I would do it on a first attempt (i.e. no POST or PUT so reverse mapping oddities can be handled later as needed). Tests pass and all. I want to hear from Martin and Pedro (and Jonathan if he's back). I wouldn't like to block this, but I think we need to consider long term consequences on what we decide here. And my follow-up aligns it with the current codebase. (In reply to Victor Grousset/tuxayo from comment #16) > There is no 404 case, it returns [ ] if tablename doesn't match an existing > one. This is a plural endpoint. Meaning it is expected to return an array. An empty one if the query returns no results. With my follow-up, the (renamed to resource_type) table_name attribute will get its values validated beforehand, as Pedro said. So a 400 will get returned if the request doesn't conform to the spec. (In reply to Pedro Amorim from comment #20) > Thanks for the review, Victor. I believe my follow-ups address the issues > you mentioned. Confirmed :) After rereading, it seem to comply with coding guidelines. > Additional comments: > I believe sub under at REST/V1/Auth.pm handles the use cases you mentioned, > this is why removing e.g. HTTP code 400 from the additional_fields route > spec specifically does not prevent a 400 response from that specific route, > as that is "called before every request to API" as in the sub description. > I believe we add this "dead API code" for every new endpoint for openAPI > documentation purposes i.e. https://api.koha-community.org/ Good point, we can see that as documentation. (In reply to Tomás Cohen Arazi from comment #25) > This is a plural endpoint. Meaning it is expected to return an array. An > empty one if the query returns no results. Great, the rule is as simple as that. Created attachment 168057 [details] [review] Bug 35197: Add additional_fields REST API endpoint Test plan: 1) Apply patch, restart plack 'koha-plack --restart kohadev' 2) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice its empty 3) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqbasket and add a new additional field 4) Do step 2) again - Notice the newly created additional field is there 5) Visit /cgi-bin/koha/admin/additional-fields.pl?tablename=aqinvoices and add a new additional field for invoices 6) Do step 2) again - Notice both additional fields are there 7) Visit /api/v1/additional_fields?tablename=aqinvoices - Notice only the additional field for aqinvoices is listed Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168058 [details] [review] Bug 35197: Add tests prove t/db_dependent/api/v1/additional_fields.t Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168059 [details] [review] Bug 35197: Field name consistency This patch adds a to_api_mapping and updates the yaml definitions to be ensure we are consistent with modern best practice for our api field names. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168060 [details] [review] Bug 35197: (QA follow-up): Update tablename prove t/db_dependent/api/v1/additional_fields.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168061 [details] [review] Bug 35197: (QA follow-up): Fix pasted comment Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168062 [details] [review] Bug 35197: (QA follow-up): Fix permissions specification Sponsored-by: UKHSA - UK Health Security Agency Sponsored-by: PTFS Europe Ltd Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168063 [details] [review] Bug 35197: (QA follow-up) Add 'additional_fields' tag description in docs Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168064 [details] [review] Bug 35197: Rename to 'Extended attribute types' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168065 [details] [review] Bug 35197: (QA follow-up) Add note about intentions This is the beggining of a wider series of developments aimed at retifying overlapping features. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Passing QA here.. but we do need to ensure we follow through on our plans to merge these features. (In reply to Martin Renvoize from comment #36) > Passing QA here.. but we do need to ensure we follow through on our plans > to merge these features. Please file follow-ups or link to the bugs for the merge of features. Pushed for 24.11! Well done everyone, thank you! Enhancement will not be backported to the 24.05.x series We don't need the additional_work_needed here anymore do we? Any follow-up work should be submitted and discussed in bug 37180 correct? |