[freeside-devel] Package Selections for pre14

[ivan]

On Sun, Jun 23, 2002 at 11:54:34PM -0400, Stephen Bechard wrote:
> 
> Okay, I have reconfigured the view/cust_main.cgi to use the above mentioned
> recommendations, along with the configuration option of remoteuser_security.
> 
> Without the file remoteuser_security in the conf directory it should be the
> same as before except the list is sorted alphabetically by pkg name.
> 
> With the file remoteuser_security in the conf directory the package list
> will be restricted to only the packages from the REMOTE_USER agent_type and
> sorted alphabetically by pkg name.
> 
> Please let me know if this one meets standards before I continue with
> modifying the edit/cust_main.cgi, signup.cgi and the config/config-view.cgi
> ;)

While I wouldn't refuse the patch for this reason, can you come up with a
more informative name than "remoteuser_security" ?  To the end user, this
doesn't really have anything to do with remote users or security.
Something that indicates this will bind "otakers" (the users added with
freeside-adduser) and "agents" together would be better.  In fact, you'll
likely have to add supporting code other places to maintain this
corrspondance - we shoult *not* count on folks entering identical data in
their agent list as they added with freeside-adduser - we should enforce
it.  Also, don't forget to add the configuration item to Conf.pm, so
people can use it :) 

See below for some specific comments on your code (in general, it's fine):

> --- /tarballs/freeside-1.4.0pre14/httemplate/view/cust_main.cgi     Mon Jun
> 10 15:44:45 2002
> +++ /aspdocs/view/cust_main.cgi-without-EFT   Sun Jun 23 23:40:48 2002
> @@ -255,18 +255,36 @@
>    qq!<INPUT TYPE="hidden" NAME="custnum" VALUE="$custnum">!.
>    '<SELECT NAME="pkgpart"><OPTION> ';
> 
> -foreach my $type_pkgs ( qsearch('type_pkgs',{'typenum'=>
> $agent->typenum }) ) {
> -  my $pkgpart = $type_pkgs->pkgpart;
> -#  my $part_pkg = qsearchs('part_pkg', { 'pkgpart' => $pkgpart } )
> -#    or do { warn "unknown type_pkgs.pkgpart $pkgpart"; next; };
> -  my $part_pkg =
> -    qsearchs('part_pkg', { 'pkgpart' => $pkgpart, 'disabled' => '' } )
> -    or next;
> -  print qq!<OPTION VALUE="$pkgpart">!. $part_pkg->pkg. ' - '.
> -        $part_pkg->comment;
> -}
> +  if ( $conf->exists('remoteuser_security') ) {
> +    my $remote_user = FS::UID->getotaker;
> +    my $agents_hash = qsearchs( 'agent', { 'agent' => $remote_user } );

This is only going to return a single record - that's what `qsearchs'
does, as opposed to `qsearch' - and it isn't a hash, it's an FS::agent
object...

and you probably need to handle the case where it isn't found...

> +    my $pkgpart;
> +    my %typenum;
> +    foreach my $agent ( $agents_hash ) {

iterating over a list of a single value... hua?

> +      next if $typenum{$agent->typenum}++;
> +      foreach ( keys %{ $agent->pkgpart_hashref } ) { $pkgpart->{$_}++; }
> #5.004_04 workaround
> +    }
> +    @part_pkg = grep { $pkgpart->{ $_->pkgpart } }
> +      qsearch( 'part_pkg', { 'disabled' => '' } );
> +  } else {
> +    my $agents_hash = qsearchs( 'agent', { 'agent' => $agent->agent } );

This seems rather redundant... you've already got the object you're
stuffing into $agents_hash in $agent

> +    my $pkgpart;
> +    my %typenum;
> +    foreach my $agent ( $agents_hash ) {
> +      next if $typenum{$agent->typenum}++;
> +      foreach ( keys %{ $agent->pkgpart_hashref } ) { $pkgpart->{$_}++; }
> #5.004_04 workaround
> +    }
> +    @part_pkg = grep { $pkgpart->{ $_->pkgpart } }
> +      qsearch( 'part_pkg', { 'disabled' => '' } );
> +  }

This is all a duplicate of the above... probably should move it all out of
the if { } else {}

-- 
_ivan