Attachment 43194 Details for Bug 14974 – Bug 14974: Joubu's try to use the REST API for cities

[patch] Bug 14974: Joubu's try to use the REST API for cities

Bug-14974-Joubus-try-to-use-the-REST-API-for-citie.patch (text/plain), 19.86 KB, created by Jonathan Druart on 2015-10-07 16:09:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2015-10-07 16:09:26 UTC

Size: 19.86 KB

patch

obsolete

>From 47d86aab9bce62e24071713a4bd3662a0e3c67a5 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Wed, 7 Oct 2015 16:53:24 +0100
>Subject: [PATCH] Bug 14974: Joubu's try to use the REST API for cities
>
>Here's my try.
>The naive way is to do AJAX calls and redirect to the list after
>delete/update/add.
>
>The main issue is that it does not simplify the code so far...
>
>Question:
>How to display notif to the user after actions (delete/update/add)?
>How to simplify the swagger file?
>How to present a search interface?
>
>See other comments in the code.
>---
> Koha/REST/V1/Cities.pm                             |  98 +++++++++++
> admin/cities.pl                                    |  53 +-----
> api/v1/swagger.json                                | 192 +++++++++++++++++++++
> .../intranet-tmpl/prog/en/modules/admin/cities.tt  | 176 ++++++++++++-------
> 4 files changed, 406 insertions(+), 113 deletions(-)
> create mode 100644 Koha/REST/V1/Cities.pm
>
>diff --git a/Koha/REST/V1/Cities.pm b/Koha/REST/V1/Cities.pm
>new file mode 100644
>index 0000000..944eb75
>--- /dev/null
>+++ b/Koha/REST/V1/Cities.pm
>@@ -0,0 +1,98 @@
>+package Koha::REST::V1::Cities;
>+
>+# This file is part of Koha.
>+#
>+# Koha is free software; you can redistribute it and/or modify it under the
>+# terms of the GNU General Public License as published by the Free Software
>+# Foundation; either version 3 of the License, or (at your option) any later
>+# version.
>+#
>+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY
>+# 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.
>+
>+use Modern::Perl;
>+
>+use Mojo::Base 'Mojolicious::Controller';
>+
>+use C4::Auth qw( haspermission );
>+use Koha::City;
>+use Koha::Cities;
>+
>+sub list_cities {
>+    my ($c, $args, $cb) = @_;
>+
>+    my $user = $c->stash('koha.user');
>+    # Check permissions here
>+
>+    # Catch possible errors here
>+    my $cities = Koha::Cities->search(
>+         {
>+            map { $args->{$_} ? ( $_ => $args->{$_} ): () } keys %$args
>+         }
>+    );
>+
>+    return $c->$cb($cities->unblessed, 200);
>+}
>+
>+sub get_city {
>+    my ($c, $args, $cb) = @_;
>+
>+    my $user = $c->stash('koha.user');
>+    # Check permissions here
>+
>+    # Catch possible errors here
>+    my $city = Koha::Cities->find($args->{cityid});
>+    unless ($city) {
>+        return $c->$cb({error => "City not found"}, 404);
>+    }
>+
>+    return $c->$cb($city->unblessed, 200);
>+}
>+
>+sub add_city {
>+    my ($c, $args, $cb) = @_;
>+
>+    my $user = $c->stash('koha.user');
>+    # Check permissions here
>+
>+    # Catch possible errors here
>+    my $city = Koha::City->new($args->{city});
>+    $city->store;
>+
>+    return $c->$cb($city->unblessed, 200);
>+}
>+
>+sub update_city {
>+    my ($c, $args, $cb) = @_;
>+
>+    my $user = $c->stash('koha.user');
>+    # Check permissions here
>+
>+    # Catch possible errors here
>+    my $city = Koha::Cities->find($args->{city}{cityid});
>+    while ( my ( $k, $v ) = each %{$args->{city}} ) {
>+        $city->$k($v);
>+    }
>+    $city->store;
>+
>+    return $c->$cb($city->unblessed, 200);
>+}
>+
>+sub delete_city {
>+    my ($c, $args, $cb) = @_;
>+
>+    my $user = $c->stash('koha.user');
>+    # Check permissions here
>+
>+    # Catch possible errors here
>+    my $city = Koha::Cities->find($args->{cityid})->delete;
>+
>+    return $c->$cb("", 200);
>+}
>+
>+1;
>diff --git a/admin/cities.pl b/admin/cities.pl
>index 4a6247c..e8abeec 100755
>--- a/admin/cities.pl
>+++ b/admin/cities.pl
>@@ -30,7 +30,7 @@ my $input       = new CGI;
> my $searchfield = $input->param('city_name') // q||;
> my $cityid      = $input->param('cityid');
> my $op          = $input->param('op') || 'list';
>-my @messages;
>+my $message     = $input->param('message');
> 
> my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
>     {   template_name   => "admin/cities.tt",
>@@ -50,55 +50,6 @@ if ( $op eq 'add_form' ) {
>     }
> 
>     $template->param( city => $city, );
>-} elsif ( $op eq 'add_validate' ) {
>-    my $cityid       = $input->param('cityid');
>-    my $city_name    = $input->param('city_name');
>-    my $city_state   = $input->param('city_state');
>-    my $city_zipcode = $input->param('city_zipcode');
>-    my $city_country = $input->param('city_country');
>-
>-    if ($cityid) {
>-        my $city = Koha::Cities->find($cityid);
>-        $city->city_name($city_name);
>-        $city->city_state($city_state);
>-        $city->city_zipcode($city_zipcode);
>-        $city->city_country($city_country);
>-        eval { $city->store; };
>-        if ($@) {
>-            push @messages, { type => 'error', code => 'error_on_update' };
>-        } else {
>-            push @messages, { type => 'message', code => 'success_on_update' };
>-        }
>-    } else {
>-        my $city = Koha::City->new(
>-            {   city_name    => $city_name,
>-                city_state   => $city_state,
>-                city_zipcode => $city_zipcode,
>-                city_country => $city_country,
>-            }
>-        );
>-        eval { $city->store; };
>-        if ($@) {
>-            push @messages, { type => 'error', code => 'error_on_insert' };
>-        } else {
>-            push @messages, { type => 'message', code => 'success_on_insert' };
>-        }
>-    }
>-    $searchfield = q||;
>-    $op          = 'list';
>-} elsif ( $op eq 'delete_confirm' ) {
>-    my $city = Koha::Cities->find($cityid);
>-    $template->param( city => $city, );
>-} elsif ( $op eq 'delete_confirmed' ) {
>-    my $city = Koha::Cities->find($cityid);
>-    my $deleted = eval { $city->delete; };
>-
>-    if ( $@ or not $deleted ) {
>-        push @messages, { type => 'error', code => 'error_on_delete' };
>-    } else {
>-        push @messages, { type => 'message', code => 'success_on_delete' };
>-    }
>-    $op = 'list';
> }
> 
> if ( $op eq 'list' ) {
>@@ -109,7 +60,7 @@ if ( $op eq 'list' ) {
> $template->param(
>     cityid      => $cityid,
>     searchfield => $searchfield,
>-    messages    => \@messages,
>+    message     => $message,
>     op          => $op,
> );
> 
>diff --git a/api/v1/swagger.json b/api/v1/swagger.json
>index a20cb20..4290852 100644
>--- a/api/v1/swagger.json
>+++ b/api/v1/swagger.json
>@@ -75,6 +75,161 @@
>           }
>         }
>       }
>+    },
>+    "/cities": {
>+      "get": {
>+        "x-mojo-controller": "Koha::REST::V1::Cities",
>+        "operationId": "listCities",
>+        "tags": ["cities"],
>+        "produces": [
>+          "application/json"
>+        ],
>+        "responses": {
>+          "200": {
>+            "description": "A list of cities",
>+            "schema": {
>+              "type": "array",
>+              "items": {
>+                "$ref": "#/definitions/city"
>+              }
>+            }
>+          },
>+          "403": {
>+            "description": "Access forbidden",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          }
>+        }
>+      },
>+      "put": {
>+        "x-mojo-controller": "Koha::REST::V1::Cities",
>+        "operationId": "AddCity",
>+        "tags": ["cities"],
>+        "parameters": [
>+          {
>+            "$ref": "#/parameters/cityBodyParam"
>+          }
>+        ],
>+        "produces": [
>+          "application/json"
>+        ],
>+        "responses": {
>+          "200": {
>+            "description": "City added",
>+            "schema": {
>+              "$ref": "#/definitions/city"
>+            }
>+          },
>+          "403": {
>+            "description": "Access forbidden",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          }
>+        }
>+      }
>+    },
>+    "/cities/{cityid}": {
>+      "get": {
>+        "x-mojo-controller": "Koha::REST::V1::Cities",
>+        "operationId": "getCity",
>+        "tags": ["cities"],
>+        "parameters": [
>+          {
>+            "$ref": "#/parameters/cityidPathParam"
>+          }
>+        ],
>+        "produces": [
>+          "application/json"
>+        ],
>+        "responses": {
>+          "200": {
>+            "description": "A city",
>+            "schema": {
>+              "$ref": "#/definitions/city"
>+            }
>+          },
>+          "403": {
>+            "description": "Access forbidden",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          },
>+          "404": {
>+            "description": "City not found",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          }
>+        }
>+      },
>+      "put": {
>+        "x-mojo-controller": "Koha::REST::V1::Cities",
>+        "operationId": "updateCity",
>+        "tags": ["cities"],
>+        "parameters": [
>+          {
>+            "$ref": "#/parameters/cityBodyParam"
>+          }
>+        ],
>+        "produces": [
>+          "application/json"
>+        ],
>+        "responses": {
>+          "200": {
>+            "description": "A city",
>+            "schema": {
>+              "$ref": "#/definitions/city"
>+            }
>+          },
>+          "403": {
>+            "description": "Access forbidden",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          },
>+          "404": {
>+            "description": "City not found",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          }
>+        }
>+      },
>+      "delete": {
>+        "x-mojo-controller": "Koha::REST::V1::Cities",
>+        "operationId": "deleteCity",
>+        "tags": ["cities"],
>+        "parameters": [
>+          {
>+            "$ref": "#/parameters/cityidPathParam"
>+          }
>+        ],
>+        "produces": [
>+          "application/json"
>+        ],
>+        "responses": {
>+          "200": {
>+            "description": "City deleted",
>+            "schema": {
>+              "type": "string"
>+            }
>+          },
>+          "403": {
>+            "description": "Access forbidden",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          },
>+          "404": {
>+            "description": "City not found",
>+            "schema": {
>+              "$ref": "#/definitions/error"
>+            }
>+          }
>+        }
>+      }
>     }
>   },
>   "definitions": {
>@@ -106,6 +261,29 @@
>           "type": "string"
>         }
>       }
>+    },
>+    "city": {
>+      "type": "object",
>+      "properties": {
>+        "cityid": {
>+          "$ref": "#/definitions/cityid"
>+        },
>+        "city_name": {
>+          "description": "city name"
>+        },
>+        "city_state": {
>+          "description": "city state"
>+        },
>+        "city_zipcode": {
>+          "description": "city zipcode"
>+        },
>+        "city_country": {
>+          "description": "city country"
>+        }
>+      }
>+    },
>+    "cityid": {
>+      "description": "city id"
>     }
>   },
>   "parameters": {
>@@ -115,6 +293,20 @@
>       "description": "Internal borrower identifier",
>       "required": "true",
>       "type": "integer"
>+    },
>+    "cityidPathParam": {
>+      "name": "cityid",
>+      "in": "path",
>+      "description": "City id",
>+      "required": "true",
>+      "type": "integer"
>+    },
>+    "cityBodyParam": {
>+      "name": "city",
>+      "in": "body",
>+      "description": "A JSON object representing a city",
>+      "required": "true",
>+      "schema": { "$ref": "#/definitions/city" }
>     }
>   }
> }
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/cities.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/cities.tt
>index facb119..2ffab05 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/cities.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/cities.tt
>@@ -5,16 +5,112 @@
> [% INCLUDE 'datatables.inc' %]
> <script type="text/javascript">
> //<![CDATA[
>+    var aDataSet = [];
>     $(document).ready(function() {
>-        $("#table_cities").dataTable($.extend(true, {}, dataTablesDefaults, {
>-            "aoColumnDefs": [
>-                { "aTargets": [ -1, -2 ], "bSortable": false, "bSearchable": false },
>-            ],
>-            "aaSorting": [[ 1, "asc" ]],
>-            "iDisplayLength": 10,
>-            "sPaginationType": "full_numbers"
>+
>+        var dtCityResults = $("#table_cities").dataTable($.extend(true, {}, dataTablesDefaults, {
>+            'bServerSide': true,
>+            'sAjaxSource': "/api/v1/cities",
>+            'fnServerData': function(sSource, aoData, fnCallback) {
>+                var echo = aoData[0].value;
>+                $.ajax({
>+                    dataType: 'json',
>+                    type: 'GET',
>+                    url: sSource,
>+                    //data: aoData,
>+                    success: function(data){
>+                        aDataSet = [];
>+                        var json = {};
>+                        $.each(data, function() {
>+                            aDataSet.push([
>+                                this.cityid,
>+                                this.city_name,
>+                                this.city_state,
>+                                this.city_zipcode,
>+                                this.city_country,
>+                                "<a href='/cgi-bin/koha/admin/cities.pl?op=add_form&amp;cityid=" + this.cityid + "'>" + _("Edit") + "</a>",
>+                                "<a data-cityid='" + this.cityid + "' href='#'>" + _("Delete") + "</a>"
>+                            ]);
>+                        });
>+                        json.iTotalRecords = data.length;
>+                        json.iTotalDisplayRecords = data.length;
>+                        json.sEcho = echo;
>+                        json.aaData = aDataSet;
>+                        fnCallback(json);
>+                    }
>+                });
>+            },
>+            'fnRowCallback': function(nRow, aData, iDisplayIndex, iDisplayIndexFull) {
>+                $("td:eq(6)", nRow).find('a').on('click', function() {
>+                    return delete_city($(this).attr('data-cityid'));
>+                });
>+            },
>+            // Should add filter/search/pagination
>+            // Not supported yet
>+            'bAutoWidth': false,
>+            'bPaginate': false,
>+            'bSort': false,
>+            'bFilter': false
>         }));
>+
>+        $("#add_city").on('submit', function(e){
>+            e.preventDefault();
>+            var city = {
>+                cityid: $("#cityid").val(),
>+                city_name: $("#city_name").val(),
>+                city_state: $("#city_state").val(),
>+                city_zipcode: $("#city_zipcode").val(),
>+                city_country: $("#city_country").val()
>+            };
>+            // Double check on required fields should be avoided
>+            if ( !city.city_name || !city.city_zipcode ) return;
>+            cityid ? update_city(city) : add_city(city);
>+        });
>     });
>+
>+    // The 3 functions should be extracted somewhere else
>+    function update_city(city) {
>+        $.ajax({
>+            type: 'PUT',
>+            url: '/api/v1/cities/' + city.cityid,
>+            data: JSON.stringify(city),
>+            success: function(data){
>+                // Need a better way to redirect, we would like to inform the user that the action succeeded
>+                window.location.href='/cgi-bin/koha/admin/cities.pl';
>+            },
>+            error: function(data){
>+                // Need a better way to inform the user that the action has failed
>+                alert(data);
>+            }
>+        });
>+    }
>+    function add_city(city) {
>+        $.ajax({
>+            type: 'PUT',
>+            url: '/api/v1/cities',
>+            data: JSON.stringify(city),
>+            success: function(data){
>+                window.location.href='/cgi-bin/koha/admin/cities.pl';
>+            },
>+            error: function(data){
>+                alert(data);
>+            }
>+        });
>+    }
>+    function delete_city(cityid) {
>+        if ( confirm(_("Are you sure you want to delete this city?")) ) {
>+            $.ajax({
>+                type: 'DELETE',
>+                url: '/api/v1/cities/' + cityid,
>+                success: function(data){
>+                    window.location.href='/cgi-bin/koha/admin/cities.pl';
>+                },
>+                error: function(data){
>+                    alert(data);
>+                }
>+            })
>+        }
>+    }
> //]]>
> </script>
> </head>
>@@ -39,21 +135,19 @@
>     <div id="yui-main">
>     <div class="yui-b">
> 
>-[% FOR m IN messages %]
>-    <div class="dialog [% m.type %]">
>-        [% SWITCH m.code %]
>-        [% CASE 'error_on_update' %]
>-            An error occurred when updating this city. Perhaps it already exists.
>+[% IF message %]
>+    <div class="dialog message">
>+    [% SWITCH message %]
>+        [% CASE 'updated' %]
>+            City updated successfully.
>+        [% CASE 'added' %]
>+            City added successfully.
>+        [% CASE 'deleted' %]
>+            City deleted successfully.
>         [% CASE 'error_on_insert' %]
>             An error occurred when adding this city. The city id might already exist.
>         [% CASE 'error_on_delete' %]
>             An error occurred when deleting this city. Check the logs.
>-        [% CASE 'success_on_update' %]
>-            City updated successfully.
>-        [% CASE 'success_on_insert' %]
>-            City added successfully.
>-        [% CASE 'success_on_delete' %]
>-            City deleted successfully.
>         [% CASE 'already_exists' %]
>             This city already exists.
>         [% CASE %]
>@@ -69,9 +163,9 @@
>         <h1>New city</h1>
>     [% END %]
> 
>-    <form action="/cgi-bin/koha/admin/cities.pl" name="Aform" method="post" class="validated">
>+    <form id="add_city" action="/cgi-bin/koha/admin/cities.pl" name="Aform" method="post" class="validated">
>         <input type="hidden" name="op" value="add_validate" />
>-        <input type="hidden" name="cityid" value="[% city.cityid %]" />
>+        <input type="hidden" name="cityid" id="cityid" value="[% city.cityid %]" />
> 
>         <fieldset class="rows">
>             <ol>
>@@ -104,37 +198,6 @@
>     </form>
> [% END %]
> 
>-[% IF op == 'delete_confirm' %]
>-    <div class="dialog alert">
>-        <h3>Delete City "[% city.city_name %]?"</h3>
>-        <table>
>-            <tr><th>City id</th>
>-                <td>[% city.cityid %]</td>
>-            </tr>
>-            <tr><th>City</th>
>-                <td>[% city.city_name %]</td>
>-            </tr>
>-            <tr><th>State</th>
>-                <td>[% city.city_state %]</td>
>-            </tr>
>-            <tr><th>Zip/Postal code</th>
>-                <td>[% city.city_zipcode %]</td>
>-            </tr>
>-            <tr><th>Country</th>
>-                <td>[% city.city_country %]</td>
>-            </tr>
>-        </table>
>-        <form action="/cgi-bin/koha/admin/cities.pl" method="post">
>-            <input type="hidden" name="op" value="delete_confirmed" />
>-            <input type="hidden" name="cityid" value="[% city.cityid %]" />
>-            <input type="submit" class="approve" value="Yes, delete" />
>-        </form>
>-        <form action="/cgi-bin/koha/admin/cities.pl" method="get">
>-            <input type="submit" class="deny" value="No, do not Delete" />
>-        </form>
>-    </div>
>-[% END %]
>-
> [% IF op == 'list' %]
> 
>     <div id="toolbar" class="btn-toolbar">
>@@ -158,17 +221,6 @@
>                 <th>&nbsp;</th>
>             </thead>
>             <tbody>
>-                [% FOREACH city IN cities %]
>-                <tr>
>-                    <td>[% city.cityid %]</td>
>-                    <td>[% city.city_name %]</td>
>-                    <td>[% city.city_state %]</td>
>-                    <td>[% city.city_zipcode %]</td>
>-                    <td>[% city.city_country %]</td>
>-                    <td><a href="/cgi-bin/koha/admin/cities.pl?op=add_form&amp;cityid=[% city.cityid %]">Edit</a></td>
>-                    <td><a href="/cgi-bin/koha/admin/cities.pl?op=delete_confirm&amp;cityid=[% city.cityid %]">Delete</a></td>
>-                </tr>
>-                [% END %]
>             </tbody>
>         </table>
>     [% ELSE %]
>-- 
>2.1.0

Actions: View | Diff | Splinter Review

Attachments on bug 14974: 43194 | 50022