Bug 25502 - /advanced_editor/macros doesn't follow coding guidelines
Summary: /advanced_editor/macros doesn't follow coding guidelines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 25482
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-14 12:04 UTC by Tomás Cohen Arazi
Modified: 2020-11-30 21:48 UTC (History)
5 users (show)

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


Attachments
Bug 25502: Adapt Advanced macros routes to current guidelines (11.83 KB, patch)
2020-05-14 12:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25502: Adapt Advanced macros routes to current guidelines (11.88 KB, patch)
2020-05-18 19:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25502: Adapt Advanced macros routes to current guidelines (11.95 KB, patch)
2020-05-19 08:33 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 2020-05-14 12:04:29 UTC
This development started before the changes we introduced in the guidelines in late 2019, and the major code changes that took place in January 2020.

- Attribute mapping logic is now on the Koha::Object-level (the patches implement that, but are not using it)
- Related to the above, some helper methods like to_api and to_model are kept, the same for the mappings in the controller, they should all go away
- Related to the above, set_from_api and new_from_api should be used instead of using helper to_api and to_model methods in the controller
- $c->objects->search doesn't use the to_model and to_api params
- Response status codes need to be changed, at least for DELETE operations
Comment 1 Tomás Cohen Arazi 2020-05-14 12:08:33 UTC
Created attachment 104877 [details] [review]
Bug 25502: Adapt Advanced macros routes to current guidelines

The original development started before the changes we introduced in the guidelines in late 2019, and the major code changes that took place in January 2020.

- Attribute mapping logic is now on the Koha::Object-level (the patches implement that, but are not using it)
- Related to the above, some helper methods like to_api and to_model are kept, the same for the mappings in the controller, they should all go away
- Related to the above, set_from_api and new_from_api should be used instead of using helper to_api and to_model methods in the controller
- $c->objects->search doesn't use the to_model and to_api params
- Response status codes need to be changed, at least for DELETE operations

Those are fixed by this patch.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/advanced_editor_macros.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 2 Victor Grousset/tuxayo 2020-05-18 19:34:07 UTC
Created attachment 105044 [details] [review]
Bug 25502: Adapt Advanced macros routes to current guidelines

The original development started before the changes we introduced in the guidelines in late 2019, and the major code changes that took place in January 2020.

- Attribute mapping logic is now on the Koha::Object-level (the patches implement that, but are not using it)
- Related to the above, some helper methods like to_api and to_model are kept, the same for the mappings in the controller, they should all go away
- Related to the above, set_from_api and new_from_api should be used instead of using helper to_api and to_model methods in the controller
- $c->objects->search doesn't use the to_model and to_api params
- Response status codes need to be changed, at least for DELETE operations

Those are fixed by this patch.

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

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 3 Martin Renvoize 2020-05-19 08:33:03 UTC
Created attachment 105062 [details] [review]
Bug 25502: Adapt Advanced macros routes to current guidelines

The original development started before the changes we introduced in the guidelines in late 2019, and the major code changes that took place in January 2020.

- Attribute mapping logic is now on the Koha::Object-level (the patches implement that, but are not using it)
- Related to the above, some helper methods like to_api and to_model are kept, the same for the mappings in the controller, they should all go away
- Related to the above, set_from_api and new_from_api should be used instead of using helper to_api and to_model methods in the controller
- $c->objects->search doesn't use the to_model and to_api params
- Response status codes need to be changed, at least for DELETE operations

Those are fixed by this patch.

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

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2020-05-19 08:34:15 UTC
Works as expected and passes QA tests.. all in all, this is a step in the right direction towards consistency, thanks Tomas.

I was surprised by how poorly the API seems to perform for this route.. but that appears to be the case prior to the patch as well as after.
Comment 5 Martin Renvoize 2020-05-19 14:21:41 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 6 Joy Nelson 2020-05-20 21:13:04 UTC
missing dependency -not backported to 19.11.x