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>

diff --git a/Koha/REST/V1/AdvancedEditorMacro.pm b/Koha/REST/V1/AdvancedEditorMacro.pm
index e941062..86a2f46 100644 (file)
--- 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 2fa8f20..e08f45a 100644 (file)
--- 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 72fb177..b95eeb2 100644 (file)
--- 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');
 
 };