X-Git-Url: http://koha-dev.rot13.org:8081/gitweb/?a=blobdiff_plain;ds=sidebyside;f=opac%2Fopac-memberentry.pl;h=5215394b96bfa15b3d8c6ad3c18906d3b72cdc66;hb=481f52308a32860e8594d13bd11086845e498ae4;hp=2100ca827a0ad02fa9d58485d9c97e9b5e1e7272;hpb=01506e0195266a0e6b6005c43b9763bf4a09dd78;p=koha-ffzg.git

diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
index 2100ca827a..5215394b96 100755
--- a/opac/opac-memberentry.pl
+++ b/opac/opac-memberentry.pl
@@ -19,26 +19,33 @@ use Modern::Perl;
 
 use CGI qw ( -utf8 );
 use Digest::MD5 qw( md5_base64 md5_hex );
-use Encode qw( encode );
-use JSON;
-use List::MoreUtils qw( each_array uniq );
+use JSON qw( to_json );
+use List::MoreUtils qw( any each_array uniq );
 use String::Random qw( random_string );
 
-use C4::Auth;
-use C4::Output;
-use C4::Members;
+use C4::Auth qw( get_template_and_user );
+use C4::Output qw( output_html_with_http_headers );
+use C4::Context;
+use C4::Members qw( checkcardnumber );
 use C4::Form::MessagingPreferences;
+use Koha::AuthUtils;
 use Koha::Patrons;
+use Koha::Patron::Consent;
 use Koha::Patron::Modification;
 use Koha::Patron::Modifications;
 use C4::Scrubber;
-use Email::Valid;
-use Koha::DateUtils;
+use Email::Address;
+use Koha::DateUtils qw( dt_from_string output_pref );
 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::Patron::Categories;
 use Koha::Token;
