Bug 12267: Remove borrower_attributes.password

When creating a patron attribute type, there is a "Allow password"
checkbox. If checked, the librarian will be able to enter a password for
this patron attribute when editing a patron.
The goal was to allow a patron to log in with a secondary password.
However, this feature has never been implemented.

"""
commit 6fc62bcd32
  CommitDate: Mon May 12 09:03:00 2008 -0500
  extended patron attributes tables & syspref (DB rev 081)

- password_allowed (if set, staff patron editor will
  allow a password to be associated with a value; this
  is mostly a hook for functionality to be implemented
  in the future.
"""

To decrease maintainability, this patch suggest to remove the 2 DB fields
borrower_attributes.password and
borrower_attribute_types.password_allowed
If they have not used by the library.

Test plan:
- Edit a patron attribute type and select "allow password"
- Edit a patron and defined a password for this attribute
- Execute the DB entry
- Note that you get a warning
- Empty the password field
- Execute the DB entry
- You do not get the warning and the 2 DB fields have been removed

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

C4/Members/AttributeTypes.pm

@@ -37,7 +37,6 @@ C4::Members::AttributeTypes - mananage extended patron attribute types
   $attr_type->repeatable($repeatable);
   $attr_type->unique_id($unique_id);
   $attr_type->opac_display($opac_display);
-  $attr_type->password_allowed($password_allowed);
   $attr_type->staff_searchable($staff_searchable);
   $attr_type->authorised_value_category($authorised_value_category);
   $attr_type->store();
