From 042fd6b92d43f895cc298b7f402618cfe9ce5aa4 Mon Sep 17 00:00:00 2001
From: Aleisha Amohia <aleishaamohia@hotmail.com>
Date: Mon, 12 Jun 2017 23:51:25 +0000
Subject: [PATCH] Bug 15326: [FOLLOW-UP] Many fixes

This patch:
- moves action buttons into a dropdown, adds a delete button to dropdown
- fixes the tests (they should now all pass)
- fixes qa tool problems
- removes the Koha::CmsPages->remove() method (unnecessary)
- makes changes to the Koha::CmsPages->add() method (now simpler, easier
to test)

This is now ready to test.

Sponsored-by: Catalyst IT
---
 Koha/CmsPages.pm                                   |  67 ++++-------
 .../prog/en/modules/tools/cmspages.tt              |  43 ++++---
 t/db_dependent/Pages.t                             | 123 ++++++++++++---------
 tools/cmspages.pl                                  |  22 +++-
 4 files changed, 145 insertions(+), 110 deletions(-)

diff --git a/Koha/CmsPages.pm b/Koha/CmsPages.pm
index 85a612f..a5ed1ea 100644
--- a/Koha/CmsPages.pm
+++ b/Koha/CmsPages.pm
@@ -49,49 +49,45 @@ sub object_class {
 }
 
 =head3 add
+
 Adds a new page to the CMS database based on the input data.
