From 3b406fcf55498307d8a89506dedbb20c8e282396 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 11 Apr 2016 10:05:01 +0100 Subject: [PATCH] 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 6fc62bcd321eddb0fd3ae46903e9ab6c8b1db2cd 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 --- C4/Members/AttributeTypes.pm | 35 +++------------------- C4/Members/Attributes.pm | 17 ++++------- admin/patron-attr-types.pl | 8 ----- installer/data/mysql/atomicupdate/bug_12267.perl | 18 +++++++++++ installer/data/mysql/kohastructure.sql | 2 -- .../prog/en/modules/admin/patron-attr-types.tt | 8 ----- .../prog/en/modules/members/memberentrygen.tt | 4 --- members/memberentry.pl | 6 +--- t/Members_AttributeTypes.t | 6 ++-- t/Members_Attributes.t | 6 ---- 10 files changed, 31 insertions(+), 79 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_12267.perl diff --git a/C4/Members/AttributeTypes.pm b/C4/Members/AttributeTypes.pm index eb4346d..be236f0 100644 --- a/C4/Members/AttributeTypes.pm +++ b/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,12 @@ 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->bind_param(2, $self->{'repeatable'}); - $sth->bind_param(3, $self->{'unique_id'}); - $sth->bind_param(4, $self->{'opac_display'}); - $sth->bind_param(5, $self->{'password_allowed'}); - $sth->bind_param(6, $self->{'staff_searchable'}); - $sth->bind_param(7, $self->{'authorised_value_category'}); - $sth->bind_param(8, $self->{'display_checkout'}); - $sth->bind_param(9, $self->{'category_code'} || undef); - $sth->bind_param(10, $self->{'class'}); - $sth->bind_param(11, $self->{'code'}); - $sth->execute; + $sth->execute( $self->{description}, $self->{repeatable}, $self->{unique_id}, $self->{opac_display}, $self->{staff_searchable}, $self->{authorised_value_category}, $self->{display_checkout}, $self->{category_code} || undef, $self->{class}, $self->{code} ); if ( defined $$self{branches} ) { $sth = $dbh->prepare("DELETE FROM borrower_attribute_types_branches WHERE bat_code = ?"); @@ -323,6 +308,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 +323,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(); diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index eedaae1..1036e58 100644 --- a/C4/Members/Attributes.pm +++ b/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) - VALUES (?, ?, ?, ?)"); + my $sth = $dbh->prepare("INSERT INTO borrower_attributes (borrowernumber, code, attribute) + VALUES (?, ?, ?)"); foreach my $attr (@$attr_list) { - $attr->{password} = undef unless exists $attr->{password}; - $sth->execute($borrowernumber, $attr->{code}, $attr->{value}, $attr->{password}); + $sth->execute($borrowernumber, $attr->{code}, $attr->{value}); 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 ); diff --git a/admin/patron-attr-types.pl b/admin/patron-attr-types.pl index b233854..3c978e9 100755 --- a/admin/patron-attr-types.pl +++ b/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); } diff --git a/installer/data/mysql/atomicupdate/bug_12267.perl b/installer/data/mysql/atomicupdate/bug_12267.perl new file mode 100644 index 0000000..c9139a7 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_12267.perl @@ -0,0 +1,18 @@ +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; + |); +} + diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index e6e3142..242755d 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/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`) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt index 3ddb09c..bb6ed53 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt @@ -139,14 +139,6 @@ function CheckAttributeTypeForm(f) { If checked, attribute will be a unique identifier — 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. -
  • - [% IF ( password_allowed_checked ) %] - - [% ELSE %] - - [% END %] - Check to make it possible to associate a password with this attribute. -
  • [% IF ( opac_display_checked ) %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt index d54c29e..badb1a5 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -1060,10 +1060,6 @@ function select_user(borrowernumber, borrower) { [% ELSE %] [% END %] - [% IF ( patron_attribute.password_allowed ) %] - ( ) - [% END %] Clear [% IF ( patron_attribute.repeatable ) %] New diff --git a/members/memberentry.pl b/members/memberentry.pl index a190740..b899a26 100755 --- a/members/memberentry.pl +++ b/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; diff --git a/t/Members_AttributeTypes.t b/t/Members_AttributeTypes.t index 6bb58ae..4b4d5a1 100755 --- a/t/Members_AttributeTypes.t +++ b/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' ], - [ 'two', 'ISSN', '0', '0', '0', '0', '0', 'blue', '0', 'yellow', 'silver' ] + [ 'one', 'ISBN', '1', '1', '1', '1', 'red', '1', 'orange', 'green' ], + [ 'two', 'ISSN', '0', '0', '0', '0', 'blue', '0', 'yellow', 'silver' ] ], ], 'add fixtures'; diff --git a/t/Members_Attributes.t b/t/Members_Attributes.t index aafd9a9..205d611 100755 --- a/t/Members_Attributes.t +++ b/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', -- 2.7.0