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.
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>
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>
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?
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>
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>
Pushed to master for 23.11. Nice work everyone, thanks!
Should we fix the '*_on' occurrences on a separate bug report or is it too late now?
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
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).
Thanks for all the hard work! Pushed to 23.05.x for the next release
Missing bug 31794 in 22.11.x. Doesn't apply. Not pushing.