+
 =cut
 
 sub add {
     my ( $self, $input ) = @_;
     carp( 'Koha::CmsPages::add() undefined parameter: input' ) unless defined $input;
+
     my $data = {
-        parent     =>   scalar          $input->param( 'parent' ) eq '' ? undef : scalar $input->param( 'parent' ),
-        location   =>   scalar          $input->param( 'disp' )   eq '' ? undef : scalar $input->param( 'disp'),
-        branchcode =>   scalar          $input->param( 'branch' ) eq '' ? undef : scalar $input->param( 'branch' ),
-        lang       =>   scalar          $input->param( 'lang' ),
-        sortorder  =>   scalar          $input->param( 'number' ),
-        title      =>   scalar          $input->param( 'title' ),
-        title_link =>   scalar          $input->param( 'title_link' ),
-        publish    =>   defined (scalar $input->param( 'publish' )),
-        content    =>   scalar          $input->param( 'content' )
+        parent     => $input->{parent},
+        location   => $input->{location},
+        branchcode => $input->{branchcode},
+        lang       => $input->{lang},
+        sortorder  => $input->{sortorder},
+        title      => $input->{title},
+        title_link => $input->{title_link},
+        publish    => $input->{publish},
+        content    => $input->{content}
     };
 
-    if (defined $input->param( 'id' )) {
-        $data->{'id'} = scalar $input->param( 'id' );
+    if (defined $input->{id}){
+        $data->{id} = $input->{id};
     }
 
     # Ensuring link titles are unique
-    my $title_link = $input->param('title_link');
-    if ( Koha::CmsPages->find({ title_link => $title_link }) ) {
-        my $existing_link = Koha::CmsPages->find({ title_link => $title_link });
-        my $link = $existing_link->unblessed;
-        unless ( $link->{'id'} eq $input->param('id') ) {
-            return undef;
+    my $title_link = $input->{title_link};
+    if ( Koha::CmsPages->find({ title_link => $title_link }) ){
+        my $link = Koha::CmsPages->find({ title_link => $title_link });
+        unless ( $link->id eq $input->{id} ){
+            return;
         }
     }
 
     # Ensuring that, if page has parent, provided location suits parent location
-    my $parent = $input->param('parent');
-    my $parent_obj = Koha::CmsPages->find($parent);
-    if ( $parent_obj ) {
-        my $parent_unblessed = $parent_obj->unblessed;
-        if ( $parent_unblessed->{'location'} ne "" ) {
-            # if parent location is not all interfaces
-            if ( $parent_unblessed->{'location'} != $input->param('disp') ) {
-                # if parent location is not the same as provided location
-                return undef;
-            }
+    my $parent = Koha::CmsPages->find($input->{parent});
+    if ( $parent && $parent->location ne "" ){
+        if ( $parent->location ne $input->{location} ){
+            return;
         }
     }
 
@@ -102,7 +98,7 @@ sub add {
 
 =head3 list
 
-Lists pages based input filtering paramters for location, branch and language.
+Lists pages based input filtering parameters for location, branch and language.
 
 =cut
 
@@ -199,21 +195,6 @@ sub child_links {
     return $rs;
 }
 
-=head3 remove
-
-Removes a page identified by id from the CMS database.
-
-=cut
-
-sub remove {
-    my ( $self, @ids ) = @_;
-
-    carp( 'Koha::CmsPages::remove() undefined parameter ids' ) unless @ids;
-
-    my $pages = $self->_resultset()->search({ id => \@ids });
-    $pages->delete;
-}
-
 =head1 AUTHOR
 
 Martin Persson <xarragon@gmail.com>
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cmspages.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cmspages.tt
index 901c94d..43dac1a 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cmspages.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cmspages.tt
@@ -9,8 +9,6 @@
 [% INCLUDE 'datatables.inc' %]
 <script type="text/javascript" src="[% interface %]/lib/tiny_mce/tiny_mce.js"></script>
 <script type="text/javascript">//<![CDATA[
-var MSG_CONFIRM_DELETE_PAGE  = _("Are you sure you want to delete the selected page(s)?");
-
 $(document).ready(function() {
     $("#pages_table").dataTable($.extend(true, {}, dataTablesDefaults, {
         "aoColumnDefs": [
@@ -20,7 +18,16 @@ $(document).ready(function() {
     }));
 
     $("#del_button").on("click",function(){
-        return confirmDelete(MSG_CONFIRM_DELETE_PAGE);
+        if ($("input[name='ids'][type='checkbox']:checked").length){
+            return confirmDelete(_("Are you sure you want to delete the selected page(s)?"));
+        } else {
+            alert(_("No pages have been selected."));
+            return false;
+        }
+    });
+
+    $(".del_single").on("click", function(){
+        return confirmDelete(_("Are you sure you want to delete this page?"));
     });
 
     $(".SelectAll").on("click", function(){
@@ -89,7 +96,7 @@ tinyMCE.init({
 [% END %]
 [% UNLESS ( form || view ) %]
 <div id="toolbar" class="btn-toolbar">
-    <a class="btn btn-small" id="newentry" href="[% link_self %]?op=form"><i class="fa fa-plus"></i> New page</a>
+    <a class="btn btn-default btn-sm" id="newentry" href="[% link_self %]?op=form"><i class="fa fa-plus"></i> New page</a>
 </div>
 [% END %]
 [% IF ( failed_add ) %]
@@ -252,7 +259,7 @@ tinyMCE.init({
                         <th>Number</th>
                         <th>Link title</th>
                         <th>Page title</th>
-                        <th>Actions</th>
+                        <th>&nbsp;</th>
                     </tr></thead>
                     <tbody>[% FOREACH page IN page_list %]
                         <tr>
@@ -278,15 +285,23 @@ tinyMCE.init({
                             <td><a href="[% link_self %]?op=form&amp;id=[% page.id %]">[% page.title_link %]</a></td>
                             <td>[% page.title %]</td>
                             <td class="actions">
-                                <a href="[% link_self %]?op=form&amp;id=[% page.id %]" class="btn btn-mini"><i class="fa fa-pencil"></i> Edit</a>
-                                [% IF ( page.location == "" ) %]
-                                    <a href="[% link_opac %]?id=[% page.id %]" class="btn btn-mini"><i class="fa fa-eye"></i> OPAC</a>
-                                    <a href="[% link_self %]?op=view&amp;id=[% page.id %]" class="btn btn-mini"><i class="fa fa-eye"></i> Intranet</a>
-                                [% ELSIF ( page.location == "1" ) %]
-                                    <a href="[% link_self %]?op=view&amp;id=[% page.id %]" class="btn btn-mini"><i class="fa fa-eye"></i> Intranet</a>
-                                [% ELSIF ( page.location == "2" ) %]
-                                    <a href="[% link_opac %]?id=[% page.id %]" class="btn btn-mini"><i class="fa fa-eye"></i> OPAC</a>
-                                [% END %]
+                                <div class="btn-group">
+                                    <a class="btn btn-default btn-xs dropdown-toggle" id="opencmspageactions[% page.id %]" role="button" data-toggle="dropdown" href="#">
+                                        Actions <b class="caret"></b></a>
+                                    <ul class="dropdown-menu pull-right" role="menu" aria-labelledby="opencmspageactions[% page.id %]">
+                                        <li><a href="[% link_self %]?op=form&amp;id=[% page.id %]"><i class="fa fa-pencil"></i> Edit</a></li>
+                                        <li><a href="[% link_self %]?op=delSingle&amp;id=[% page.id %]" class="del_single"><i class="fa fa-trash"></i> Delete</a></li>
+                                        [% IF ( page.location == "" ) %]
+                                            <li><a href="[% link_opac %]?id=[% page.id %]"><i class="fa fa-eye"></i> OPAC</a></li>
+                                            <li><a href="[% link_self %]?op=view&amp;id=[% page.id %]"><i class="fa fa-eye"></i> Intranet</a></li>
+                                        [% ELSIF ( page.location == "1" ) %]
+                                            <li><a href="[% link_self %]?op=view&amp;id=[% page.id %]"><i class="fa fa-eye"></i> Intranet</a></li>
+                                        [% ELSIF ( page.location == "2" ) %]
+                                            <li><a href="[% link_opac %]?id=[% page.id %]"><i class="fa fa-eye"></i> OPAC</a></li>
+                                        [% END %]
+                                    </ul>
+                                </div>
+                            </td>
                         </tr>
                     [% END %]</tbody>
                 </table>
diff --git a/t/db_dependent/Pages.t b/t/db_dependent/Pages.t
index 2c98c38..bad7477 100644
--- a/t/db_dependent/Pages.t
+++ b/t/db_dependent/Pages.t
@@ -18,75 +18,96 @@
 
 use Modern::Perl;
 
-use Test::More tests => 2;
+use Test::More tests => 10;
 use Test::Warn;
-use Data::Dumper::Concise;
 
 use C4::Context;
 use Koha::Database;
 use t::lib::TestBuilder;
 
 BEGIN {
-    use_ok('Koha::Objects');
     use_ok('Koha::CmsPages');
+    use_ok('Koha::CmsPage');
 }
 
-my $builder = t::lib::TestBuilder->new();
+my $database = Koha::Database->new();
+my $schema   = $database->schema();
+my $dbh      = C4::Context->dbh;
+my $builder  = t::lib::TestBuilder->new();
 
-# Start transaction
-my $dbh = C4::Context->dbh;
-$dbh->{ AutoCommit } = 0;
-$dbh->{ RaiseError } = 1;
-$dbh->do( 'DELETE FROM items' );
+$schema->storage->txn_begin();
+$dbh->do( 'DELETE FROM cms_pages' );
+$dbh->do( 'DELETE FROM issues' );
 $dbh->do( 'DELETE FROM borrowers' );
+$dbh->do( 'DELETE FROM items' );
 $dbh->do( 'DELETE FROM branches' );
-$dbh->do( 'DELETE FROM cms_pages' );
 
-my $branchcode = $builder->build({ source => 'Branch' });
-
-my $page10 = $builder->build({
-	source => 'CmsPage',
-	value  => {
-		parent     => undef,
-		title_link => 'page 10',
-		title      => 'full page 10',
-		published  => 1,
-		sortorder  => 1,
-	}
-});
-my $page11 = $builder->build({
-	source => 'CmsPage',
-	value  => {
-		parent     => $page10->{'id'},
-		title_link => 'subpage 11',
-		title      => 'full subpage 11',
-		published  => 1,
-		sortorder  => 0,
-	}
+my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'};
+my $borrowernumber = $builder->build({ source => 'Borrower' })->{'borrowernumber'};
+
+# new parent page
+my $parent_page = Koha::CmsPages->add({
+    id => 1,
+    branchcode => $branchcode,
+    location => 2,
+    lang => '',
+    parent => undef,
+    title_link => 'page1',
+    title => 'Page 1',
+    publish => 0,
+    sortorder => 1,
+    content => "This is the first page.",
 });
-my $page12 = $builder->build({
-	source => 'CmsPage',
-	value  => {
-		parent     => $page10->{'id'},
-		title_link => 'subpage 12',
-		title      => 'full subpage 12',
-		published  => 1,
-		sortorder  => 1,
-	}
+
+is($parent_page->title, 'Page 1', "First page created");
+
+# new child page
+my $child_page = Koha::CmsPages->add({
+    id => 2,
+    branchcode => $branchcode,
+    location => 2,
+    lang => '',
+    parent => $parent_page->id,
+    title_link => 'page2',
+    title => 'Page 2',
+    publish => 1,
+    sortorder => 2,
+    content => "This is the second page.",
 });
-my $page20 = $builder->build({
-	source => 'CmsPage',
-	value  => {
-		parent     => undef,
-		title_link => 'page 20',
-		title      => 'full page 20',
-		published  => 0,
-		sortorder  => 2,
-	}
+
+# $page->parent specifies who this page is the parent of
+is(Koha::CmsPages->find($child_page->parent)->title, 'Page 1', 'Child and parent page connected');
+
+# Koha::CmsPages->list() with no specified ID should return all pages
+my ( $page_list, $parent_list, $data ) = Koha::CmsPages->list( '' );
+is(scalar @{ $page_list }, 2, "Two pages have been created");
+
+# Koha::CmsPages->list() is used for filtering (filter display location)
+( $page_list, $parent_list, $data ) = Koha::CmsPages->list( '', 2, '', '' );
+is(scalar @{ $page_list }, 2, "Both pages can be seen on staff interface");
+
+is($parent_list->[0]->{id}, $parent_page->id, "Correct page is returned as a parent");
+is($data, undef, "Data is undefined because no ID was given");
+
+# Koha::CmsPages->list() is used to show data about a page when given an ID
+( $page_list, $parent_list, $data ) = Koha::CmsPages->list( 2 );
+is($data->{content}, "This is the second page.", "Data is defined when given an ID");
+
+# Try to add a child page under a page where their locations do not match up
+my $test_page = Koha::CmsPages->add({
+    id => 3,
+    branchcode => $branchcode,
+    location => 1,
+    lang => '',
+    parent => $child_page->id,
+    title_link => 'failedadd',
+    title => 'Failed add',
+    publish => 0,
+    sortorder => 3,
+    content => "This page should not be added",
 });
 
-my $result = Koha::CmsPages->child_links( undef );
-is_deeply( $result, [ $page10, $page20 ]);
+is($test_page, undef, "Child page cannot be added under a page if their locations do not match");
 
 $dbh->rollback();
 
diff --git a/tools/cmspages.pl b/tools/cmspages.pl
index 846f5e1..ccacc3a 100755
--- a/tools/cmspages.pl
+++ b/tools/cmspages.pl
@@ -25,6 +25,7 @@ use C4::Output;
 use C4::Languages qw( getTranslatedLanguages );
 use Koha::CmsPages;
 use Koha::Libraries;
+use Koha::CmsPage;
 
 my $link_self = '/cgi-bin/koha/tools/cmspages.pl';
 
@@ -72,16 +73,33 @@ if ( $op eq 'form' ) {
         op   => 'add'
     );
 } elsif ( $op eq 'add' ) {
-    my $success = Koha::CmsPages->add( $cgi );
+    my $success = Koha::CmsPages->add({
+        id         => scalar $cgi->param('id'),
+        parent     => scalar $cgi->param('parent'),
+        location   => scalar $cgi->param('disp'),
+        branchcode => scalar $cgi->param('branch'),
+        lang       => scalar $cgi->param('lang'),
+        sortorder  => scalar $cgi->param('number'),
+        title      => scalar $cgi->param('title'),
+        title_link => scalar $cgi->param('title_link'),
+        publish    => scalar $cgi->param('publish'),
+        content    => scalar $cgi->param('content'),
+    });
     unless ($success) {
         $template->param( failed_add => 1 );
     }
     if ($success) {
         print $cgi->redirect( $link_self );
     }
+} elsif ( $op eq 'delSingle' ) {
+    Koha::CmsPages->find( $id )->delete();
+    print $cgi->redirect( $link_self );
 } elsif ( $op eq 'del' ) {
     # param => multi_param; no list-ctx warnings
-    Koha::CmsPages->remove( $cgi->multi_param( 'ids' ) );
+    my @ids = $cgi->multi_param('ids');
+    foreach (@ids){
+        Koha::CmsPages->find($_)->delete;
+    }
     print $cgi->redirect( $link_self );
 } elsif ( $op eq 'view' ) {
     my $cms_data = Koha::CmsPages->page( $id );
-- 
2.1.4