@@ -123,7 +122,6 @@ sub new {
     $self->{'repeatable'} = 0;
     $self->{'unique_id'} = 0;
     $self->{'opac_display'} = 0;
-    $self->{'password_allowed'} = 0;
     $self->{'staff_searchable'} = 0;
     $self->{'display_checkout'} = 0;
     $self->{'authorised_value_category'} = '';
@@ -165,7 +163,6 @@ sub fetch {
     $self->{'repeatable'}                = $row->{'repeatable'};
     $self->{'unique_id'}                 = $row->{'unique_id'};
     $self->{'opac_display'}              = $row->{'opac_display'};
-    $self->{'password_allowed'}          = $row->{'password_allowed'};
     $self->{'staff_searchable'}          = $row->{'staff_searchable'};
     $self->{'display_checkout'}          = $row->{'display_checkout'};
     $self->{'authorised_value_category'} = $row->{'authorised_value_category'};
@@ -206,7 +203,6 @@ sub store {
                                          repeatable = ?,
                                          unique_id = ?,
                                          opac_display = ?,
-                                         password_allowed = ?,
                                          staff_searchable = ?,
                                          authorised_value_category = ?,
                                          display_checkout = ?,
@@ -215,23 +211,23 @@ sub store {
                                      WHERE code = ?");
     } else {
         $sth = $dbh->prepare_cached("INSERT INTO borrower_attribute_types 
-                                        (description, repeatable, unique_id, opac_display, password_allowed,
+                                        (description, repeatable, unique_id, opac_display,
                                          staff_searchable, authorised_value_category, display_checkout, category_code, class, code)
-                                        VALUES (?, ?, ?, ?, ?,
+                                        VALUES (?, ?, ?, ?,
                                                 ?, ?, ?, ?, ?, ?)");
     }
-    $sth->bind_param(1, $self->{'description'});
+    $sth->execute(
-    $sth->bind_param(2, $self->{'repeatable'});
+        $self->{description},
-    $sth->bind_param(3, $self->{'unique_id'});
+        $self->{repeatable},
-    $sth->bind_param(4, $self->{'opac_display'});
+        $self->{unique_id},
-    $sth->bind_param(5, $self->{'password_allowed'});
+        $self->{opac_display},
-    $sth->bind_param(6, $self->{'staff_searchable'});
+        $self->{staff_searchable},
-    $sth->bind_param(7, $self->{'authorised_value_category'});
+        $self->{authorised_value_category},
-    $sth->bind_param(8, $self->{'display_checkout'});
+        $self->{display_checkout},
-    $sth->bind_param(9, $self->{'category_code'} || undef);
+        $self->{category_code} || undef,
-    $sth->bind_param(10, $self->{'class'});
+        $self->{class},
-    $sth->bind_param(11, $self->{'code'});
+        $self->{code},
-    $sth->execute;
+    );
 
     if ( defined $$self{branches} ) {
         $sth = $dbh->prepare("DELETE FROM borrower_attribute_types_branches WHERE bat_code = ?");
@@ -323,6 +319,7 @@ sub unique_id {
     my $self = shift;
     @_ ? $self->{'unique_id'} = ((shift) ? 1 : 0) : $self->{'unique_id'};
 }
+
 =head2 opac_display
 
   my $opac_display = $attr_type->opac_display();
@@ -337,20 +334,7 @@ sub opac_display {
     my $self = shift;
     @_ ? $self->{'opac_display'} = ((shift) ? 1 : 0) : $self->{'opac_display'};
 }
-=head2 password_allowed
-
-  my $password_allowed = $attr_type->password_allowed();
-  $attr_type->password_allowed($password_allowed);
-
-Accessor.  The C<$password_allowed> argument
-is interpreted as a Perl boolean.
 
-=cut
-
-sub password_allowed {
-    my $self = shift;
-    @_ ? $self->{'password_allowed'} = ((shift) ? 1 : 0) : $self->{'password_allowed'};
-}
 =head2 staff_searchable
 
   my $staff_searchable = $attr_type->staff_searchable();

C4/Members/Attributes.pm

@@ -59,7 +59,6 @@ code (attribute type code)
 description (attribute type description)
 value (attribute value)
 value_description (attribute value description (if associated with an authorised value))
-password (password, if any, associated with attribute
 
 If the C<$opac_only> parameter is present and has a true value, only the attributes
 marked for OPAC display are returned.
@@ -71,7 +70,7 @@ sub GetBorrowerAttributes {
     my $opac_only = @_ ? shift : 0;
 
     my $dbh = C4::Context->dbh();
-    my $query = "SELECT code, description, attribute, lib, password, display_checkout, category_code, class
+    my $query = "SELECT code, description, attribute, lib, display_checkout, category_code, class
                  FROM borrower_attributes
                  JOIN borrower_attribute_types USING (code)
                  LEFT JOIN authorised_values ON (category = authorised_value_category AND attribute = authorised_value)
@@ -87,7 +86,6 @@ sub GetBorrowerAttributes {
             description       => $row->{'description'},
             value             => $row->{'attribute'},
             value_description => $row->{'lib'},
-            password          => $row->{'password'},
             display_checkout  => $row->{'display_checkout'},
             category_code     => $row->{'category_code'},
             class             => $row->{'class'},
@@ -205,7 +203,7 @@ sub CheckUniqueness {
 
 =head2 SetBorrowerAttributes 
 
-  SetBorrowerAttributes($borrowernumber, [ { code => 'CODE', value => 'value', password => 'password' }, ... ] );
+  SetBorrowerAttributes($borrowernumber, [ { code => 'CODE', value => 'value' }, ... ] );
 
 Set patron attributes for the patron identified by C<$borrowernumber>,
 replacing any that existed previously.
@@ -221,11 +219,10 @@ sub SetBorrowerAttributes {
 
     DeleteBorrowerAttributes( $borrowernumber, $no_branch_limit );
 
-    my $sth = $dbh->prepare("INSERT INTO borrower_attributes (borrowernumber, code, attribute, password)
+    my $sth = $dbh->prepare("INSERT INTO borrower_attributes (borrowernumber, code, attribute)
-                             VALUES (?, ?, ?, ?)");
+                             VALUES (?, ?, ?)");
     foreach my $attr (@$attr_list) {
-        $attr->{password} = undef unless exists $attr->{password};
+        $sth->execute($borrowernumber, $attr->{code}, $attr->{value});
-        $sth->execute($borrowernumber, $attr->{code}, $attr->{value}, $attr->{password});
         if ($sth->err) {
             warn sprintf('Database returned the following error: %s', $sth->errstr);
             return; # bail immediately on errors
@@ -301,10 +298,6 @@ sub UpdateBorrowerAttribute {
     my $dbh = C4::Context->dbh;
     my $query = "INSERT INTO borrower_attributes SET attribute = ?, code = ?, borrowernumber = ?";
     my @params = ( $attribute->{attribute}, $attribute->{code}, $borrowernumber );
-    if ( defined $attribute->{password} ) {
-        $query .= ", password = ?";
-        push @params, $attribute->{password};
-    }
     my $sth = $dbh->prepare( $query );
 
     $sth->execute( @params );

admin/patron-attr-types.pl

@@ -113,9 +113,6 @@ sub error_add_attribute_type_form {
     if ($input->param('unique_id')) {
         $template->param(unique_id_checked => 1);
     }
-    if ($input->param('password_allowed')) {
-        $template->param(password_allowed_checked => 1);
-    }
     if ($input->param('opac_display')) {
         $template->param(opac_display_checked => 1);
     }
@@ -167,8 +164,6 @@ sub add_update_attribute_type {
     $attr_type->staff_searchable($staff_searchable);
     my $authorised_value_category = $input->param('authorised_value_category');
     $attr_type->authorised_value_category($authorised_value_category);
-    my $password_allowed = $input->param('password_allowed');
-    $attr_type->password_allowed($password_allowed);
     my $display_checkout = $input->param('display_checkout');
     $attr_type->display_checkout($display_checkout);
     $attr_type->category_code($input->param('category_code'));
@@ -242,9 +237,6 @@ sub edit_attribute_type_form {
         $template->param(unique_id_checked => 1);
     }
     $template->param(unique_id_disabled => 1);
-    if ($attr_type->password_allowed()) {
-        $template->param(password_allowed_checked => 1);
-    }
     if ($attr_type->opac_display()) {
         $template->param(opac_display_checked => 1);
     }

installer/data/mysql/atomicupdate/bug_12267.perl

@@ -0,0 +1,17 @@
+my $dbh = C4::Context->dbh;
+my ( $column_has_been_used ) = $dbh->selectrow_array(q|
+    SELECT COUNT(*)
+    FROM borrower_attributes
+    WHERE password IS NOT NULL
+|);
+
+if ( $column_has_been_used ) {
+    warn q|WARNING: The columns borrower_attribute_types.password_allowed and borrower_attributes.column have been removed from the Koha codebase. There were not used. However your installation has at least 1 borrower_attributes.password defined. In order not to alter your data, the columns have been kept, please remove them manually if you do not use them this feature.|;
+} else {
+    $dbh->do(q|
+        ALTER TABLE borrower_attribute_types DROP column password_allowed
+    |);
+    $dbh->do(q|
+        ALTER TABLE borrower_attributes DROP column password;
+    |);
+}

installer/data/mysql/kohastructure.sql

@@ -292,7 +292,6 @@ CREATE TABLE `borrower_attribute_types` ( -- definitions for custom patron field
   `repeatable` tinyint(1) NOT NULL default 0, -- defines whether one patron/borrower can have multiple values for this custom field  (1 for yes, 0 for no)
   `unique_id` tinyint(1) NOT NULL default 0, -- defines if this value needs to be unique (1 for yes, 0 for no)
   `opac_display` tinyint(1) NOT NULL default 0, -- defines if this field is visible to patrons on their account in the OPAC (1 for yes, 0 for no)
-  `password_allowed` tinyint(1) NOT NULL default 0, -- defines if it is possible to associate a password with this custom field (1 for yes, 0 for no)
   `staff_searchable` tinyint(1) NOT NULL default 0, -- defines if this field is searchable via the patron search in the staff client (1 for yes, 0 for no)
   `authorised_value_category` varchar(32) default NULL, -- foreign key from authorised_values that links this custom field to an authorized value category
   `display_checkout` tinyint(1) NOT NULL default 0,-- defines if this field displays in checkout screens
@@ -311,7 +310,6 @@ CREATE TABLE `borrower_attributes` ( -- values of custom patron fields known as
   `borrowernumber` int(11) NOT NULL, -- foreign key from the borrowers table, defines which patron/borrower has this attribute
   `code` varchar(10) NOT NULL, -- foreign key from the borrower_attribute_types table, defines which custom field this value was entered for
   `attribute` varchar(255) default NULL, -- custom patron field value
-  `password` varchar(64) default NULL, -- password associated with this field
   KEY `borrowernumber` (`borrowernumber`),
   KEY `code_attribute` (`code`, `attribute`),
   CONSTRAINT `borrower_attributes_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`)

koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt

@@ -139,14 +139,6 @@ function CheckAttributeTypeForm(f) {
             <span>If checked, attribute will be a unique identifier &mdash; if a value is given to a patron record, the same value
                   cannot be given to a different record.  This setting cannot be changed after an attribute is defined.</span>
        </li>
-       <li><label for="password_allowed">Allow password: </label>
-          [% IF ( password_allowed_checked ) %]
-            <input type="checkbox" id="password_allowed" name="password_allowed" checked="checked" />
-          [% ELSE %]
-            <input type="checkbox" id="password_allowed" name="password_allowed" />
-          [% END %]
-            <span>Check to make it possible to associate a password with this attribute.</span>
-       </li>
        <li><label for="opac_display">Display in OPAC: </label>
           [% IF ( opac_display_checked ) %]
             <input type="checkbox" id="opac_display" name="opac_display" checked="checked" />

koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt

@@ -1062,10 +1062,6 @@ function select_user(borrowernumber, borrower) {
                         [% ELSE %]
                             <textarea rows="2" cols="30" id="[% patron_attribute.form_id %]" name="[% patron_attribute.form_id %]">[% patron_attribute.value %]</textarea>
                         [% END %]
-                        [% IF ( patron_attribute.password_allowed ) %]
-                            (<label class="yesno" for="[% patron_attribute.form_id %]_password">Password:</label> <input type="password" maxlength="64" value="[% patron_attribute.password %]"
-                                   id="[% patron_attribute.form_id %]_password" name="[% patron_attribute.form_id %]_password" />)
-                        [% END %]
                         <a href="#" onclick="clear_entry(this); return false;"><i class="fa fa-fw fa-trash"></i> Clear</a>
                         [% IF ( patron_attribute.repeatable ) %]
                         <a href="#" onclick="clone_entry(this); return false;"><i class="fa fa-fw fa-plus"></i> New</a>

members/memberentry.pl

@@ -718,11 +718,10 @@ sub  parse_extended_patron_attributes {
     foreach my $key (@patron_attr) {
         my $value = $input->param($key);
         next unless defined($value) and $value ne '';
-        my $password = $input->param("${key}_password");
         my $code     = $input->param("${key}_code");
         next if exists $dups{$code}->{$value};
         $dups{$code}->{$value} = 1;
-        push @attr, { code => $code, value => $value, password => $password };
+        push @attr, { code => $code, value => $value };
     }
     return \@attr;
 }
@@ -756,16 +755,13 @@ sub patron_attributes_form {
             code              => $attr_type->code(),
             description       => $attr_type->description(),
             repeatable        => $attr_type->repeatable(),
-            password_allowed  => $attr_type->password_allowed(),
             category          => $attr_type->authorised_value_category(),
             category_code     => $attr_type->category_code(),
-            password          => '',
         };
         if (exists $attr_hash{$attr_type->code()}) {
             foreach my $attr (@{ $attr_hash{$attr_type->code()} }) {
                 my $newentry = { %$entry };
                 $newentry->{value} = $attr->{value};
-                $newentry->{password} = $attr->{password};
                 $newentry->{use_dropdown} = 0;
                 if ($attr_type->authorised_value_category()) {
                     $newentry->{use_dropdown} = 1;

t/Members_AttributeTypes.t

@@ -48,13 +48,13 @@ fixtures_ok [
     [
         'code',             'description',
         'repeatable',       'unique_id',
-        'opac_display',     'password_allowed',
+        'opac_display',
         'staff_searchable', 'authorised_value_category',
         'display_checkout', 'category_code',
         'class'
     ],
-    [ 'one', 'ISBN', '1', '1', '1', '1', '1', 'red',  '1', 'orange', 'green' ],
+    [ 'one', 'ISBN', '1', '1', '1', '1', 'red',  '1', 'orange', 'green' ],
-    [ 'two', 'ISSN', '0', '0', '0', '0', '0', 'blue', '0', 'yellow', 'silver' ]
+    [ 'two', 'ISSN', '0', '0', '0', '0', 'blue', '0', 'yellow', 'silver' ]
 
     ],
 ], 'add fixtures';

t/Members_Attributes.t

@@ -17,7 +17,6 @@ INIT {
                        'opac_display' => '1',
                        'staff_searchable' => '1',
                        'description' => 'Grade level',
-                       'password_allowed' => '0',
                        'authorised_value_category' => '',
                        'repeatable' => '0',
                        'code' => 'grade',
@@ -27,7 +26,6 @@ INIT {
                            'opac_display' => '0',
                            'staff_searchable' => '1',
                            'description' => 'Deans List (annual)',
-                           'password_allowed' => '0',
                            'authorised_value_category' => '',
                            'repeatable' => '1',
                            'code' => 'deanslist',
@@ -37,7 +35,6 @@ INIT {
                            'opac_display' => '0',
                            'staff_searchable' => '0',
                            'description' => 'Some Ext. Attribute',
-                           'password_allowed' => '0',
                            'authorised_value_category' => '',
                            'repeatable' => '0',
                            'code' => 'somedata',
@@ -47,7 +44,6 @@ INIT {
                            'opac_display' => '0',
                            'staff_searchable' => '0',
                            'description' => 'Another Ext. Attribute',
-                           'password_allowed' => '0',
                            'authorised_value_category' => '',
                            'repeatable' => '0',
                            'code' => 'extradata',
@@ -57,7 +53,6 @@ INIT {
                            'opac_display' => '1',
                            'staff_searchable' => '1',
                            'description' => 'School ID Number',
-                           'password_allowed' => '0',
                            'authorised_value_category' => '',
                            'repeatable' => '0',
                            'code' => 'school_id',
@@ -67,7 +62,6 @@ INIT {
                           'opac_display' => '1',
                           'staff_searchable' => '1',
                           'description' => 'Homeroom',
-                          'password_allowed' => '0',
                           'authorised_value_category' => '',
                           'repeatable' => '0',
                           'code' => 'homeroom',