Bug 13757: (QA followup) Fix non-editable attrs on failed save

When a field is not editable but displayable in the OPAC, and you submit
an incomplete/wrong update, those attributes are displayed as empty.

This patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
index 84e9a6c..231e2bf 100644 (file)
--- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
+++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
@@ -894,7 +894,7 @@
                     [% FOREACH pa_class IN patron_attribute_classes %]
                         [% IF pa_class.class %]
                             <fieldset id="aai_[% pa_loo.class %]" class="rows patron-attributes">
-                            <legend>[% pa_loo.lib %]</legend>
+                            <legend>[% pa_class.lib %]</legend>
                         [% ELSE %]
                             <fieldset class="rows patron-attributes">
                             <legend>Additional information</legend>
@@ -912,9 +912,8 @@
                                                 <select id="[% form_id %]" name="patron_attribute_value">
                                                     <option value=""></option>
                                                     [% FOREACH auth_val IN AuthorisedValues.Get( pa.type.authorised_value_category, 1 ) %]
-                                                        [% IF ( auth_val.authorised_value == pa_value.value ) %]
+                                                        [% IF ( auth_val.authorised_value == pa_value ) %]
                                                             <option value="[% auth_val.authorised_value %]" selected="selected">
-                                                                [%# Yes, lib; GetAuthorisedValues takes care of intelligently setting this from lib_opac %]
                                                                 [% auth_val.lib %]
                                                             </option>
                                                         [% ELSE %]
@@ -925,7 +924,7 @@
                                                     [% END %]
                                                 </select>
                                             [% ELSE %]
-                                                <textarea rows="2" cols="30" id="[% form_id %]" name="patron_attribute_value">[% pa_value.value %]</textarea>
+                                                <textarea rows="2" cols="30" id="[% form_id %]" name="patron_attribute_value">[% pa_value %]</textarea>
                                             [% END %]
                                             <a href="#" class="clear-attribute">Clear</a>
                                             [% IF ( pa.type.repeatable ) %]
@@ -933,9 +932,9 @@
                                             [% END %]
                                         [% ELSE %]
                                             [% IF ( pa.type.authorised_value_category ) %]
-                                                [% AuthorisedValues.GetByCode( pa.type.authorised_value_category, pa_value.value, 1 ) | html_line_break %]
+                                                [% AuthorisedValues.GetByCode( pa.type.authorised_value_category, pa_value, 1 ) | html_line_break %]
                                             [% ELSE %]
-                                                [% pa_value.value | html_line_break %]
+                                                [% pa_value | html_line_break %]
                                             [% END %]
                                         [% END %]
                                     </li>

diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
index 4b00623..2a20d63 100755 (executable)
--- a/opac/opac-memberentry.pl
+++ b/opac/opac-memberentry.pl
@@ -29,14 +29,16 @@ use C4::Output;
 use C4::Members;
 use C4::Members::Attributes qw( GetBorrowerAttributes );
 use C4::Form::MessagingPreferences;
-use Koha::Patrons;
-use Koha::Patron::Modification;
-use Koha::Patron::Modifications;
 use C4::Scrubber;
 use Email::Valid;
 use Koha::DateUtils;
 use Koha::Libraries;
+use Koha::Patron::Attribute::Types;
+use Koha::Patron::Attributes;
 use Koha::Patron::Images;
+use Koha::Patron::Modification;
+use Koha::Patron::Modifications;
+use Koha::Patrons;
 use Koha::Token;
 
 my $cgi = new CGI;
@@ -246,15 +248,15 @@ elsif ( $action eq 'update' ) {
                 secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ),
             }),
         );
