Attachment 114118 Details for Bug 17314 – Bug 17314: (QA follow-up)

[patch] Bug 17314: (QA follow-up)

Bug-17314-QA-follow-up.patch (text/plain), 20.51 KB, created by Arthur Suzuki on 2020-12-02 20:53:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Arthur Suzuki

Created: 2020-12-02 20:53:40 UTC

Size: 20.51 KB

patch

obsolete

>From bd69b6ef55cff87eeb7830226d696653ebe4f6f5 Mon Sep 17 00:00:00 2001
>From: Arthur Suzuki <arthur.suzuki@biblibre.com>
>Date: Sat, 12 Sep 2020 01:00:47 +0200
>Subject: [PATCH] Bug 17314: (QA follow-up)
>
>---
> Koha/Patron.pm                             |   1 +
> Koha/REST/V1/Suggestions.pm                | 297 ++++++++++++-----------------
> Koha/Suggestion.pm                         |  64 +++++++
> api/v1/swagger/definitions/suggestion.json |  18 +-
> api/v1/swagger/parameters.json             |   4 +-
> api/v1/swagger/parameters/suggestion.json  |   4 +-
> api/v1/swagger/paths.json                  |   4 +-
> api/v1/swagger/paths/suggestions.json      |  12 +-
> 8 files changed, 209 insertions(+), 195 deletions(-)
>
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index e26ba47c25..735a69756a 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -1806,6 +1806,7 @@ sub to_api_mapping {
> =head2 Internal methods
> 
> =head3 _type
>+
> =cut
> 
> sub _type {
>diff --git a/Koha/REST/V1/Suggestions.pm b/Koha/REST/V1/Suggestions.pm
>index 64f7d54de9..223315c4e8 100644
>--- a/Koha/REST/V1/Suggestions.pm
>+++ b/Koha/REST/V1/Suggestions.pm
>@@ -11,9 +11,8 @@ package Koha::REST::V1::Suggestions;
> # WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
> # A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
> #
>-# You should have received a copy of the GNU General Public License along
>-# with Koha; if not, write to the Free Software Foundation, Inc.,
>-# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http:Â°www.gnu.org/licenses>.
> 
> use Modern::Perl;
> 
>@@ -28,67 +27,133 @@ use Koha::Suggestions;
> 
> use Koha::ItemTypes;
> use Koha::Libraries;
>+use Koha::DateUtils;
> use List::MoreUtils qw/any/;
> 
> use Try::Tiny;
> 
>-sub list {
>-    my $c = shift->openapi->valid_input or return;
>+=head1 NAME
> 
>-    my $suggestions;
>-    my $filter;
>-    my $args = $c->validation->output;
>+Koha::REST::V1::Suggestion
> 
>-    for my $filter_param ( keys %$args ) {
>-        $filter->{$filter_param} = { LIKE => $args->{$filter_param} . '%' };
>-    }
>+=head1 API
>+
>+=head2 Methods
>+
>+=head3 list
>+
>+List Koha::Suggestion objects
>+
>+=cut
>+
>+sub list {
>+    my $c = shift->openapi->valid_input or return;
> 
>     return try {
>-        $suggestions = Koha::Suggestions->search($filter, \&_to_model, \&_to_api);
>-        return $c->render( status => 200, openapi => $suggestions );
>+        my $suggestion_set = Koha::Suggestions->new;
>+        my $suggestions    = $c->objects->search( $suggestion_set );
>+        return $c->render(
>+            status  => 200,
>+            openapi => $suggestions
>+        );
>     }
>     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($_);
>     };
> }
> 
>+=head3 get
>+
>+Controller function that handles retrieving a single Koha::Suggestion object
>+
>+=cut
>+
> sub get {
>     my $c = shift->openapi->valid_input or return;
>-    my $args = $c->validation->output;
> 
>-    my $suggestion = Koha::Suggestions->find($args->{suggestionid});
>-    unless ($suggestion) {
>-        return $c->render( status => 404, openapi => {error => 'Suggestion not found'});
>-    }
>+    return try {
>+        my $suggestion_id = $c->validation->param('suggestion_id');
>+        my $suggestion    = Koha::Suggestions->find($suggestion_id);
>+
>+        unless ($suggestion) {
>+            return $c->render( status => 404, openapi => { error => "Suggestion not found." } );
>+        }
> 
>-    return $c->render( status => 200, openapi => $suggestion->unblessed );
>+        return $c->render( status => 200, openapi => $suggestion->to_api );
>+    }
>+    catch {
>+        $c->unhandled_exception($_);
>+    };
> }
> 
>+=head3 add
>+
>+Controller function that handles adding a new Koha::Suggestion object
>+
>+=cut
>+
> sub add {
>     my $c = shift->openapi->valid_input or return;
>-    my $args = $c->validation->output;
>-
>-    my $suggestion = Koha::Suggestion->new( $args->{body} );
>+    my $params = $c->validation->param('body');
>+    $params->{'date_created'} = dt_from_string();
>+    $params->{'status'} = 'ASKED' unless defined $params->{'status'};
> 
>     return try {
>-        $suggestion->store;
>-        return $c->render( status => 200, openapi => $suggestion->unblessed );
>+        my $suggestion = Koha::Suggestion->new_from_api( $params )->store;
>+        $c->res->headers->location( $c->req->url->to_string . '/' . $suggestion->suggestionid );
>+        return $c->render(
>+            status  => 201,
>+            openapi => $suggestion->to_api
>+        );
>     }
>     catch {
>-        if ( $_->isa('DBIx::Class::Exception') ) {
>-            return $c->render( status => 500, openapi => { error => $_->msg } );
>+
>+        my $to_api_mapping = Koha::Suggestion->new->to_api_mapping;
>+
>+        unless ( blessed $_ && $_->can('rethrow') ) {
>+            return $c->render(
>+                status  => 500,
>+                openapi => { error => "Something went wrong, check Koha logs for details." }
>+            );
>+        }
>+        if ( $_->isa('Koha::Exceptions::Object::DuplicateID') ) {
>+            return $c->render(
>+                status  => 409,
>+                openapi => { error => $_->error, conflict => $_->duplicate_id }
>+            );
>+        }
>+        elsif ( $_->isa('Koha::Exceptions::Object::FKConstraint') ) {
>+            return $c->render(
>+                status  => 400,
>+                openapi => {
>+                          error => "Given "
>+                        . $to_api_mapping->{ $_->broken_fk }
>+                        . " does not exist"
>+                }
>+            );
>+        }
>+        elsif ( $_->isa('Koha::Exceptions::BadParameter') ) {
>+            return $c->render(
>+                status  => 400,
>+                openapi => {
>+                          error => "Given "
>+                        . $to_api_mapping->{ $_->parameter }
>+                        . " does not exist"
>+                }
>+            );
>         }
>         else {
>-            return $c->render( status => 500, openapi => { error => 'Something went wrong, check the logs.' } );
>+            $c->unhandled_exception($_);
>         }
>-    }
>+    };
> }
> 
>+=head3 update
>+
>+Controller function that handles modifying Koha::Suggestion object
>+
>+=cut
>+
> sub update {
>     my $c = shift->openapi->valid_input or return;
> 
>@@ -98,20 +163,20 @@ sub update {
> 
>     return try {
> 
>-        $suggestion = Koha::Suggestions->find( $args->{suggestionid} );
>+        $suggestion = Koha::Suggestions->find( $args->{suggestion_id} );
> 
>         my $body = $args->{body};
> 
>         # Remove unchaned fields so that we can use our body validation from the add subroutine
>         foreach my $param ( keys %{ $body } ) {
>             if (exists $body->{$param}) {
>-                delete $body->{$param} if $body->{$param} eq $suggestion->unblessed->{$param};
>+                delete $body->{$param} if $body->{$param} eq $suggestion->to_api->{$param};
>             }
>         }
> 
>-        $suggestion->set( $body );
>+        $suggestion->set_from_api( $body );
>         $suggestion->store();
>-        return $c->render( status => 200, openapi => $suggestion->unblessed );
>+        return $c->render( status => 200, openapi => $suggestion->to_api );
>     }
>     catch {
>         if ( not defined $suggestion ) {
>@@ -127,6 +192,12 @@ sub update {
> 
> }
> 
>+=head3 delete
>+
>+Controller function that handles removing a Koha::Suggestion object
>+
>+=cut
>+
> sub delete {
>     my $c = shift->openapi->valid_input or return;
> 
>@@ -135,7 +206,7 @@ sub delete {
>     my $suggestion;
> 
>     return try {
>-        $suggestion = Koha::Suggestions->find( $args->{suggestionid} );
>+        $suggestion = Koha::Suggestions->find( $args->{suggestion_id} );
>         $suggestion->delete;
>         return $c->render( status => 200, openapi => '' );
>     }
>@@ -175,11 +246,11 @@ sub _validate_body {
> 
>         foreach my $param ( keys %{ $body } ) {
>             unless (any { /^$param$/ } @allowed_fields) {
>-                # Ouch ! User trying to edit field he has no rights to edit!
>+                # Ouch ! User trying to edit field they has no rights to edit!
>                 my $verb = 'specified ';
>                 return $c->render( status => 403 , openapi => {
>-				error => 'You ' . $verb . $param . ', but allowed fields are only ' . join(', ', @allowed_fields)}
>-				 );
>+                                error => 'You ' . $verb . $param . ', but allowed fields are only ' . join(', ', @allowed_fields)}
>+                                 );
>             }
>         }
>     }
>@@ -187,7 +258,7 @@ sub _validate_body {
>     # Set user's branchcode if not specified (after validation input, because user does not
>     # necessarily have rights to choose a branchcode)
>     if ( not exists $body->{branchcode} ) {
>-	    $body->{branchcode} = C4::Context->userenv->{branch};
>+            $body->{branchcode} = C4::Context->userenv->{branch};
>     }
> 
>     # Is suggester anonymous?
>@@ -221,21 +292,21 @@ sub _validate_body {
>     if ( exists $body->{itemtype} ) {
>         my @item_types = map {$_->unblessed->{itemtype}} Koha::ItemTypes->search;
>         return $c->render( status => 400 , openapi => {error => 'itemtype must be one of ' . join(', ', @item_types)} )
>-	unless grep(/^$body->{itemtype}$/, @item_types);
>+        unless grep(/^$body->{itemtype}$/, @item_types);
>     }
> 
>     # Check branchcode is valid
>     if ( exists $body->{branchcode} ) {
>         my @branch_codes = map {$_->unblessed->{branchcode}} Koha::Libraries->search;
>         return $c->render( status => 400 , openapi => {error => 'branchcode must be one of ' . join(', ', @branch_codes)} )
>-	unless grep(/^$body->{branchcode}$/, @branch_codes);
>+        unless grep(/^$body->{branchcode}$/, @branch_codes);
>     }
> 
>     # Check patron reason is valid
>     if ( exists $body->{patronreason} ) {
>         my @authorized_values = map { $_->{authorised_value} } @{ C4::Koha::GetAuthorisedValues('OPAC_SUG') };
>         return $c->render( status => 400, openapi => {error => 'patronreason must be one of ' . join(', ', @authorized_values)} )
>-	unless grep(/^$body->{patronreason}$/, @authorized_values);
>+        unless grep(/^$body->{patronreason}$/, @authorized_values);
>     }
> 
>     # Check suggestedby patron exists
>@@ -253,138 +324,4 @@ sub _validate_body {
>     # Everything's fine ..
>     return 0;
> }
>-=head3 _to_api
>-
>-Helper function that maps unblessed Koha::Suggestion objects into REST api
>-attribute names.
>-
>-=cut
>-
>-sub _to_api {
>-    my $suggestion = shift;
>-
>-    # Rename attributes
>-    foreach my $column ( keys %{ $Koha::REST::V1::Suggestions::to_api_mapping } ) {
>-        my $mapped_column = $Koha::REST::V1::Suggestions::to_api_mapping->{$column};
>-        if (    exists $suggestion->{ $column }
>-             && defined $mapped_column )
>-        {
>-            # key != undef
>-            $suggestion->{ $mapped_column } = delete $suggestion->{ $column };
>-        }
>-        elsif (    exists $suggestion->{ $column }
>-                && !defined $mapped_column )
>-        {
>-            # key == undef
>-            delete $suggestion->{ $column };
>-        }
>-    }
>-
>-    return $suggestion;
>-}
>-
>-=head3 _to_model
>-
>-Helper function that maps REST api objects into Koha::Suggestion
>-attribute names.
>-
>-=cut
>-
>-sub _to_model {
>-    my $suggestion = shift;
>-
>-    foreach my $attribute ( keys %{ $Koha::REST::V1::Suggestions::to_model_mapping } ) {
>-        my $mapped_attribute = $Koha::REST::V1::Suggestions::to_model_mapping->{$attribute};
>-        if (    exists $suggestion->{ $attribute }
>-             && defined $mapped_attribute )
>-        {
>-            # key => !undef
>-            $suggestion->{ $mapped_attribute } = delete $suggestion->{ $attribute };
>-        }
>-        elsif (    exists $suggestion->{ $attribute }
>-                && !defined $mapped_attribute )
>-        {
>-            # key => undef / to be deleted
>-            delete $suggestion->{ $attribute };
>-        }
>-    }
>-
>-    return $suggestion;
>-}
>-
>-=head2 Global variables
>-
>-=head3 $to_api_mapping
>-
>-=cut
>-
>-our $to_api_mapping = {
>-   suggestionid => 'suggestion_id',
>-   suggestedby => 'suggested_by',
>-   suggesteddate => 'suggestion_date',
>-   managedby => 'managed_by',
>-   manageddate => 'managed_date',
>-   accepteddate => 'accepted_date',
>-   rejectedby => 'rejected_by',
>-   rejectiondate => 'rejected_date',
>-   STATUS => 'status',
>-   note => 'note',
>-   author => 'author',
>-   title => 'title',
>-   copyrightdate => 'copyright_date',
>-   publishercode => 'publisher_code',
>-   date => 'date_created',
>-   volumedesc => 'volume_desc',
>-   publicationyear => 'publication_year',
>-   place => 'publication_place',
>-   isbn => 'isbn',
>-   biblionumber => 'biblio_id',
>-   reason => 'reason',
>-   patronreason => 'patron_reason',
>-   budgetid => 'budget_id',
>-   branchcode => 'library_id',
>-   collectiontitle => 'collection_title',
>-   itemtype => 'item_type',
>-   quantity => 'quantity',
>-   currency => 'currency',
>-   price => 'item_price',
>-   total => 'total_price'
>-};
>-
>-=head3 $to_model_mapping
>-
>-=cut
>-
>-our $to_model_mapping = {
>-   suggestion_id => 'suggestionid',
>-   suggested_by => 'suggestedby',
>-   suggestion_date => 'suggesteddate',
>-   managed_by => 'managedby',
>-   managed_date => 'manageddate',
>-   accepted_date => 'accepteddate',
>-   rejected_by => 'rejectedby',
>-   rejected_date => 'rejectiondate',
>-   status => 'STATUS',
>-   note => 'note',
>-   author => 'author',
>-   title => 'title',
>-   copyright_date => 'copyrightdate',
>-   publisher_code => 'publishercode',
>-   date_created => 'date',
>-   volume_desc => 'volumedesc',
>-   publication_year => 'publicationyear',
>-   publication_place => 'place',
>-   isbn => 'isbn',
>-   biblio_id => 'biblionumber',
>-   reason => 'reason',
>-   patron_reason => 'patronreason',
>-   budget_id => 'budgetid',
>-   library_id => 'branchcode',
>-   collection_title => 'collectiontitle',
>-   item_type => 'itemtype',
>-   quantity => 'quantity',
>-   currency => 'currency',
>-   item_price => 'price',
>-   total_price => 'total'
>-};
> 1;
>diff --git a/Koha/Suggestion.pm b/Koha/Suggestion.pm
>index f9f8c2a490..586a01e289 100644
>--- a/Koha/Suggestion.pm
>+++ b/Koha/Suggestion.pm
>@@ -139,6 +139,70 @@ sub _type {
>     return 'Suggestion';
> }
> 
>+
>+=head3 to_api
>+
>+    my $json = $patron->to_api;
>+
>+Overloaded method that returns a JSON representation of the Koha::Patron object,
>+suitable for API output.
>+
>+=cut
>+
>+sub to_api {
>+    my ( $self, $params ) = @_;
>+
>+    my $json_suggestion = $self->SUPER::to_api( $params );
>+
>+    return $json_suggestion;
>+}
>+
>+=head3 to_api_mapping
>+
>+This method returns the mapping for representing a Koha::Patron object
>+on the API.
>+
>+=cut
>+
>+sub to_api_mapping {
>+   return {
>+      suggestionid => 'suggestion_id',
>+      suggestedby => 'suggested_by',
>+      suggesteddate => 'suggestion_date',
>+      managedby => 'managed_by',
>+      manageddate => 'managed_date',
>+      acceptedby => 'accepted_by',
>+      accepteddate => 'accepted_date',
>+      rejectedby => 'rejected_by',
>+      rejecteddate => 'rejected_date',
>+      lastmodificationdate => 'last_modification_date',
>+      lastmodificationby => 'last_modification_by',
>+      STATUS => 'status',
>+      note => 'note',
>+      author => 'author',
>+      title => 'title',
>+      copyrightdate => 'copyright_date',
>+      publishercode => 'publisher_code',
>+      date => 'date_created',
>+      volumedesc => 'volume_desc',
>+      publicationyear => 'publication_year',
>+      place => 'publication_place',
>+      isbn => 'isbn',
>+      biblionumber => 'biblio_id',
>+      reason => 'reason',
>+      patronreason => 'patron_reason',
>+      budgetid => 'budget_id',
>+      branchcode => 'library_id',
>+      collectiontitle => 'collection_title',
>+      itemtype => 'item_type',
>+      quantity => 'quantity',
>+      currency => 'currency',
>+      price => 'item_price',
>+      total => 'total_price',
>+      archived => 'archived',
>+   };
>+}
>+
> =head1 AUTHOR
> 
> Kyle M Hall <kyle@bywatersolutions.com>
>diff --git a/api/v1/swagger/definitions/suggestion.json b/api/v1/swagger/definitions/suggestion.json
>index 3a9c4c828f..11e5095c1e 100644
>--- a/api/v1/swagger/definitions/suggestion.json
>+++ b/api/v1/swagger/definitions/suggestion.json
>@@ -9,7 +9,7 @@
>       "type": ["string", "null"],
>       "description": "patron_id for the person making the suggestion, foreign key linking to the borrowers table"
>     },
>-    "suggested_date": {
>+    "suggestion_date": {
>       "type": ["string", "null"],
>       "format": "date",
>       "description": "the suggestion was submitted"
>@@ -41,6 +41,15 @@
>       "format": "date",
>       "description": "date the suggestion was marked as rejected"
>     },
>+    "last_modification_by": {
>+      "type": ["string", "null"],
>+      "description": "patron the suggestion was last modified by"
>+    },
>+    "last_modification_date": {
>+      "type": ["string", "null"],
>+      "format": "date",
>+      "description": "date the suggestion was last modified"
>+    },
>     "status": {
>       "type": "string",
>       "description": "Suggestion status",
>@@ -74,8 +83,7 @@
>     },
>     "date_created": {
>       "type": ["string", "null"],
>-      "format": "date",
>-      "description": "date created"
>+      "description": "timestamp of date created"
>     },
>     "volume_desc": {
>       "type": ["string", "null"],
>@@ -136,6 +144,10 @@
>     "total_price": {
>       "type": ["string", "null"],
>       "description": "suggested total cost (price*quantity updated for currency)"
>+    },
>+    "archived": {
>+      "type": ["boolean", "null"],
>+      "description": "archived (processed) suggestion"
>     }
>   },
>   "additionalProperties": false
>diff --git a/api/v1/swagger/parameters.json b/api/v1/swagger/parameters.json
>index f2b53a3aec..d2f52d1f55 100644
>--- a/api/v1/swagger/parameters.json
>+++ b/api/v1/swagger/parameters.json
>@@ -108,7 +108,7 @@
>   "fundidPathParam": {
>     "$ref": "parameters/fund.json#/fundidPathParam"
>   },
>-  "suggestionidPathParam": {
>-    "$ref": "parameters/suggestion.json#/suggestionidPathParam"
>+  "suggestion_id_pp": {
>+    "$ref": "parameters/suggestion.json#/suggestion_id_pp"
>   }
> }
>diff --git a/api/v1/swagger/parameters/suggestion.json b/api/v1/swagger/parameters/suggestion.json
>index 75647ea81c..0de69f476f 100644
>--- a/api/v1/swagger/parameters/suggestion.json
>+++ b/api/v1/swagger/parameters/suggestion.json
>@@ -1,6 +1,6 @@
> {
>-  "suggestionidPathParam": {
>-    "name": "suggestionid",
>+  "suggestion_id_pp": {
>+    "name": "suggestion_id",
>     "in": "path",
>     "description": "Internal suggestion identifier",
>     "required": true,
>diff --git a/api/v1/swagger/paths.json b/api/v1/swagger/paths.json
>index 84d8fd0d0f..7ad8a9d20d 100644
>--- a/api/v1/swagger/paths.json
>+++ b/api/v1/swagger/paths.json
>@@ -149,7 +149,7 @@
>   "/suggestions": {
>     "$ref": "paths/suggestions.json#/~1suggestions"
>   },
>-  "/suggestions/{suggestionid}": {
>-    "$ref": "paths/suggestions.json#/~1suggestions~1{suggestionid}"
>+  "/suggestions/{suggestion_id}": {
>+    "$ref": "paths/suggestions.json#/~1suggestions~1{suggestion_id}"
>   }
> }
>diff --git a/api/v1/swagger/paths/suggestions.json b/api/v1/swagger/paths/suggestions.json
>index e7cc622e88..2858556e8d 100644
>--- a/api/v1/swagger/paths/suggestions.json
>+++ b/api/v1/swagger/paths/suggestions.json
>@@ -139,7 +139,7 @@
>         "application/json"
>       ],
>       "responses": {
>-        "200": {
>+        "201": {
>           "description": "Suggestion added",
>           "schema": {
>             "$ref": "../definitions.json#/suggestion"
>@@ -171,13 +171,13 @@
>       }
>     }
>   },
>-  "/suggestions/{suggestionid}": {
>+  "/suggestions/{suggestion_id}": {
>     "get": {
>       "x-mojo-to": "Suggestions#get",
>-      "operationId": "get",
>+      "operationId": "getSuggestions",
>       "tags": ["patrons", "suggestions"],
>       "parameters": [{
>-          "$ref": "../parameters.json#/suggestionidPathParam"
>+          "$ref": "../parameters.json#/suggestion_id_pp"
>         }
>       ],
>       "produces": [
>@@ -214,7 +214,7 @@
>       "operationId": "update",
>       "tags": ["patrons", "suggestions"],
>       "parameters": [{
>-        "$ref": "../parameters.json#/suggestionidPathParam"
>+        "$ref": "../parameters.json#/suggestion_id_pp"
>       }, {
>         "name": "body",
>         "in": "body",
>@@ -270,7 +270,7 @@
>       "operationId": "delete",
>       "tags": ["patrons", "suggestions"],
>       "parameters": [{
>-        "$ref": "../parameters.json#/suggestionidPathParam"
>+        "$ref": "../parameters.json#/suggestion_id_pp"
>       }],
>       "produces": [
>         "application/json"
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 17314: 58098 | 58100 | 58101 | 58993 | 60133 | 90171 | 90172 | 90176 | 91545 | 91546 | 93761 | 93762 | 93763 | 93764 | 93765 | 108380 | 109972 | 109973 | 109974 | 109975 | 109976 | 109977 | 114113 | 114114 | 114115 | 114116 | 114117 | 114118 | 120111 | 120133 | 120134 | 120135 | 120136 | 125097 | 125098 | 125099 | 125100 | 125101 | 125102 | 125103 | 125104 | 127533 | 127534 | 127535 | 127536