[Bps-public-commit] rt-authen-externalauth branch, multiple-emails, updated. 0.09-42-gbddaadc

[Jesse Vincent]

Just as drive-by review, there were a couple bits of code here 
that are very concise but quite hard to read and maintain. I'm sure
that much of this code dates from before the commits that just went
in, but it's gotta change.


> +        next unless
> +            grep ref $_,
> +            map $config->{'attr_map'}{ $_ },
> +            @{ $config->{'attr_match_list'} };

I bet this could be made easier to read and maintain

> 
> [...]
> +        foreach my $rt_attr ( @{ $config->{'attr_match_list'} } ) {
> +            next unless exists $args{ $rt_attr }
> +                && defined $args{ $rt_attr }
> +                && length $args{ $rt_attr };
> +            next unless ref $config->{'attr_map'}{ $rt_attr };

I suspect the above would be a little cleaner if it was flipped into
"if we get what we want, then" rather than "if it's not what we want, then skip"

> +
> +sub FindRecordsByOtherFields {
> +    my $user = shift;
> +    my %args = @_;
> +
> +    my @info_services = $RT::ExternalInfoPriority ? @{$RT::ExternalInfoPriority} : ();
> +    foreach my $service ( splice @info_services ) {
> +        my $config = $RT::ExternalSettings->{ $service };
> +        next if $config->{'type'} eq 'cookie';
> +        next unless @{ $config->{'attr_match_list'} } > 1;

same.

> +        push @info_services, $service;
> +    }
> +    return unless @info_services;
> +
> +    # find user in external service and fetch alternative values
> +    # for a field
> +    foreach my $service (@info_services) {
> +        my $config = $RT::ExternalSettings->{$service};
> +
> +        foreach my $search_by ( @{ $config->{'attr_match_list'} } ) {
> +            next unless exists $args{ $search_by }
> +                && defined $args{ $search_by }
> +                && length $args{ $search_by };
> +
> +            my @fetch =
> +                map ref $_? @$_ : $_,
> +                grep defined,
> +                map $config->{'attr_map'}{ $_ },
> +                grep $_ ne $search_by,
> +                @{ $config->{'attr_match_list'} };

That needs to get rewritten into boring looking code. two greps and two maps 
is way too hard to read.

> +            my %res =
> +                map { $_ => $config->{'attr_map'}{ $_ } }
> +                grep defined $config->{'attr_map'}{ $_ },
> +                grep $_ ne $search_by,
> +                @{ $config->{'attr_match_list'} }
> +            ;

Same.