-        $template->param( patron_attribute_classes => GeneratePatronAttributesForm( undef, $attributes ) );
+        $template->param( patron_attribute_classes => GeneratePatronAttributesForm( $borrowernumber, $attributes ) );
 
         $template->param( action => 'edit' );
     }
     else {
         my %borrower_changes = DelUnchangedFields( $borrowernumber, %borrower );
-        my $extended_attributes_changes = ExtendedAttributesMatch( $borrowernumber, $attributes );
+        my $extended_attributes_changes = FilterUnchagedAttributes( $borrowernumber, $attributes );
 
-        if ( %borrower_changes || $extended_attributes_changes ) {
+        if ( %borrower_changes || scalar @{$extended_attributes_changes} > 0 ) {
             ( $template, $borrowernumber, $cookie ) = get_template_and_user(
                 {
                     template_name   => "opac-memberentry-update-submitted.tt",
@@ -265,7 +267,7 @@ elsif ( $action eq 'update' ) {
             );
 
             $borrower_changes{borrowernumber} = $borrowernumber;
-            $borrower_changes{extended_attributes} = to_json($attributes);
+            $borrower_changes{extended_attributes} = to_json($extended_attributes_changes);
 
             # FIXME update the following with
             # Koha::Patron::Modifications->search({ borrowernumber => $borrowernumber })->delete;
@@ -286,7 +288,7 @@ elsif ( $action eq 'update' ) {
                 action => 'edit',
                 nochanges => 1,
                 borrower => GetMember( borrowernumber => $borrowernumber ),
-                patron_attribute_classes => GeneratePatronAttributesForm( undef, $attributes ),
+                patron_attribute_classes => GeneratePatronAttributesForm( $borrowernumber, $attributes ),
                 csrf_token => Koha::Token->new->generate_csrf({
                     id     => Encode::encode( 'UTF-8', $borrower->{userid} ),
                     secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ),
@@ -472,72 +474,106 @@ sub DelEmptyFields {
     return %borrower;
 }
 
-sub ExtendedAttributesMatch {
+sub FilterUnchagedAttributes {
     my ( $borrowernumber, $entered_attributes ) = @_;
 
-    my @patron_attributes_arr = GetBorrowerAttributes( $borrowernumber, 1 );
-    my $patron_attributes = $patron_attributes_arr[0];
+    my @patron_attributes = grep {$_->opac_editable} Koha::Patron::Attributes->search({ borrowernumber => $borrowernumber })->as_list;
+
+    my $patron_attribute_types;
+    foreach my $attr (@patron_attributes) {
+        $patron_attribute_types->{ $attr->code } += 1;
+    }
+
+    my $passed_attribute_types;
+    foreach my $attr (@{ $entered_attributes }) {
+        $passed_attribute_types->{ $attr->{ code } } += 1;
+    }
+
+    my @changed_attributes;
+
+    # Loop through the current patron attributes
+    foreach my $attribute_type ( keys %{ $patron_attribute_types } ) {
+        if ( $patron_attribute_types->{ $attribute_type } !=  $passed_attribute_types->{ $attribute_type } ) {
+            # count differs, overwrite all attributes for given type
+            foreach my $attr (@{ $entered_attributes }) {
+                push @changed_attributes, $attr
+                    if $attr->{ code } eq $attribute_type;
+            }
+        } else {
+            # count matches, check values
+            my $changes = 0;
+            foreach my $attr (grep { $_->code eq $attribute_type } @patron_attributes) {
+                $changes = 1
+                    unless any { $_->{ value } eq $attr->attribute } @{ $entered_attributes };
+                last if $changes;
+            }
 
-    if ( scalar @{$entered_attributes} != scalar @{$patron_attributes} ) {
-        return 1;
+            if ( $changes ) {
+                foreach my $attr (@{ $entered_attributes }) {
+                    push @changed_attributes, $attr
+                        if $attr->{ code } eq $attribute_type;
+                }
+            }
+        }
     }
 
-    foreach my $attr ( @{$patron_attributes} ) {
-        next if any {
-            $_->{code} eq $attr->{code} and $_->{value} eq $attr->{value};
+    # Loop through passed attributes, looking for new ones
+    foreach my $attribute_type ( keys %{ $passed_attribute_types } ) {
+        if ( !defined $patron_attribute_types->{ $attribute_type } ) {
+            # YAY, new stuff
+            foreach my $attr (grep { $_->{code} eq $attribute_type } @{ $entered_attributes }) {
+                push @changed_attributes, $attr;
+            }
         }
-        @{$entered_attributes};
-        return 1;
     }
 
-    return 0;
+    return \@changed_attributes;
 }
 
-
 sub GeneratePatronAttributesForm {
     my ( $borrowernumber, $entered_attributes ) = @_;
 
     # Get all attribute types and the values for this patron (if applicable)
-    my @types = C4::Members::AttributeTypes::GetAttributeTypes();
-
-    if (scalar(@types) == 0) {
+    my @types = grep { $_->opac_editable() or $_->opac_display }
+        Koha::Patron::Attribute::Types->search()->as_list();
+    if ( scalar(@types) == 0 ) {
         return [];
     }
 
-    my %attr_values = ();
+    my @displayable_attributes = grep { $_->opac_display }
+        Koha::Patron::Attributes->search({ borrowernumber => $borrowernumber })->as_list;
 
-    if ( $borrowernumber ) {
-        my $attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
+    my %attr_values = ();
 
-        # Remap the patron's attributes into a hash of arrayrefs per attribute (depends on
-        # autovivification)
-        foreach my $attr (@$attributes) {
-            push @{ $attr_values{ $attr->{code} } }, $attr;
+    # Build the attribute values list either from the passed values
+    # or taken from the patron itself
+    if ( defined $entered_attributes ) {
+        foreach my $attr (@$entered_attributes) {
+            push @{ $attr_values{ $attr->{code} } }, $attr->{value};
         }
     }
-
-    if ( $entered_attributes ) {
-        foreach my $attr (@$entered_attributes) {
-            push @{ $attr_values{ $attr->{code} } }, $attr;
+    elsif ( defined $borrowernumber ) {
+        my @editable_attributes = grep { $_->opac_editable } @displayable_attributes;
+        foreach my $attr (@editable_attributes) {
+            push @{ $attr_values{ $attr->code } }, $attr->attribute;
         }
     }
 
+    # Add the non-editable attributes (that don't come from the form)
+    foreach my $attr ( grep { !$_->opac_editable } @displayable_attributes ) {
+        push @{ $attr_values{ $attr->code } }, $attr->attribute;
+    }
+
     # Find all existing classes
-    my @classes = uniq( map { $_->{class} } @types );
-    @classes = sort @classes;
+    my @classes = sort( uniq( map { $_->class } @types ) );
     my %items_by_class;
 
-    foreach my $attr_type_desc (@types) {
-        my $attr_type = C4::Members::AttributeTypes->fetch( $attr_type_desc->{code} );
-        # Make sure this attribute should be displayed in the OPAC
-        next unless ( $attr_type->opac_display() );
-        # Then, make sure it either has values or is editable
-        next unless ( $attr_values{ $attr_type->code() } || $attr_type->opac_editable() );
-
+    foreach my $attr_type (@types) {
         push @{ $items_by_class{ $attr_type->class() } }, {
             type => $attr_type,
-            # If editable, make sure there's at least one empty entry, to make the template's job easier
-            values => $attr_values{ $attr_type->code() } || [{}]
+            # If editable, make sure there's at least one empty entry,
+            # to make the template's job easier
+            values => $attr_values{ $attr_type->code() } || ['']
         };
     }
 
@@ -546,43 +582,60 @@ sub GeneratePatronAttributesForm {
     foreach my $class (@classes) {
         next unless ( $items_by_class{$class} );
 
-        my $av = Koha::AuthorisedValues->search({ category => 'PA_CLASS', authorised_value => $class });
+        my $av = Koha::AuthorisedValues->search(
+            { category => 'PA_CLASS', authorised_value => $class } );
+
         my $lib = $av->count ? $av->next->opac_description : $class;
 
-        push @class_loop, {
+        push @class_loop,
+            {
             class => $class,
             items => $items_by_class{$class},
             lib   => $lib,
-        };
+            };
     }
 
     return \@class_loop;
 }
 
 sub ParsePatronAttributes {
-    my ($borrowernumber,$cgi) = @_;
+    my ( $borrowernumber, $cgi ) = @_;
 
     my @codes  = $cgi->multi_param('patron_attribute_code');
     my @values = $cgi->multi_param('patron_attribute_value');
 
     my $ea = each_array( @codes, @values );
     my @attributes;
-    my %dups = ();
+
+    my $delete_candidates = {};
 
     while ( my ( $code, $value ) = $ea->() ) {
-        # Don't skip if the patron already has attributes with $code, because
-        # it means we are being requested to remove the attributes.
-        next
-            unless defined($value) and $value ne ''
-            or Koha::Patron::Attributes->search(
-            { borrowernumber => $borrowernumber, code => $code } )->count > 0;
-        next if exists $dups{$code}->{$value};
-        $dups{$code}->{$value} = 1;
+        if ( !defined($value) or $value eq '' ) {
+            $delete_candidates->{$code} = 1
+                unless $delete_candidates->{$code};
+        }
+        else {
+            # we've got a value
+            push @attributes, { code => $code, value => $value };
+
+            # 'code' is no longer a delete candidate
+            delete $delete_candidates->{$code};
+        }
+    }
 
-        push @attributes, { code => $code, value => $value };
+    foreach my $code ( keys %{$delete_candidates} ) {
+        if (Koha::Patron::Attributes->search(
+                { borrowernumber => $borrowernumber, code => $code }
+            )->count > 0
+            )
+        {
+            push @attributes, { code => $code, value => '' }
+                unless any { $_->{code} eq $code } @attributes;
+        }
     }
 
     return \@attributes;
 }
 
+
 1;