Attachment 69262 Details for Bug 19410 – Bug 19410: (QA followup) Move build_query_params_from_api into a helper

[patch] Bug 19410: (QA followup) Move build_query_params_from_api into a helper

Bug-19410-QA-followup-Move-buildqueryparamsfromapi.patch (text/plain), 13.82 KB, created by Julian Maurice on 2017-11-21 19:32:08 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Julian Maurice

Created: 2017-11-21 19:32:08 UTC

Size: 13.82 KB

patch

obsolete

>From 222ade3a55fcf2a3543ac6906da6ed2d81e87b31 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>Date: Tue, 21 Nov 2017 16:12:08 -0300
>Subject: [PATCH] Bug 19410: (QA followup) Move build_query_params_from_api
> into a helper
>
>This patch creates the 'build_query_params' helper, instead of the
>original function in Koha::Objects.
>
>Unit tests are removed for Koha::Objects::_build_query_params_from_api and
>written for the helper plugin.
>
>The objects.search helper gets a call to build_query_params added. Tests for it
>updated to match this behaviour change.
>
>To test:
>- Apply this patches
>- Run:
>  $ kshell
> k$ prove t/Koha/REST/Plugin/Query.t \
>          t/db_dependent/Koha/Objects.t \
>          t/db_dependent/Koha/REST/Plugin/Objects.t
>=> SUCCESS: Tests pass!
>- Sign off :-D
>
>Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
>Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
>---
> Koha/Objects.pm                           | 41 ---------------
> Koha/REST/Plugin/Objects.pm               |  1 +
> Koha/REST/Plugin/Query.pm                 | 47 +++++++++++++++++
> t/Koha/REST/Plugin/Query.t                | 57 +++++++++++++++++++-
> t/db_dependent/Koha/Objects.t             | 65 +----------------------
> t/db_dependent/Koha/REST/Plugin/Objects.t | 86 ++++++++++++++++++++++++-------
> 6 files changed, 172 insertions(+), 125 deletions(-)
>
>diff --git a/Koha/Objects.pm b/Koha/Objects.pm
>index cfc2475d7c..58a0c686cf 100644
>--- a/Koha/Objects.pm
>+++ b/Koha/Objects.pm
>@@ -171,47 +171,6 @@ sub search_related {
>     }
> }
> 
>-=head2 _build_query_params_from_api
>-
>-    my $params = _build_query_params_from_api( $filtered_params, $reserved_params );
>-
>-Builds the params for searching on DBIC based on the selected matching algorithm.
>-Valid options are I<contains>, I<starts_with>, I<ends_with> and I<exact>. Default is
>-I<contains>. If other value is passed, a Koha::Exceptions::WrongParameter exception
>-is raised.
>-
>-=cut
>-
>-sub _build_query_params_from_api {
>-
>-    my ( $filtered_params, $reserved_params ) = @_;
>-
>-    my $params;
>-    my $match = $reserved_params->{_match} // 'contains';
>-
>-    foreach my $param ( keys %{$filtered_params} ) {
>-        if ( $match eq 'contains' ) {
>-            $params->{$param} =
>-              { like => '%' . $filtered_params->{$param} . '%' };
>-        }
>-        elsif ( $match eq 'starts_with' ) {
>-            $params->{$param} = { like => $filtered_params->{$param} . '%' };
>-        }
>-        elsif ( $match eq 'ends_with' ) {
>-            $params->{$param} = { like => '%' . $filtered_params->{$param} };
>-        }
>-        elsif ( $match eq 'exact' ) {
>-            $params->{$param} = $filtered_params->{$param};
>-        }
>-        else {
>-            Koha::Exceptions::WrongParameter->throw(
>-                "Invalid value for _match param ($match)");
>-        }
>-    }
>-
>-    return $params;
>-}
>-
> =head3 single
> 
> my $object = Koha::Objects->search({}, { rows => 1 })->single
>diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm
>index afd5e8e34e..eb3f951a39 100644
>--- a/Koha/REST/Plugin/Objects.pm
>+++ b/Koha/REST/Plugin/Objects.pm
>@@ -67,6 +67,7 @@ sub register {
>                 }
>             );
> 
>+            $filtered_params = $c->build_query_params( $filtered_params, $reserved_params );
>             # Perform search
>             my $objects = $objects_set->search( $filtered_params, $attributes );
> 
>diff --git a/Koha/REST/Plugin/Query.pm b/Koha/REST/Plugin/Query.pm
>index 45cfe14e7c..77d2fadd6a 100644
>--- a/Koha/REST/Plugin/Query.pm
>+++ b/Koha/REST/Plugin/Query.pm
>@@ -19,6 +19,8 @@ use Modern::Perl;
> 
> use Mojo::Base 'Mojolicious::Plugin';
> 
>+use Koha::Exceptions;
>+
> =head1 NAME
> 
> Koha::REST::Plugin::Query
>@@ -88,6 +90,51 @@ Generates the DBIC order_by attributes based on I<$params>, and merges into I<$a
>             return $attributes;
>         }
>     );
>+
>+=head3 _build_query_params_from_api
>+
>+    my $params = _build_query_params_from_api( $filtered_params, $reserved_params );
>+
>+Builds the params for searching on DBIC based on the selected matching algorithm.
>+Valid options are I<contains>, I<starts_with>, I<ends_with> and I<exact>. Default is
>+I<contains>. If other value is passed, a Koha::Exceptions::WrongParameter exception
>+is raised.
>+
>+=cut
>+
>+    $app->helper(
>+        'build_query_params' => sub {
>+
>+            my ( $c, $filtered_params, $reserved_params ) = @_;
>+
>+            my $params;
>+            my $match = $reserved_params->{_match} // 'contains';
>+
>+            foreach my $param ( keys %{$filtered_params} ) {
>+                if ( $match eq 'contains' ) {
>+                    $params->{$param} =
>+                      { like => '%' . $filtered_params->{$param} . '%' };
>+                }
>+                elsif ( $match eq 'starts_with' ) {
>+                    $params->{$param} = { like => $filtered_params->{$param} . '%' };
>+                }
>+                elsif ( $match eq 'ends_with' ) {
>+                    $params->{$param} = { like => '%' . $filtered_params->{$param} };
>+                }
>+                elsif ( $match eq 'exact' ) {
>+                    $params->{$param} = $filtered_params->{$param};
>+                }
>+                else {
>+                    # We should never reach here, because the OpenAPI plugin should
>+                    # prevent invalid params to be passed
>+                    Koha::Exceptions::WrongParameter->throw(
>+                        "Invalid value for _match param ($match)");
>+                }
>+            }
>+
>+            return $params;
>+        }
>+    );
> }
> 
> =head2 Internal methods
>diff --git a/t/Koha/REST/Plugin/Query.t b/t/Koha/REST/Plugin/Query.t
>index 28519622a6..a4c6684ba5 100644
>--- a/t/Koha/REST/Plugin/Query.t
>+++ b/t/Koha/REST/Plugin/Query.t
>@@ -19,6 +19,7 @@ use Modern::Perl;
> 
> # Dummy app for testing the plugin
> use Mojolicious::Lite;
>+use Try::Tiny;
> 
> app->log->level('error');
> 
>@@ -79,9 +80,26 @@ get '/dbic_merge_sorting' => sub {
>     $c->render( json => $attributes, status => 200 );
> };
> 
>+get '/build_query' => sub {
>+    my $c = shift;
>+    my ( $filtered_params, $reserved_params ) =
>+      $c->extract_reserved_params( $c->req->params->to_hash );
>+    my $query;
>+    try {
>+        $query = $c->build_query_params( $filtered_params, $reserved_params );
>+        $c->render( json => { query => $query }, status => 200 );
>+    }
>+    catch {
>+        $c->render(
>+            json => { exception_msg => $_->message, exception_type => ref($_) },
>+            status => 400
>+        );
>+    };
>+};
>+
> # The tests
> 
>-use Test::More tests => 2;
>+use Test::More tests => 3;
> use Test::Mojo;
> 
> subtest 'extract_reserved_params() tests' => sub {
>@@ -123,3 +141,40 @@ subtest 'dbic_merge_sorting() tests' => sub {
>       ->json_is( '/b' => 'b', 'Existing values are kept (b)' )
>       ->json_is( '/order_by' => [ 'uno', { -desc => 'dos' }, { -asc => 'tres' } ] );
> };
>+
>+subtest '_build_query_params_from_api' => sub {
>+
>+    plan tests => 16;
>+
>+    my $t = Test::Mojo->new;
>+
>+    # _match => contains
>+    $t->get_ok('/build_query?_match=contains&title=Ender&author=Orson')
>+      ->status_is(200)
>+      ->json_is( '/query' =>
>+          { author => { like => '%Orson%' }, title => { like => '%Ender%' } } );
>+
>+    # _match => starts_with
>+    $t->get_ok('/build_query?_match=starts_with&title=Ender&author=Orson')
>+      ->status_is(200)
>+      ->json_is( '/query' =>
>+          { author => { like => 'Orson%' }, title => { like => 'Ender%' } } );
>+
>+    # _match => ends_with
>+    $t->get_ok('/build_query?_match=ends_with&title=Ender&author=Orson')
>+      ->status_is(200)
>+      ->json_is( '/query' =>
>+          { author => { like => '%Orson' }, title => { like => '%Ender' } } );
>+
>+    # _match => exact
>+    $t->get_ok('/build_query?_match=exact&title=Ender&author=Orson')
>+      ->status_is(200)
>+      ->json_is( '/query' => { author => 'Orson', title => 'Ender' } );
>+
>+    # _match => blah
>+    $t->get_ok('/build_query?_match=blah&title=Ender&author=Orson')
>+      ->status_is(400)
>+      ->json_is( '/exception_msg'  => 'Invalid value for _match param (blah)' )
>+      ->json_is( '/exception_type' => 'Koha::Exceptions::WrongParameter' );
>+
>+};
>diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t
>index cf261b2e2d..9c41d9f145 100644
>--- a/t/db_dependent/Koha/Objects.t
>+++ b/t/db_dependent/Koha/Objects.t
>@@ -19,7 +19,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 16;
>+use Test::More tests => 15;
> use Test::Exception;
> use Test::Warn;
> 
>@@ -239,66 +239,3 @@ subtest '->is_paged and ->pager tests' => sub {
> 
>     $schema->storage->txn_rollback;
> };
>-
>-subtest '_build_query_params_from_api' => sub {
>-
>-    plan tests => 5;
>-
>-    my $filtered_params = {
>-        title  => "Ender",
>-        author => "Orson"
>-    };
>-
>-    subtest '_match => contains' => sub {
>-        plan tests => 2;
>-        my $reserved_params = { _match => 'contains' };
>-        my $params = Koha::Objects::_build_query_params_from_api( $filtered_params, $reserved_params );
>-
>-        is_deeply( $params->{author}, { like => '%Orson%' } );
>-        is_deeply( $params->{title}, { like => '%Ender%' } );
>-    };
>-
>-    subtest '_match => starts_with' => sub {
>-        plan tests => 2;
>-        my $reserved_params = { _match => 'starts_with' };
>-        my $params = Koha::Objects::_build_query_params_from_api( $filtered_params, $reserved_params );
>-
>-        is_deeply( $params->{author}, { like => 'Orson%' } );
>-        is_deeply( $params->{title}, { like => 'Ender%' } );
>-    };
>-
>-    subtest '_match => ends_with' => sub {
>-        plan tests => 2;
>-        my $reserved_params = { _match => 'ends_with' };
>-        my $params = Koha::Objects::_build_query_params_from_api( $filtered_params, $reserved_params );
>-
>-        is_deeply( $params->{author}, { like => '%Orson' } );
>-        is_deeply( $params->{title}, { like => '%Ender' } );
>-    };
>-
>-    subtest '_match => exact' => sub {
>-        plan tests => 2;
>-        my $reserved_params = { _match => 'exact' };
>-        my $params = Koha::Objects::_build_query_params_from_api( $filtered_params, $reserved_params );
>-
>-        is( $params->{author}, 'Orson' );
>-        is( $params->{title},  'Ender' );
>-    };
>-
>-    subtest '_match => blah' => sub {
>-        plan tests => 2;
>-
>-        my $reserved_params = { _match => 'blah' };
>-        throws_ok {
>-            Koha::Objects::_build_query_params_from_api( $filtered_params,
>-                $reserved_params );
>-        }
>-        'Koha::Exceptions::WrongParameter',
>-          'Exception thrown on invalid _match parameter';
>-
>-        is( $@,
>-            'Invalid value for _match param (blah)',
>-            'Exception carries the right message'
>-        );
>-    };
>-};
>diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t
>index 6d305f546d..46eed8fd65 100644
>--- a/t/db_dependent/Koha/REST/Plugin/Objects.t
>+++ b/t/db_dependent/Koha/REST/Plugin/Objects.t
>@@ -31,12 +31,11 @@ get '/patrons' => sub {
>     my $c = shift;
>     $c->validation->output($c->req->params->to_hash);
>     my $patrons = $c->objects->search(Koha::Patrons->new);
>-    $c->render( status => 200, json => $patrons->TO_JSON );
>+    $c->render( status => 200, json => $patrons );
> };
> 
> 
> # The tests
>-
> use Test::More tests => 1;
> use Test::Mojo;
> 
>@@ -44,34 +43,83 @@ use t::lib::TestBuilder;
> use Koha::Database;
> 
> my $schema = Koha::Database->new()->schema();
>-$schema->storage->txn_begin();
>+
> 
> my $builder = t::lib::TestBuilder->new;
>-$builder->build({
>-    source => 'Borrower',
>-    value => {
>-        firstname => 'Manuel',
>-    },
>-});
>-$builder->build({
>-    source => 'Borrower',
>-    value => {
>-        firstname => 'Manuel',
>-    },
>-});
> 
> subtest 'objects.search helper' => sub {
> 
>-    plan tests => 6;
>+    plan tests => 34;
> 
>     my $t = Test::Mojo->new;
> 
>-    $t->get_ok('/patrons?firstname=Manuel&_per_page=1&_page=1')
>+    $schema->storage->txn_begin;
>+
>+    # Delete existing patrons
>+    Koha::Patrons->search->delete;
>+    # Create two sample patrons that match the query
>+    $builder->build_object({
>+        class => 'Koha::Patrons',
>+        value => {
>+            firstname => 'Manuel'
>+        }
>+    });
>+    $builder->build_object({
>+        class => 'Koha::Patrons',
>+        value => {
>+            firstname => 'Manuela'
>+        }
>+    });
>+
>+    $t->get_ok('/patrons?firstname=manuel&_per_page=1&_page=1')
>         ->status_is(200)
>         ->header_like( 'Link' => qr/<http:\/\/.*\?.*&_page=2.*>; rel="next",/ )
>         ->json_has('/0')
>         ->json_hasnt('/1')
>         ->json_is('/0/firstname' => 'Manuel');
>-};
> 
>-$schema->storage->txn_rollback();
>+    $builder->build_object({
>+        class => 'Koha::Patrons',
>+        value => {
>+            firstname => 'Emanuel'
>+        }
>+    });
>+
>+    # _match=starts_with
>+    $t->get_ok('/patrons?firstname=manuel&_per_page=3&_page=1&_match=starts_with')
>+        ->status_is(200)
>+        ->json_has('/0')
>+        ->json_has('/1')
>+        ->json_hasnt('/2')
>+        ->json_is('/0/firstname' => 'Manuel')
>+        ->json_is('/1/firstname' => 'Manuela');
>+
>+    # _match=ends_with
>+    $t->get_ok('/patrons?firstname=manuel&_per_page=3&_page=1&_match=ends_with')
>+        ->status_is(200)
>+        ->json_has('/0')
>+        ->json_has('/1')
>+        ->json_hasnt('/2')
>+        ->json_is('/0/firstname' => 'Manuel')
>+        ->json_is('/1/firstname' => 'Emanuel');
>+
>+    # _match=exact
>+    $t->get_ok('/patrons?firstname=manuel&_per_page=3&_page=1&_match=exact')
>+        ->status_is(200)
>+        ->json_has('/0')
>+        ->json_hasnt('/1')
>+        ->json_is('/0/firstname' => 'Manuel');
>+
>+    # _match=contains
>+    $t->get_ok('/patrons?firstname=manuel&_per_page=3&_page=1&_match=contains')
>+        ->status_is(200)
>+        ->json_has('/0')
>+        ->json_has('/1')
>+        ->json_has('/2')
>+        ->json_hasnt('/3')
>+        ->json_is('/0/firstname' => 'Manuel')
>+        ->json_is('/1/firstname' => 'Manuela')
>+        ->json_is('/2/firstname' => 'Emanuel');
>+
>+    $schema->storage->txn_rollback;
>+};
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 19410: 67613 | 67731 | 67732 | 67750 | 67751 | 67795 | 67798 | 69248 | 69253 | 69254 | 69255 | 69256 | 69257 | 69258 | 69259 | 69260 | 69261 | 69262 | 69282 | 69283 | 69284 | 69286 | 69287 | 69385 | 69386 | 69387 | 69388 | 69389 | 69397 | 69398 | 69399 | 69400 | 69401 | 69545 | 69703