Bug 33996 - Authority objects missing mapping
Summary: Authority objects missing mapping
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 31790
Blocks: 33971
  Show dependency treegraph
 
Reported: 2023-06-13 12:59 UTC by Tomás Cohen Arazi
Modified: 2023-07-18 11:58 UTC (History)
6 users (show)

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


Attachments
Bug 33996: Add missing authority objects mapping (9.14 KB, patch)
2023-06-13 13:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33996: Add missing authority objects mapping (9.18 KB, patch)
2023-06-14 15:03 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 33996: Add missing authority objects mapping (9.24 KB, patch)
2023-06-26 14:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33996: (QA follow-up) Fix mappings to match guidelines (1.18 KB, patch)
2023-06-26 14:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2023-06-13 12:59:47 UTC
Koha::Authority doesn't have a `to_api_mapping()` method. I noticed because the tests were using 'authid' but the PATH parameter is named 'authority_id'.

We need to fix this urgently.
Comment 1 Tomás Cohen Arazi 2023-06-13 13:15:55 UTC
Created attachment 152293 [details] [review]
Bug 33996: Add missing authority objects mapping

This patch adds a minimal mapping (i.e. some things get excluded from
the API objects for now). It makes some attributes consistent with the
current API names, and the rest can be added later when they become
needed and we discuss a proper name. But right now is a mess :-/

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/authorities.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Sam Lau 2023-06-14 15:03:48 UTC
Created attachment 152341 [details] [review]
Bug 33996: Add missing authority objects mapping

This patch adds a minimal mapping (i.e. some things get excluded from
the API objects for now). It makes some attributes consistent with the
current API names, and the rest can be added later when they become
needed and we discuss a proper name. But right now is a mess :-/

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/authorities.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 3 Jonathan Druart 2023-06-16 13:47:57 UTC
You were against "_on".

We have:
Koha/Authority.pm:        modification_time => 'updated_on',
Koha/Biblio/ItemGroup.pm:        updated_on   => 'modification_date'


api/v1/swagger/definitions/patron.yaml:  updated_on:
api/v1/swagger/definitions/return_claim.yaml:  updated_on:

Can we make a decision maybe?
Comment 4 Martin Renvoize 2023-06-26 14:17:20 UTC
Created attachment 152688 [details] [review]
Bug 33996: Add missing authority objects mapping

This patch adds a minimal mapping (i.e. some things get excluded from
the API objects for now). It makes some attributes consistent with the
current API names, and the rest can be added later when they become
needed and we discuss a proper name. But right now is a mess :-/

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/authorities.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2023-06-26 14:17:23 UTC
Created attachment 152689 [details] [review]
Bug 33996: (QA follow-up) Fix mappings to match guidelines

The date field guidelines are recorded on the wiki:
https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER1.3.4.1_date.2Fdatetime.2Ftimestamp_fields

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Tomás Cohen Arazi 2023-06-29 14:08:54 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 7 Jonathan Druart 2023-07-04 06:45:16 UTC
Should we fix the '*_on' occurrences on a separate bug report or is it too late now?
Comment 8 Martin Renvoize 2023-07-04 07:01:26 UTC
You mean the *_on elsewhere in the codebase.. yeah, I think it's a good idea to work towards the guidelines on another bug.  I'd love to see that done codebase wide, but I'm also worried that may constitute a V2 change for the API
Comment 9 Jonathan Druart 2023-07-04 07:26:05 UTC
The other problem we had with "_date" is that we are using it regardless the datatype (datetime, date, timestamp) and could lead to confusion. Just noting here as it was the reason why I didn't like it (but not the place to discuss this anyway).
Comment 10 Martin Renvoize 2023-07-17 14:47:05 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 11 Pedro Amorim 2023-07-18 11:58:05 UTC
Missing bug 31794 in 22.11.x. Doesn't apply. Not pushing.