[Koha-patches] [PATCH] Bug 4248 Rework Cities dropdown for members entry

[Colin Campbell]

Garry Collum's patch correctly identified the core flaw in operation
looking closer revealed some other flaws (2 "blank" options, some vars
and documentation belonging to a superceded version, etc.
Changed GetCities to return a structure rather than the messy two returns
Let the template do the concatenation and removed the CGI::Popup
---
 C4/Members.pm                                      |   47 ++++++-------------
 .../prog/en/modules/members/memberentrygen.tmpl    |   11 ++++-
 members/memberentry.pl                             |   26 ++++++-----
 3 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/C4/Members.pm b/C4/Members.pm
index 98adc27..310c60d 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -1541,48 +1541,31 @@ sub add_member_orgs {
 
 }    # sub add_member_orgs
 
-=head2 GetCities (OUEST-PROVENCE)
+=head2 GetCities
 
-  ($id_cityarrayref, $city_hashref) = &GetCities();
+  $cityarrayref = GetCities();
 
-Looks up the different city and zip in the database. Returns two
-elements: a reference-to-array, which lists the zip city
-codes, and a reference-to-hash, which maps the name of the city.
-WHERE =>OUEST PROVENCE OR EXTERIEUR
+  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 ($type_city) = @_;
     my $dbh   = C4::Context->dbh;
-    my $query = qq|SELECT cityid,city_zipcode,city_name 
-        FROM cities 
-        ORDER BY city_name|;
-    my $sth = $dbh->prepare($query);
-
-    #$sth->execute($type_city);
-    $sth->execute();
-    my %city;
-    my @id;
-    #    insert empty value to create a empty choice in cgi popup
-    push @id, " ";
-    $city{""} = "";
-    while ( my $data = $sth->fetchrow_hashref ) {
-        push @id, $data->{'city_zipcode'}."|".$data->{'city_name'};
-        $city{ $data->{'city_zipcode'}."|".$data->{'city_name'} } = $data->{'city_name'};
-    }
-
-#test to know if the table contain some records if no the function return nothing
-    my $id = @id;
-    if ( $id == 1 ) {
-        # all we have is the one blank row
-        return ();
-    }
-    else {
-        unshift( @id, "" );
-        return ( \@id, \%city );
-    }
+    my $city_arr = $dbh->selectall_arrayref(
+        q|SELECT cityid,city_zipcode,city_name FROM cities ORDER BY city_name|,
+        { Slice => {} });
+    if ( @{$city_arr} ) {
+        unshift @{$city_arr}, {
+            city_zipcode => q{},
+            city_name    => q{},
+            cityid       => q{},
+        };
+    }
+
+    return  $city_arr;
 }
 
 =head2 GetSortDetails (OUEST-PROVENCE)
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tmpl b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tmpl
index 503fe54..811b622 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tmpl
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tmpl
@@ -403,7 +403,16 @@
       City, State: </label>
         
         <input type="text" id="city" name="city" size="20" value="<!-- TMPL_VAR NAME="city" -->" />
-        <!-- TMPL_IF NAME="city_cgipopup" -->or <strong>choose</strong> <!-- TMPL_VAR NAME="citypopup" --><!-- /TMPL_IF -->
+        <!-- TMPL_IF NAME="city_cgipopup" -->or <strong>choose</strong>
+        <select id="select_city" name "select_city">
+        <!-- TMPL_LOOP NAME="city_loop" -->
+            <option value="<!-- TMPL_VAR NAME='city_zipcode'-->|<!-- TMPL_VAR NAME='city_name' -->"
+                <!-- TMPL_IF NAME="selected" --> selected="1" <!-- /TMPL_IF --> >
+                <!-- TMPL_VAR NAME="city_name" --> <!-- TMPL_VAR NAME="city_zipcode" -->
+            </option>
+        <!-- /TMPL_LOOP -->
+        </select>
+        <!-- /TMPL_IF -->
 	  <!-- TMPL_IF NAME="mandatorycity" --><span class="required">Required</span><!-- /TMPL_IF -->
     </li>
     <li> 
diff --git a/members/memberentry.pl b/members/memberentry.pl
index f42e981..f51f299 100755
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -69,7 +69,6 @@ my $op             = $input->param('op');
 my $destination    = $input->param('destination');
 my $cardnumber     = $input->param('cardnumber');
 my $check_member   = $input->param('check_member');
-my $name_city      = $input->param('name_city');
 my $nodouble       = $input->param('nodouble');
 $nodouble = 1 if $op eq 'modify'; # FIXME hack to represent fact that if we're
                                   # modifying an existing patron, it ipso facto
@@ -435,15 +434,20 @@ $select_city=getidcity($data{'city'}) if defined $guarantorid and ($guarantorid
 if (!defined($select_city) or $select_city eq '' ){
 	$default_city = &getidcity($data{'city'});
 }
-my($cityid);
-($cityid,$name_city)=GetCities();
-$template->param( city_cgipopup => 1) if ($cityid );
-my $citypopup = CGI::popup_menu(-name=>'select_city',
-        -id => 'select_city',
-        '-values' =>$cityid,
-        -labels=>$name_city,
-        -default=>$default_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 $default_roadtype;
 $default_roadtype=$data{'streettype'} ;
@@ -629,7 +633,7 @@ $template->param(
   guarantorid => (defined($borrower_data->{'guarantorid'})) ? $borrower_data->{'guarantorid'} : $guarantorid,
   ethcatpopup => $ethcatpopup,
   relshiploop => \@relshipdata,
-  citypopup => $citypopup,
+  city_loop => $city_arrayref,
   roadpopup => $roadpopup,  
   borrotitlepopup => $borrotitlepopup,
   guarantorinfo   => $guarantorinfo,
-- 
1.6.6.1