Bug 35197 - Expose additional_field definitions through REST API
Summary: Expose additional_field definitions through REST API
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact: Tomás Cohen Arazi
URL:
Keywords: RM_priority
Depends on: 35190
Blocks: 35044
  Show dependency treegraph
 
Reported: 2023-10-31 11:53 UTC by Pedro Amorim
Modified: 2024-04-29 17:51 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35197: Add additional_fields REST API endpoint (6.62 KB, patch)
2023-10-31 11:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: Add additional_fields REST API endpoint (6.64 KB, patch)
2023-11-06 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: Add tests (4.35 KB, patch)
2023-11-06 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: Add additional_fields REST API endpoint (6.66 KB, patch)
2023-11-06 12:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: Add tests (4.35 KB, patch)
2023-11-06 12:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: Add additional_fields REST API endpoint (6.72 KB, patch)
2023-11-06 23:18 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35197: Add tests (4.41 KB, patch)
2023-11-06 23:18 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35197: Add additional_fields REST API endpoint (6.79 KB, patch)
2023-12-07 13:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35197: Add tests (4.47 KB, patch)
2023-12-07 13:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35197: Field name consistency (2.67 KB, patch)
2023-12-07 13:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35197: (QA follow-up): Update tablename (2.16 KB, patch)
2024-04-29 11:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: (QA follow-up): Fix pasted comment (1003 bytes, patch)
2024-04-29 11:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: (QA follow-up): Fix permissions specification (1.48 KB, patch)
2024-04-29 11:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35197: (QA follow-up) Add 'additional_fields' tag description in docs (972 bytes, patch)
2024-04-29 17:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35197: Rename to 'Extended attribute types' (12.43 KB, patch)
2024-04-29 17:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-10-31 11:53:01 UTC

    
Comment 1 Pedro Amorim 2023-10-31 11:56:26 UTC Comment hidden (obsolete)
Comment 2 Pedro Amorim 2023-11-06 11:51:08 UTC Comment hidden (obsolete)
Comment 3 Pedro Amorim 2023-11-06 11:51:11 UTC Comment hidden (obsolete)
Comment 4 Pedro Amorim 2023-11-06 12:18:14 UTC
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
Comment 5 Pedro Amorim 2023-11-06 12:18:17 UTC
Created attachment 158515 [details] [review]
Bug 35197: Add tests

prove t/db_dependent/api/v1/additional_fields.t
Comment 6 Pedro Amorim 2023-11-06 12:19:14 UTC
Rebased on top of bug 35190
Comment 7 Lucas Gass 2023-11-06 23:18:48 UTC
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>
Comment 8 Lucas Gass 2023-11-06 23:18:50 UTC
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>
Comment 9 Martin Renvoize 2023-12-07 13:24:21 UTC
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>
Comment 10 Martin Renvoize 2023-12-07 13:24:24 UTC
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>
Comment 11 Martin Renvoize 2023-12-07 13:24:26 UTC
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>
Comment 12 Martin Renvoize 2023-12-07 13:26:00 UTC
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
Comment 13 Martin Renvoize 2024-02-28 22:39:40 UTC
Ping Tomas...
Comment 14 Katrin Fischer 2024-04-26 20:24:52 UTC
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 :)
Comment 15 Victor Grousset/tuxayo 2024-04-28 04:04:03 UTC
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.
Comment 16 Victor Grousset/tuxayo 2024-04-28 23:28:00 UTC
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)
Comment 17 Pedro Amorim 2024-04-29 11:15:08 UTC
Created attachment 165726 [details] [review]
Bug 35197: (QA follow-up): Update tablename

prove t/db_dependent/api/v1/additional_fields.t
Comment 18 Pedro Amorim 2024-04-29 11:15:11 UTC
Created attachment 165727 [details] [review]
Bug 35197: (QA follow-up): Fix pasted comment
Comment 19 Pedro Amorim 2024-04-29 11:15:13 UTC
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
Comment 20 Pedro Amorim 2024-04-29 11:18:17 UTC
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.
Comment 21 Tomás Cohen Arazi 2024-04-29 11:31:06 UTC
Looking today.
Comment 22 Tomás Cohen Arazi 2024-04-29 17:16:34 UTC
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>
Comment 23 Tomás Cohen Arazi 2024-04-29 17:40:52 UTC
Created attachment 165780 [details] [review]
Bug 35197: Rename to 'Extended attribute types'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2024-04-29 17:44:29 UTC
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.
Comment 25 Tomás Cohen Arazi 2024-04-29 17:51:08 UTC
(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.