Bugzilla – Attachment 105044 Details for
Bug 25502
/advanced_editor/macros doesn't follow coding guidelines
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 25502: Adapt Advanced macros routes to current guidelines
Bug-25502-Adapt-Advanced-macros-routes-to-current-.patch (text/plain), 11.88 KB, created by
Victor Grousset/tuxayo
on 2020-05-18 19:34:07 UTC
(
hide
)
Description:
Bug 25502: Adapt Advanced macros routes to current guidelines
Filename:
MIME Type:
Creator:
Victor Grousset/tuxayo
Created:
2020-05-18 19:34:07 UTC
Size:
11.88 KB
patch
obsolete
>From e2e731034441d31cc605e8213ec54f0e5f4c35fc Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >Date: Thu, 14 May 2020 09:06:51 -0300 >Subject: [PATCH] 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> >--- > Koha/REST/V1/AdvancedEditorMacro.pm | 180 +++++------------- > .../swagger/paths/advancededitormacros.json | 4 +- > .../api/v1/advanced_editor_macros.t | 12 +- > 3 files changed, 53 insertions(+), 143 deletions(-) > >diff --git a/Koha/REST/V1/AdvancedEditorMacro.pm b/Koha/REST/V1/AdvancedEditorMacro.pm >index e94106242a..86a2f46648 100644 >--- a/Koha/REST/V1/AdvancedEditorMacro.pm >+++ b/Koha/REST/V1/AdvancedEditorMacro.pm >@@ -21,11 +21,13 @@ use Koha::AdvancedEditorMacros; > > use Try::Tiny; > >-=head1 API >+=head1 Name > >-=head2 Class Methods >+Koha::REST::V1::AdvancedEditorMacro > >-=cut >+=head1 API >+ >+=head2 Methods > > =head3 list > >@@ -37,19 +39,20 @@ sub list { > my $c = shift->openapi->valid_input or return; > my $patron = $c->stash('koha.user'); > return try { >- my $macros_set = Koha::AdvancedEditorMacros->search({ -or => { shared => 1, borrowernumber => $patron->borrowernumber } }); >- my $macros = $c->objects->search( $macros_set, \&_to_model, \&_to_api ); >- return $c->render( status => 200, openapi => $macros ); >+ my $macros_set = Koha::AdvancedEditorMacros->search( >+ { >+ -or => >+ { shared => 1, borrowernumber => $patron->borrowernumber } >+ } >+ ); >+ my $macros = $c->objects->search( $macros_set ); >+ return $c->render( >+ status => 200, >+ openapi => $macros >+ ); > } > catch { >- if ( $_->isa('DBIx::Class::Exception') ) { >- return $c->render( status => 500, >- openapi => { error => $_->{msg} } ); >- } >- else { >- return $c->render( status => 500, >- openapi => { error => "Something went wrong, check the logs. $_"} ); >- } >+ $c->unhandled_exception($_); > }; > > } >@@ -123,15 +126,17 @@ sub add { > } > > return try { >- my $macro = Koha::AdvancedEditorMacro->new( _to_model( $c->validation->param('body') ) ); >- $macro->store; >+ my $macro = Koha::AdvancedEditorMacro->new_from_api( $c->validation->param('body') ); >+ $macro->store->discard_changes; > $c->res->headers->location( $c->req->url->to_string . '/' . $macro->id ); > return $c->render( > status => 201, > openapi => $macro->to_api > ); > } >- catch { handle_error($_) }; >+ catch { >+ $c->unhandled_exception($_); >+ }; > } > > =head3 add_shared >@@ -148,15 +153,17 @@ sub add_shared { > openapi => { error => "To create private macros you must use advancededitor" } ); > } > return try { >- my $macro = Koha::AdvancedEditorMacro->new( _to_model( $c->validation->param('body') ) ); >- $macro->store; >+ my $macro = Koha::AdvancedEditorMacro->new_from_api( $c->validation->param('body') ); >+ $macro->store->discard_changes; > $c->res->headers->location( $c->req->url->to_string . '/' . $macro->id ); > return $c->render( > status => 201, > openapi => $macro->to_api > ); > } >- catch { handle_error($_) }; >+ catch { >+ $c->unhandled_exception($_); >+ }; > } > > =head3 update >@@ -188,11 +195,13 @@ sub update { > > return try { > my $params = $c->req->json; >- $macro->set( _to_model($params) ); >- $macro->store(); >+ $macro->set_from_api( $params ); >+ $macro->store->discard_changes; > return $c->render( status => 200, openapi => $macro->to_api ); > } >- catch { handle_error($_) }; >+ catch { >+ $c->unhandled_exception($_); >+ }; > } > > =head3 update_shared >@@ -218,11 +227,13 @@ sub update_shared { > > return try { > my $params = $c->req->json; >- $macro->set( _to_model($params) ); >- $macro->store(); >+ $macro->set_from_api( $params ); >+ $macro->store->discard_changes; > return $c->render( status => 200, openapi => $macro->to_api ); > } >- catch { handle_error($_) }; >+ catch { >+ $c->unhandled_exception($_); >+ }; > } > > =head3 delete >@@ -253,9 +264,11 @@ sub delete { > > return try { > $macro->delete; >- return $c->render( status => 200, openapi => "" ); >+ return $c->render( status => 204, openapi => q{} ); > } >- catch { handle_error($_) }; >+ catch { >+ $c->unhandled_exception($_); >+ }; > } > > =head3 delete_shared >@@ -280,114 +293,11 @@ sub delete_shared { > > return try { > $macro->delete; >- return $c->render( status => 200, openapi => "" ); >- } >- catch { handle_error($_,$c) }; >-} >- >-=head3 _handle_error >- >-Helper function that passes exception or error >- >-=cut >- >-sub _handle_error { >- my ($err,$c) = @_; >- if ( $err->isa('DBIx::Class::Exception') ) { >- return $c->render( status => 500, >- openapi => { error => $err->{msg} } ); >- } >- else { >- return $c->render( status => 500, >- openapi => { error => "Something went wrong, check the logs."} ); >- } >-}; >- >- >-=head3 _to_api >- >-Helper function that maps a hashref of Koha::AdvancedEditorMacro attributes into REST api >-attribute names. >- >-=cut >- >-sub _to_api { >- my $macro = shift; >- >- # Rename attributes >- foreach my $column ( keys %{ $Koha::REST::V1::AdvancedEditorMacro::to_api_mapping } ) { >- my $mapped_column = $Koha::REST::V1::AdvancedEditorMacro::to_api_mapping->{$column}; >- if ( exists $macro->{ $column } >- && defined $mapped_column ) >- { >- # key /= undef >- $macro->{ $mapped_column } = delete $macro->{ $column }; >- } >- elsif ( exists $macro->{ $column } >- && !defined $mapped_column ) >- { >- # key == undef => to be deleted >- delete $macro->{ $column }; >- } >- } >- >- return $macro; >-} >- >-=head3 _to_model >- >-Helper function that maps REST api objects into Koha::AdvancedEditorMacros >-attribute names. >- >-=cut >- >-sub _to_model { >- my $macro = shift; >- >- foreach my $attribute ( keys %{ $Koha::REST::V1::AdvancedEditorMacro::to_model_mapping } ) { >- my $mapped_attribute = $Koha::REST::V1::AdvancedEditorMacro::to_model_mapping->{$attribute}; >- if ( exists $macro->{ $attribute } >- && defined $mapped_attribute ) >- { >- # key /= undef >- $macro->{ $mapped_attribute } = delete $macro->{ $attribute }; >- } >- elsif ( exists $macro->{ $attribute } >- && !defined $mapped_attribute ) >- { >- # key == undef => to be deleted >- delete $macro->{ $attribute }; >- } >+ return $c->render( status => 204, openapi => q{} ); > } >- >- if ( exists $macro->{shared} ) { >- $macro->{shared} = ($macro->{shared}) ? 1 : 0; >- } >- >- >- return $macro; >+ catch { >+ $c->unhandled_exception($_); >+ }; > } > >-=head2 Global variables >- >-=head3 $to_api_mapping >- >-=cut >- >-our $to_api_mapping = { >- id => 'macro_id', >- macro => 'macro_text', >- borrowernumber => 'patron_id', >-}; >- >-=head3 $to_model_mapping >- >-=cut >- >-our $to_model_mapping = { >- macro_id => 'id', >- macro_text => 'macro', >- patron_id => 'borrowernumber', >-}; >- > 1; >diff --git a/api/v1/swagger/paths/advancededitormacros.json b/api/v1/swagger/paths/advancededitormacros.json >index 2fa8f2034a..e08f45ab46 100644 >--- a/api/v1/swagger/paths/advancededitormacros.json >+++ b/api/v1/swagger/paths/advancededitormacros.json >@@ -308,7 +308,7 @@ > "application/json" > ], > "responses": { >- "200": { >+ "204": { > "description": "Advanced editor macro deleted", > "schema": { > "type": "string" >@@ -477,7 +477,7 @@ > "application/json" > ], > "responses": { >- "200": { >+ "204": { > "description": "Advanced editor macro deleted", > "schema": { > "type": "string" >diff --git a/t/db_dependent/api/v1/advanced_editor_macros.t b/t/db_dependent/api/v1/advanced_editor_macros.t >index 72fb17703c..b95eeb2cb5 100644 >--- a/t/db_dependent/api/v1/advanced_editor_macros.t >+++ b/t/db_dependent/api/v1/advanced_editor_macros.t >@@ -151,7 +151,7 @@ subtest 'get() tests' => sub { > > $t->get_ok( "//$userid:$password@/api/v1/advanced_editor/macros/shared/" . $macro_1->id ) > ->status_is( 200, 'Can get a shared macro via shared endpoint' ) >- ->json_is( '' => Koha::REST::V1::AdvancedEditorMacro::_to_api( $macro_1->TO_JSON ), 'Macro correctly retrieved' ); >+ ->json_is( $macro_1->to_api ); > > $t->get_ok( "//$userid:$password@/api/v1/advanced_editor/macros/" . $macro_2->id ) > ->status_is( 403, 'Cannot access another users macro' ) >@@ -159,7 +159,7 @@ subtest 'get() tests' => sub { > > $t->get_ok( "//$userid:$password@/api/v1/advanced_editor/macros/" . $macro_3->id ) > ->status_is( 200, 'Can get your own private macro' ) >- ->json_is( '' => Koha::REST::V1::AdvancedEditorMacro::_to_api( $macro_3->TO_JSON ), 'Macro correctly retrieved' ); >+ ->json_is( $macro_3->to_api ); > > my $non_existent_code = $macro_1->id; > $macro_1->delete; >@@ -202,7 +202,7 @@ subtest 'add() tests' => sub { > class => 'Koha::AdvancedEditorMacros', > value => { shared => 0 } > }); >- my $macro_values = Koha::REST::V1::AdvancedEditorMacro::_to_api( $macro->TO_JSON ); >+ my $macro_values = $macro->to_api; > delete $macro_values->{macro_id}; > $macro->delete; > >@@ -317,7 +317,7 @@ subtest 'update() tests' => sub { > }); > my $macro_id = $macro->id; > my $macro_2_id = $macro_2->id; >- my $macro_values = Koha::REST::V1::AdvancedEditorMacro::_to_api( $macro->TO_JSON ); >+ my $macro_values = $macro->to_api; > delete $macro_values->{macro_id}; > > # Unauthorized attempt to update >@@ -448,7 +448,7 @@ subtest 'delete() tests' => sub { > ->status_is(403, "Cannot delete macro without permission"); > > $t->delete_ok( "//$auth_userid:$password@/api/v1/advanced_editor/macros/$macro_id") >- ->status_is(200, 'Can delete macro with permission'); >+ ->status_is( 204, 'Can delete macro with permission'); > > $t->delete_ok( "//$auth_userid:$password@/api/v1/advanced_editor/macros/$macro_2_id") > ->status_is(403, 'Cannot delete other users macro with permission'); >@@ -469,7 +469,7 @@ subtest 'delete() tests' => sub { > $t->delete_ok( "//$auth_userid:$password@/api/v1/advanced_editor/macros/$macro_2_id") > ->status_is(403, 'Cannot delete other users shared macro with permission on private endpoint'); > $t->delete_ok( "//$auth_userid:$password@/api/v1/advanced_editor/macros/shared/$macro_2_id") >- ->status_is(200, 'Can delete other users shared macro with permission'); >+ ->status_is(204, 'Can delete other users shared macro with permission'); > > }; > >-- >2.26.2
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 25502
:
104877
|
105044
|
105062