Bug 4248 Rework Cities dropdown for members entry

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

Signed-off-by: Galen Charlton <gmcharlt@gmail.com>

C4/Members.pm

@@ -1508,48 +1508,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'};
+    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{},
+        };
     }
 
-#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 );
-    }
+    return  $city_arr;
 }
 
 =head2 GetSortDetails (OUEST-PROVENCE)

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> 

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
@@ -434,15 +433,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'} ;
@@ -628,7 +632,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,