Bug 15631: Koha::Cities - remove getidcity and GetCities

C4::Members::getidcity and C4::Members::GetCities simply retrieved
cities info from the cities table.
The job done in members/memberentry.pl looked really weird and complicated.
Either I have missed something, or this patch can simplify it.

The expected behavior is:
1. Create a new patron => No city selected
2. Edit an existing patron => The borrowers.city value is selected
3. Add a guarantee => The borrowers.city of the guarantor is selected
4. Edit a guarantee => The borrowers.city of the guarantee is selected

Test plan:
Confirm that the expected behaviors are the ones before and after this patch.

Signed-off-by: Natasha <tasham_8@hotmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Brendan Gallagher brendan@bywatersolutions.com

diff --git a/C4/Members.pm b/C4/Members.pm
index f758501..8a47566 100644 (file)
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -67,13 +67,10 @@ BEGIN {
         &GetPendingIssues
         &GetAllIssues
 
-        &getidcity
-
         &GetFirstValidEmailAddress
         &GetNoticeEmailAddress
 
         &GetAge
-        &GetCities
         &GetSortDetails
         &GetTitles
 
@@ -1362,21 +1359,6 @@ sub get_cardnumber_length {
     return ( $min, $max );
 }
 
-=head2 getdcity (OUEST-PROVENCE)
-
-recover cityid  with city_name condition
-
-=cut
-
-sub getidcity {
-    my ($city_name) = @_;
-    my $dbh = C4::Context->dbh;
-    my $sth = $dbh->prepare("select cityid from cities where city_name=? ");
-    $sth->execute($city_name);
-    my $data = $sth->fetchrow;
-    return $data;
-}
-
 =head2 GetFirstValidEmailAddress
 
   $email = GetFirstValidEmailAddress($borrowernumber);
@@ -1683,35 +1665,6 @@ sub SetAge{
     return $borrower;
 }    # sub SetAge
 
-=head2 GetCities
-
-  $cityarrayref = GetCities();
-
-  Returns an array_ref of the entries in the cities table
-  If there are entries in the table an empty row is returned
-  This is currently only used to populate a popup in memberentry
-
-=cut
-
-sub GetCities {
-
-    my $dbh   = C4::Context->dbh;
-    my $city_arr = $dbh->selectall_arrayref(
-        q|SELECT cityid,city_zipcode,city_name,city_state,city_country FROM cities ORDER BY city_name|,
-        { Slice => {} });
-    if ( @{$city_arr} ) {
-        unshift @{$city_arr}, {
-            city_zipcode => q{},
-            city_name    => q{},
-            cityid       => q{},
-            city_state   => q{},
-            city_country => q{},
-        };
-    }
-
-    return  $city_arr;
-}
-
 =head2 GetSortDetails (OUEST-PROVENCE)
 
   ($lib) = &GetSortDetails($category,$sortvalue);

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc
index e2ef537..39a979c 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc
@@ -80,17 +80,18 @@
       [% END %]
       City: </label>
         <input type="text" id="city" name="city" size="20" value="[% city %]" />
-        [% IF ( city_cgipopup ) %]or choose
+        [% IF cities.count %]or choose
         <select id="select_city" name="select_city">
-        [% FOREACH city_loo IN city_loop %]
-            [% IF ( city_loo.selected ) %]
-            <option value="[% city_loo.city_zipcode %]|[% city_loo.city_name %]|[% city_loo.city_state %]|[% city_loo.city_country %]" selected="selected">
-            [% ELSE %]
-            <option value="[% city_loo.city_zipcode %]|[% city_loo.city_name %]|[% city_loo.city_state %]|[% city_loo.city_country %]">
+            <option value="|||"></option>
+            [% FOREACH c IN cities %]
+                [% IF c.city_name == city %]
+                <option value="[% c.city_zipcode %]|[% c.city_name %]|[% c.city_state %]|[% c.city_country %]" selected="selected">
+                [% ELSE %]
+                <option value="[% c.city_zipcode %]|[% c.city_name %]|[% c.city_state %]|[% c.city_country %]">
+                [% END %]
+                    [% c.city_name %] [% c.city_state %] [% c.city_zipcode %]
+                </option>
             [% END %]
-                [% city_loo.city_name %] [% city_loo.city_state %] [% city_loo.city_zipcode %]
-            </option>
-        [% END %]
         </select>
         [% END %]
       [% IF ( mandatorycity ) %]<span class="required">Required</span>[% END %]

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc
index 48844f9..3211684 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc
@@ -68,17 +68,18 @@
       [% END %]
       City: </label>
         <input type="text" id="city" name="city" size="20" value="[% city %]" />
-        [% IF ( city_cgipopup ) %]or choose
+        [% IF cities.count %]or choose
         <select id="select_city" name="select_city">
-        [% FOREACH city_loo IN city_loop %]
-            [% IF ( city_loo.selected ) %]
-            <option value="[% city_loo.city_zipcode %]|[% city_loo.city_name %]|[% city_loo.city_state %]|[% city_loo.city_country %]" selected="selected">
-            [% ELSE %]
-            <option value="[% city_loo.city_zipcode %]|[% city_loo.city_name %]|[% city_loo.city_state %]|[% city_loo.city_country %]">
+            <option value="|||"></option>
+            [% FOREACH c IN cities %]
+                [% IF c.city_name == city %]
+                <option value="[% c.city_zipcode %]|[% c.city_name %]|[% c.city_state %]|[% c.city_country %]" selected="selected">
+                [% ELSE %]
+                <option value="[% c.city_zipcode %]|[% c.city_name %]|[% c.city_state %]|[% c.city_country %]">
+                [% END %]
+                    [% c.city_name %] [% c.city_state %] [% c.city_zipcode %]
+                </option>
             [% END %]
-                [% city_loo.city_name %] [% city_loo.city_state %] [% city_loo.city_zipcode %]
-            </option>
-        [% END %]
         </select>
         [% END %]
       [% IF ( mandatorycity ) %]<span class="required">Required</span>[% END %]

diff --git a/members/memberentry.pl b/members/memberentry.pl
index 4951614..5486928 100755 (executable)
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -40,6 +40,7 @@ use C4::Letters;
 use C4::Branch; # GetBranches
 use C4::Form::MessagingPreferences;
 use Koha::Borrower::Debarments;
+use Koha::Cities;
 use Koha::DateUtils;
 use Email::Valid;
 use Module::Load;
@@ -89,12 +90,10 @@ $nodouble = 1 if ($op eq 'modify' or $op eq 'duplicate');    # FIXME hack to rep
                                      # modifying an existing patron, it ipso facto
                                      # isn't a duplicate.  Marking FIXME because this
                                      # script needs to be refactored.
-my $select_city   = $input->param('select_city');
 my $nok           = $input->param('nok');
 my $guarantorinfo = $input->param('guarantorinfo');
 my $step          = $input->param('step') || 0;
 my @errors;
-my $default_city;
 my $borrower_data;
 my $NoUpdateLogin;
 my $userenv = C4::Context->userenv;
@@ -537,31 +536,14 @@ foreach (qw(C A S P I X)) {
 $template->param('typeloop' => \@typeloop,
         no_categories => $no_categories);
 if($no_categories){ $no_add = 1; }
-# test in city
-if ( $guarantorid ) {
-    $select_city = getidcity($data{city});
-}
-($default_city=$select_city) if ($step eq 0);
-if (!defined($select_city) or $select_city eq '' ){
-       $default_city = &getidcity($data{'city'});
-}
 
-my $city_arrayref = GetCities();
-if (@{$city_arrayref} ) {
-    $template->param( city_cgipopup => 1);
 
-    if ($default_city) { # flag the current or default val
-        for my $city ( @{$city_arrayref} ) {
-            if ($default_city == $city->{cityid}) {
-                $city->{selected} = 1;
-                last;
-            }
-        }
-    }
-}
-  
+my $cities = Koha::Cities->search( {}, { order_by => 'city_name' } );
 my $roadtypes = C4::Koha::GetAuthorisedValues( 'ROADTYPE', $data{streettype} );
-$template->param( roadtypes => $roadtypes);
+$template->param(
+    roadtypes => $roadtypes,
+    cities    => $cities,
+);
 
 my $default_borrowertitle = '';
 unless ( $op eq 'duplicate' ) { $default_borrowertitle=$data{'title'} }
@@ -689,7 +671,6 @@ $template->param(  step  => $step   ) if $step;     # associate with step to know wh
 $template->param(
   BorrowerMandatoryField => C4::Context->preference("BorrowerMandatoryField"),#field to test with javascript
   category_type => $category_type,#to know the category type of the borrower
-  select_city => $select_city,
   "$category_type"  => 1,# associate with step to know where u are
   destination   => $destination,#to know wher u come from and wher u must go in redirect
   check_member    => $check_member,#to know if the borrower already exist(=>1) or not (=>0) 
@@ -701,7 +682,6 @@ $template->param(
   borrowernumber  => $borrowernumber, #register number
   guarantorid => ($borrower_data->{'guarantorid'} || $guarantorid),
   relshiploop => \@relshipdata,
-  city_loop => $city_arrayref,
   borrotitlepopup => $borrotitlepopup,
   guarantorinfo   => $guarantorinfo,
   flagloop  => \@flagdata,