-
-my $cgi = new CGI;
+use Koha::AuthorisedValues;
+my $cgi = CGI->new;
 my $dbh = C4::Context->dbh;
 
 my ( $template, $borrowernumber, $cookie ) = get_template_and_user(
@@ -68,10 +75,14 @@ if ( $action eq q{} ) {
 
 my $mandatory = GetMandatoryFields($action);
 
-my @libraries = Koha::Libraries->search;
-if ( my @libraries_to_display = split '\|', C4::Context->preference('PatronSelfRegistrationLibraryList') ) {
-    @libraries = map { my $b = $_; my $branchcode = $_->branchcode; grep( /^$branchcode$/, @libraries_to_display ) ? $b : () } @libraries;
+my $params = {};
+if ( $action eq 'create' || $action eq 'new' ) {
+    my @PatronSelfRegistrationLibraryList = split '\|', C4::Context->preference('PatronSelfRegistrationLibraryList');
+    $params = { branchcode => { -in => \@PatronSelfRegistrationLibraryList } }
+      if @PatronSelfRegistrationLibraryList;
 }
+my @libraries = Koha::Libraries->search($params);
+
 my ( $min, $max ) = C4::Members::get_cardnumber_length();
 if ( defined $min ) {
      $template->param(
@@ -80,24 +91,28 @@ if ( defined $min ) {
      );
  }
 
+my $defaultCategory = Koha::Patron::Categories->find(C4::Context->preference('PatronSelfRegistrationDefaultCategory'));
+
 $template->param(
     action            => $action,
-    hidden            => GetHiddenFields( $mandatory, 'registration' ),
+    hidden            => GetHiddenFields( $mandatory, $action ),
     mandatory         => $mandatory,
     libraries         => \@libraries,
     OPACPatronDetails => C4::Context->preference('OPACPatronDetails'),
+    defaultCategory  => $defaultCategory,
 );
 
-my $attributes = ParsePatronAttributes($cgi);
+my $attributes = ParsePatronAttributes($borrowernumber,$cgi);
 my $conflicting_attribute = 0;
 
 foreach my $attr (@$attributes) {
-    unless ( C4::Members::Attributes::CheckUniqueness($attr->{code}, $attr->{value}, $borrowernumber) ) {
-        my $attr_info = C4::Members::AttributeTypes->fetch($attr->{code});
+    my $attribute = Koha::Patron::Attribute->new($attr);
+    if ( !$attribute->unique_ok ) {
+        my $attr_type = Koha::Patron::Attribute::Types->find($attr->{code});
         $template->param(
             extended_unique_id_failed_code => $attr->{code},
-            extended_unique_id_failed_value => $attr->{value},
-            extended_unique_id_failed_description => $attr_info->description()
+            extended_unique_id_failed_value => $attr->{attribute},
+            extended_unique_id_failed_description => $attr_type->description,
         );
         $conflicting_attribute = 1;
     }
@@ -108,8 +123,9 @@ if ( $action eq 'create' ) {
     my %borrower = ParseCgiForBorrower($cgi);
 
     %borrower = DelEmptyFields(%borrower);
+    $borrower{categorycode} ||= C4::Context->preference('PatronSelfRegistrationDefaultCategory');
 
-    my @empty_mandatory_fields = CheckMandatoryFields( \%borrower, $action );
+    my @empty_mandatory_fields = (CheckMandatoryFields( \%borrower, $action ), CheckMandatoryAttributes( \%borrower, $attributes ) );
     my $invalidformfields = CheckForInvalidFields(\%borrower);
     delete $borrower{'password2'};
     my $cardnumber_error_code;
@@ -141,10 +157,12 @@ if ( $action eq 'create' ) {
             borrower       => \%borrower
         );
         $template->param( patron_attribute_classes => GeneratePatronAttributesForm( undef, $attributes ) );
+    } elsif ( ! grep { $borrower{branchcode} eq $_->branchcode } @libraries ) {
+        die "Branchcode not allowed"; # They hack the form
     }
     else {
         if (
-            C4::Context->boolean_preference(
+            C4::Context->preference(
                 'PatronSelfRegistrationVerifyByEmail')
           )
         {
@@ -163,7 +181,7 @@ if ( $action eq 'create' ) {
                 $verification_token = md5_hex( time().{}.rand().{}.$$ );
             }
 
-            $borrower{password}           = random_string("..........");
+            $borrower{password}          = Koha::AuthUtils::generate_password(Koha::Patron::Categories->find($borrower{categorycode})) unless $borrower{password};
             $borrower{verification_token} = $verification_token;
 
             Koha::Patron::Modification->new( \%borrower )->store();
@@ -172,12 +190,13 @@ if ( $action eq 'create' ) {
             my $letter = C4::Letters::GetPreparedLetter(
                 module      => 'members',
                 letter_code => 'OPAC_REG_VERIFY',
+                lang        => 'default', # Patron does not have a preferred language defined yet
                 tables      => {
                     borrower_modifications => $verification_token,
                 },
             );
 
-            C4::Letters::EnqueueLetter(
+            my $message_id = C4::Letters::EnqueueLetter(
                 {
                     letter                 => $letter,
                     message_transport_type => 'email',
@@ -186,6 +205,7 @@ if ( $action eq 'create' ) {
                       C4::Context->preference('KohaAdminEmailAddress'),
                 }
             );
+            C4::Letters::SendQueuedMessages({ message_id => $message_id });
         }
         else {
             ( $template, $borrowernumber, $cookie ) = get_template_and_user(
@@ -197,16 +217,28 @@ if ( $action eq 'create' ) {
                 }
             );
 
-            $template->param( OpacPasswordChange =>
-                  C4::Context->preference('OpacPasswordChange') );
-
-            my ( $borrowernumber, $password ) = AddMember_Opac(%borrower);
-            C4::Members::Attributes::SetBorrowerAttributes( $borrowernumber, $attributes );
-            C4::Form::MessagingPreferences::handle_form_action($cgi, { borrowernumber => $borrowernumber }, $template, 1, C4::Context->preference('PatronSelfRegistrationDefaultCategory') ) if $borrowernumber && C4::Context->preference('EnhancedMessagingPreferences');
+            $borrower{password}         ||= Koha::AuthUtils::generate_password(Koha::Patron::Categories->find($borrower{categorycode}));
+            my $consent_dt = delete $borrower{gdpr_proc_consent};
+            my $patron = Koha::Patron->new( \%borrower )->store;
+            Koha::Patron::Consent->new({ borrowernumber => $patron->borrowernumber, type => 'GDPR_PROCESSING', given_on => $consent_dt })->store if $consent_dt;
+            if ( $patron ) {
+                $patron->extended_attributes->filter_by_branch_limitations->delete;
+                $patron->extended_attributes($attributes);
+                if ( C4::Context->preference('EnhancedMessagingPreferences') ) {
+                    C4::Form::MessagingPreferences::handle_form_action(
+                        $cgi,
+                        { borrowernumber => $patron->borrowernumber },
+                        $template,
+                        1,
+                        C4::Context->preference('PatronSelfRegistrationDefaultCategory')
+                    );
+                }
 
-            $template->param( password_cleartext => $password );
-            $template->param(
-                borrower => GetMember( borrowernumber => $borrowernumber ) );
+                $template->param( password_cleartext => $patron->plain_text_password );
+                $template->param( borrower => $patron->unblessed );
+            } else {
+                # FIXME Handle possible errors here
+            }
             $template->param(
                 PatronSelfRegistrationAdditionalInstructions =>
                   C4::Context->preference(
@@ -217,19 +249,18 @@ if ( $action eq 'create' ) {
 }
 elsif ( $action eq 'update' ) {
 
-    my $borrower = GetMember( borrowernumber => $borrowernumber );
+    my $borrower = Koha::Patrons->find( $borrowernumber )->unblessed;
     die "Wrong CSRF token"
         unless Koha::Token->new->check_csrf({
-            id     => Encode::encode( 'UTF-8', $borrower->{userid} ),
-            secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ),
+            session_id => scalar $cgi->cookie('CGISESSID'),
             token  => scalar $cgi->param('csrf_token'),
         });
 
     my %borrower = ParseCgiForBorrower($cgi);
+    $borrower{borrowernumber} = $borrowernumber;
 
-    my %borrower_changes = DelEmptyFields(%borrower);
-    my @empty_mandatory_fields =
-      CheckMandatoryFields( \%borrower_changes, $action );
+    my @empty_mandatory_fields = grep { $_ ne 'password' } # password is not required when editing personal details
+      ( CheckMandatoryFields( \%borrower, $action ), CheckMandatoryAttributes( \%borrower, $attributes ) );
     my $invalidformfields = CheckForInvalidFields(\%borrower);
 
     # Send back the data to the template
@@ -241,17 +272,19 @@ elsif ( $action eq 'update' ) {
             invalid_form_fields    => $invalidformfields,
             borrower               => \%borrower,
             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') ) ),
+                session_id => scalar $cgi->cookie('CGISESSID'),
             }),
         );
-        $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 );
-        if (%borrower_changes) {
+        $borrower_changes{'changed_fields'} = join ',', keys %borrower_changes;
+        my $extended_attributes_changes = FilterUnchangedAttributes( $borrowernumber, $attributes );
+
+        if ( %borrower_changes || scalar @{$extended_attributes_changes} > 0 ) {
             ( $template, $borrowernumber, $cookie ) = get_template_and_user(
                 {
                     template_name   => "opac-memberentry-update-submitted.tt",
@@ -262,63 +295,69 @@ elsif ( $action eq 'update' ) {
             );
 
             $borrower_changes{borrowernumber} = $borrowernumber;
-            $borrower_changes{extended_attributes} = to_json($attributes);
-
-            # FIXME update the following with
-            # Koha::Patron::Modifications->search({ borrowernumber => $borrowernumber })->delete;
-            # when bug 17091 will be pushed
-            my $patron_modifications = Koha::Patron::Modifications->search({ borrowernumber => $borrowernumber });
-            while ( my $patron_modification = $patron_modifications->next ) {
-                $patron_modification->delete;
-            }
+            $borrower_changes{extended_attributes} = to_json($extended_attributes_changes);
+
+            Koha::Patron::Modifications->search({ borrowernumber => $borrowernumber })->delete;
 
             my $m = Koha::Patron::Modification->new( \%borrower_changes )->store();
+            #Automatically approve patron profile changes if set in syspref
 
-            $template->param(
-                borrower => GetMember( borrowernumber => $borrowernumber ),
-            );
+            if (C4::Context->preference('AutoApprovePatronProfileSettings')) {
+                # Need to get the object from database, otherwise it is not complete enough to allow deletion
+                # when approval has been performed.
+                my $tmp_m = Koha::Patron::Modifications->find({borrowernumber => $borrowernumber});
+                $tmp_m->approve() if $tmp_m;
+            }
+
+            my $patron = Koha::Patrons->find( $borrowernumber );
+            $template->param( borrower => $patron->unblessed );
         }
         else {
+            my $patron = Koha::Patrons->find( $borrowernumber );
             $template->param(
                 action => 'edit',
                 nochanges => 1,
-                borrower => GetMember( borrowernumber => $borrowernumber ),
+                borrower => $patron->unblessed,
+                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') ) ),
+                    session_id => scalar $cgi->cookie('CGISESSID'),
                 }),
             );
         }
     }
 }
 elsif ( $action eq 'edit' ) {    #Display logged in borrower's data
-    my $borrower = GetMember( borrowernumber => $borrowernumber );
+    my $patron = Koha::Patrons->find( $borrowernumber );
+    my $borrower = $patron->unblessed;
 
     $template->param(
         borrower  => $borrower,
-        guarantor => scalar Koha::Patrons->find($borrowernumber)->guarantor(),
-        hidden => GetHiddenFields( $mandatory, 'modification' ),
+        hidden => GetHiddenFields( $mandatory, 'edit' ),
         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') ) ),
+            session_id => scalar $cgi->cookie('CGISESSID'),
         }),
     );
 
     if (C4::Context->preference('OPACpatronimages')) {
-        my $patron_image = Koha::Patron::Images->find($borrower->{borrowernumber});
-        $template->param( display_patron_image => 1 ) if $patron_image;
+        $template->param( display_patron_image => 1 ) if $patron->image;
     }
 
     $template->param( patron_attribute_classes => GeneratePatronAttributesForm( $borrowernumber ) );
 } else {
+    # Render self-registration page
     $template->param( patron_attribute_classes => GeneratePatronAttributesForm() );
 }
 
 my $captcha = random_string("CCCCC");
+my $patron_param = Koha::Patrons->find( $borrowernumber );
+$template->param(
+    has_guarantor_flag => $patron_param->guarantor_relationships->guarantors->_resultset->count
+) if $patron_param;
 
 $template->param(
     captcha        => $captcha,
-    captcha_digest => md5_base64($captcha)
+    captcha_digest => md5_base64($captcha),
+    patron         => $patron_param
 );
 
 output_html_with_http_headers $cgi, $cookie, $template->output, undef, { force_no_caching => 1 };
@@ -327,7 +366,7 @@ sub GetHiddenFields {
     my ( $mandatory, $action ) = @_;
     my %hidden_fields;
 
-    my $BorrowerUnwantedField = $action eq 'modification' ?
+    my $BorrowerUnwantedField = $action eq 'edit' || $action eq 'update' ?
       C4::Context->preference( "PatronSelfModificationBorrowerUnwantedField" ) :
       C4::Context->preference( "PatronSelfRegistrationBorrowerUnwantedField" );
 
@@ -351,6 +390,7 @@ sub GetMandatoryFields {
       C4::Context->preference("PatronSelfRegistrationBorrowerMandatoryField");
 
     my @fields = split( /\|/, $BorrowerMandatoryField );
+    push @fields, 'gdpr_proc_consent' if C4::Context->preference('GDPR_Policy') && $action eq 'create';
 
     foreach (@fields) {
         $mandatory_fields{$_} = 1;
@@ -358,7 +398,7 @@ sub GetMandatoryFields {
 
     if ( $action eq 'create' || $action eq 'new' ) {
         $mandatory_fields{'email'} = 1
-          if C4::Context->boolean_preference(
+          if C4::Context->preference(
             'PatronSelfRegistrationVerifyByEmail');
     }
 
@@ -381,34 +421,67 @@ sub CheckMandatoryFields {
     return @empty_mandatory_fields;
 }
 
+sub CheckMandatoryAttributes{
+    my ( $borrower, $attributes ) = @_;
+
+    my @empty_mandatory_fields;
+
+    for my $attribute (@$attributes ) {
+        my $attr = Koha::Patron::Attribute::Types->find($attribute->{code});
+        push @empty_mandatory_fields, $attribute->{code}
+            if $attr && $attr->mandatory && $attribute->{attribute} =~ m|^\s*$|;
+    }
+
+    return @empty_mandatory_fields;
+}
+
 sub CheckForInvalidFields {
-    my $minpw = C4::Context->preference('minPasswordLength');
     my $borrower = shift;
     my @invalidFields;
     if ($borrower->{'email'}) {
-        unless ( Email::Valid->address($borrower->{'email'}) ) {
+        unless ( $borrower->{'email'} =~ m/$Email::Address::mailbox/ ) {
             push(@invalidFields, "email");
         } elsif ( C4::Context->preference("PatronSelfRegistrationEmailMustBeUnique") ) {
-            my $patrons_with_same_email = Koha::Patrons->search( { email => $borrower->{email} })->count;
+            my $patrons_with_same_email = Koha::Patrons->search( # FIXME Should be search_limited?
+                {
+                    email => $borrower->{email},
+                    (
+                        exists $borrower->{borrowernumber}
+                        ? ( borrowernumber =>
+                              { '!=' => $borrower->{borrowernumber} } )
+                        : ()
+                    )
+                }
+            )->count;
             if ( $patrons_with_same_email ) {
                 push @invalidFields, "duplicate_email";
             }
+        } elsif ( C4::Context->preference("PatronSelfRegistrationConfirmEmail")
+            && $borrower->{'email'} ne $borrower->{'repeat_email'}
+            && !defined $borrower->{borrowernumber} ) {
+            push @invalidFields, "email_match";
         }
+        # email passed all tests, so prevent attempting to store repeat_email
+        delete $borrower->{'repeat_email'};
     }
     if ($borrower->{'emailpro'}) {
-        push(@invalidFields, "emailpro") if (!Email::Valid->address($borrower->{'emailpro'}));
+        push(@invalidFields, "emailpro") if ($borrower->{'emailpro'} !~ m/$Email::Address::mailbox/);
     }
     if ($borrower->{'B_email'}) {
-        push(@invalidFields, "B_email") if (!Email::Valid->address($borrower->{'B_email'}));
-    }
-    if ( $borrower->{'password'} ne $borrower->{'password2'} ){
-        push(@invalidFields, "password_match");
+        push(@invalidFields, "B_email") if ($borrower->{'B_email'} !~ m/$Email::Address::mailbox/);
     }
-    if ( $borrower->{'password'}  && $minpw && (length($borrower->{'password'}) < $minpw) ) {
-       push(@invalidFields, "password_invalid");
+    if ( defined $borrower->{'password'}
+        and $borrower->{'password'} ne $borrower->{'password2'} )
+    {
+        push( @invalidFields, "password_match" );
     }
     if ( $borrower->{'password'} ) {
-       push(@invalidFields, "password_spaces") if ($borrower->{'password'} =~ /^\s/ or $borrower->{'password'} =~ /\s$/);
+        my ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $borrower->{password}, Koha::Patron::Categories->find($borrower->{categorycode}||C4::Context->preference('PatronSelfRegistrationDefaultCategory')) );
+          unless ( $is_valid ) {
+              push @invalidFields, 'password_too_short' if $error eq 'too_short';
+              push @invalidFields, 'password_too_weak' if $error eq 'too_weak';
+              push @invalidFields, 'password_has_whitespaces' if $error eq 'has_whitespaces';
+          }
     }
 
     return \@invalidFields;
@@ -420,36 +493,52 @@ sub ParseCgiForBorrower {
     my $scrubber = C4::Scrubber->new();
     my %borrower;
 
-    foreach ( $cgi->param ) {
-        if ( $_ =~ '^borrower_' ) {
-            my ($key) = substr( $_, 9 );
-            $borrower{$key} = $scrubber->scrub( scalar $cgi->param($_) );
+    foreach my $field ( $cgi->param ) {
+        if ( $field =~ '^borrower_' ) {
+            my ($key) = substr( $field, 9 );
+            if ( $field !~ '^borrower_password' ) {
+                $borrower{$key} = $scrubber->scrub( scalar $cgi->param($field) );
+            } else {
+                # Allow html characters for passwords
+                $borrower{$key} = $cgi->param($field);
+            }
         }
     }
 
-    my $dob_dt;
-    $dob_dt = eval { dt_from_string( $borrower{'dateofbirth'} ); }
-        if ( $borrower{'dateofbirth'} );
+    if ( defined $borrower{'dateofbirth'} ) {
+        my $dob_dt;
+        $dob_dt = eval { dt_from_string( $borrower{'dateofbirth'} ); }
+            if ( $borrower{'dateofbirth'} );
 
-    if ( $dob_dt ) {
-        $borrower{'dateofbirth'} = output_pref ( { dt => $dob_dt, dateonly => 1, dateformat => 'iso' } );
-    }
-    else {
-        # Trigger validation
-        $borrower{'dateofbirth'} = undef;
+        if ( $dob_dt ) {
+            $borrower{'dateofbirth'} = output_pref( { dt => $dob_dt, dateonly => 1, dateformat => 'iso' } );
+        }
+        else {
+            # Trigger validation
+            $borrower{'dateofbirth'} = undef;
+        }
     }
 
+    # Replace checkbox 'agreed' by datetime in gdpr_proc_consent
+    $borrower{gdpr_proc_consent} = dt_from_string if  $borrower{gdpr_proc_consent} && $borrower{gdpr_proc_consent} eq 'agreed';
+
     return %borrower;
 }
 
 sub DelUnchangedFields {
     my ( $borrowernumber, %new_data ) = @_;
+    # get the mandatory fields so we can get the hidden fields
+    my $mandatory = GetMandatoryFields('edit');
+    my $patron = Koha::Patrons->find( $borrowernumber );
+    my $current_data = $patron->unblessed;
+    # get the hidden fields so we don't obliterate them should they have data patrons aren't allowed to modify
+    my $hidden_fields = GetHiddenFields($mandatory, 'edit');
 
-    my $current_data = GetMember( borrowernumber => $borrowernumber );
 
     foreach my $key ( keys %new_data ) {
-        if ( $current_data->{$key} eq $new_data{$key} ) {
-            delete $new_data{$key};
+        next if defined($new_data{$key}) xor defined($current_data->{$key});
+        if ( !defined($new_data{$key}) || $current_data->{$key} eq $new_data{$key} || $hidden_fields->{$key} ) {
+           delete $new_data{$key};
         }
     }
 
@@ -466,51 +555,109 @@ sub DelEmptyFields {
     return %borrower;
 }
 
-sub GeneratePatronAttributesForm {
+sub FilterUnchangedAttributes {
     my ( $borrowernumber, $entered_attributes ) = @_;
 
-    # Get all attribute types and the values for this patron (if applicable)
-    my @types = C4::Members::AttributeTypes::GetAttributeTypes();
+    my @patron_attributes = grep {$_->type->opac_editable ? $_ : ()} Koha::Patron::Attributes->search({ borrowernumber => $borrowernumber })->as_list;
 
-    if (scalar(@types) == 0) {
-        return [];
+    my $patron_attribute_types;
+    foreach my $attr (@patron_attributes) {
+        $patron_attribute_types->{ $attr->code } += 1;
     }
 
-    my %attr_values = ();
+    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 ( $borrowernumber ) {
-        my $attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
+            if ( $changes ) {
+                foreach my $attr (@{ $entered_attributes }) {
+                    push @changed_attributes, $attr
+                        if $attr->{ code } eq $attribute_type;
+                }
+            }
+        }
+    }
 
-        # 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;
+    # 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;
+            }
         }
     }
 
-    if ( $entered_attributes ) {
+    return \@changed_attributes;
+}
+
+sub GeneratePatronAttributesForm {
+    my ( $borrowernumber, $entered_attributes ) = @_;
+
+    # Get all attribute types and the values for this patron (if applicable)
+    my @types = grep { $_->opac_editable() or $_->opac_display }
+        Koha::Patron::Attribute::Types->search()->as_list();
+    if ( scalar(@types) == 0 ) {
+        return [];
+    }
+
+    my @displayable_attributes = grep { $_->type->opac_display ? $_ : () }
+        Koha::Patron::Attributes->search({ borrowernumber => $borrowernumber })->as_list;
+
+    my %attr_values = ();
+
+    # 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;
+            push @{ $attr_values{ $attr->{code} } }, $attr->{value};
         }
     }
+    elsif ( defined $borrowernumber ) {
+        my @editable_attributes = grep { $_->type->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 { !$_->type->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() } || ['']
+        }
+            unless !defined $attr_values{ $attr_type->code() }
+                    and !$attr_type->opac_editable;
     }
 
     # Finally, build a list of containing classes
@@ -518,37 +665,66 @@ 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 ( $cgi ) = @_;
+    my ( $borrowernumber, $cgi ) = @_;
+
+    my @codes  = $cgi->multi_param('patron_attribute_code');
+    my @values = $cgi->multi_param('patron_attribute_value');
 
-    my @codes = $cgi->param('patron_attribute_code');
-    my @values = $cgi->param('patron_attribute_value');
-    my @passwords = $cgi->param('patron_attribute_password');
+    my @editable_attribute_types
+        = map { $_->code } Koha::Patron::Attribute::Types->search({ opac_editable => 1 });
 
-    my $ea = each_array( @codes, @values, @passwords );
+    my $ea = each_array( @codes, @values );
     my @attributes;
-    my %dups = ();
 
-    while ( my ( $code, $value, $password ) = $ea->() ) {
-        next unless defined($value) and $value ne '';
-        next if exists $dups{$code}->{$value};
-        $dups{$code}->{$value} = 1;
+    my $delete_candidates = {};
 
-        push @attributes, { code => $code, value => $value, password => $password };
+    while ( my ( $code, $value ) = $ea->() ) {
+        if ( any { $_ eq $code } @editable_attribute_types ) {
+            # It is an editable attribute
+            if ( !defined($value) or $value eq '' ) {
+                $delete_candidates->{$code} = 1
+                    unless $delete_candidates->{$code};
+            }
+            else {
+                # we've got a value
+                push @attributes, { code => $code, attribute => $value };
+
+                # 'code' is no longer a delete candidate
+                delete $delete_candidates->{$code}
+                    if defined $delete_candidates->{$code};
+            }
+        }
+    }
+
+    foreach my $code ( keys %{$delete_candidates} ) {
+        if ( not $borrowernumber # self-registration
+            || Koha::Patron::Attributes->search({
+                borrowernumber => $borrowernumber, code => $code })->count > 0 )
+        {
+            push @attributes, { code => $code, attribute => '' }
+                unless any { $_->{code} eq $code } @attributes;
+        }
     }
 
     return \@attributes;
 }
+
+
+1;