From a066f03b5e89f1a031565c024c05864449153ba6 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 7 May 2020 15:36:24 +0200 Subject: [PATCH] Bug 8326: Make repeatable and unique_id modifiable when editing patron attribute type This patch add the ability to modify the attribute "repeatable" and "unique_id" for a patron's attribute type. Prior to this patch it was not possible, to keep data integrity. When editing an attr type, the controller will check is the value can be modified, depending on the existing patron's attributes. Test plan: 0/ Setup Create 1 patron attribute PA1 that can be repeatable Create 1 patron attribute PA2 that does not have the unique restriction Create 1 patron attribute PA3 that cannot be repeatable and does not have the unique restriction 1/ Edit them and confirm that you can modify the repeatable and unique restrictions 2/ Restore values from 0. Create a patron P1 with several PA1, PA2=42 and whatever in PA3 3/ Edit PA1 => you cannot remove the repeatable flag but can still remove the unique 4/ Create a patron P2 with PA2=42 5/ Edit PA2 => you cannot add the unique flag Play a bit more with the different combinaisons and confirm that it works as advertised. Signed-off-by: David Nind Signed-off-by: Katrin Fischer Signed-off-by: Jonathan Druart --- Koha/Exceptions/Patron/Attribute/Type.pm | 31 ++++++ Koha/Patron/Attribute/Type.pm | 85 +++++++++++++++ admin/patron-attr-types.pl | 24 ++++- .../en/modules/admin/patron-attr-types.tt | 17 +-- t/db_dependent/Koha/Patron/Attribute/Types.t | 102 +++++++++++++++++- 5 files changed, 249 insertions(+), 10 deletions(-) create mode 100644 Koha/Exceptions/Patron/Attribute/Type.pm diff --git a/Koha/Exceptions/Patron/Attribute/Type.pm b/Koha/Exceptions/Patron/Attribute/Type.pm new file mode 100644 index 0000000000..741b2f7254 --- /dev/null +++ b/Koha/Exceptions/Patron/Attribute/Type.pm @@ -0,0 +1,31 @@ +package Koha::Exceptions::Patron::Attribute::Type; + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Patron::Attribute::Type' => { + description => 'Something went wrong' + }, + 'Koha::Exceptions::Patron::Attribute::Type::CannotChangeProperty' => { + isa => 'Koha::Exceptions::Patron::Attribute::Type', + description => "Cannot change property", + fields => ['property'], + }, +); + +sub full_message { + my $self = shift; + + my $msg = $self->message; + + unless ( $msg) { + if ( $self->isa('Koha::Exceptions::Patron::Attribute::Type::CannotChangeProperty') ) { + $msg = sprintf("The property '%s' cannot be changed, some patron attributes are using it that way.", $self->property ); + } + } + + return $msg; +} + +1; diff --git a/Koha/Patron/Attribute/Type.pm b/Koha/Patron/Attribute/Type.pm index 6e7d00589d..772cad3c43 100644 --- a/Koha/Patron/Attribute/Type.pm +++ b/Koha/Patron/Attribute/Type.pm @@ -18,6 +18,7 @@ package Koha::Patron::Attribute::Type; use Modern::Perl; use Koha::Database; +use Koha::Exceptions::Patron::Attribute::Type; use base qw(Koha::Object Koha::Object::Limit::Library); @@ -31,6 +32,90 @@ Koha::Patron::Attribute::Type - Koha::Patron::Attribute::Type Object class =cut +=head3 store + + my $attribute = Koha::Patron::Attribute->new({ code => 'a_code', ... }); + try { $attribute->store } + catch { handle_exception }; + +=cut + +sub store { + + my $self = shift; + + $self->check_repeatables; + $self->check_unique_ids; + + return $self->SUPER::store(); +} + +=head3 attributes + +=cut + +sub attributes { + my ($self) = @_; + my $attributes_rs = $self->_result->borrower_attributes; + Koha::Patron::Attributes->_new_from_dbic($attributes_rs); +} + +=head2 Internal Methods + +=cut + +=head3 check_repeatables + +=cut + +sub check_repeatables { + my ($self) = @_; + + return $self if $self->repeatable; + + my $count = $self->attributes->search( + {}, + { + select => [ { count => 'id', '-as' => 'c' } ], + group_by => 'borrowernumber', + having => { c => { '>' => 1 } } + } + )->count; + + Koha::Exceptions::Patron::Attribute::Type::CannotChangeProperty->throw( + property => 'repeatable' ) + if $count; + + return $self; +} + +=head3 check_unique_ids + +=cut + +sub check_unique_ids { + my ($self) = @_; + + return $self unless $self->unique_id; + + my $count = $self->attributes->search( + {}, + { + select => [ { count => 'id', '-as' => 'c' } ], + group_by => 'attribute', + having => { c => { '>' => 1 } } + } + )->count; + + Koha::Exceptions::Patron::Attribute::Type::CannotChangeProperty->throw( + property => 'unique_id' ) + if $count; + + return $self; +} + + + =head3 _type =cut diff --git a/admin/patron-attr-types.pl b/admin/patron-attr-types.pl index af58243193..1924c0660f 100755 --- a/admin/patron-attr-types.pl +++ b/admin/patron-attr-types.pl @@ -142,13 +142,14 @@ sub add_update_attribute_type { { code => $code, description => $description, - repeatable => $repeatable, - unique_id => $unique_id, } ); } + $attr_type->set( { + repeatable => $repeatable, + unique_id => $unique_id, opac_display => $opac_display, opac_editable => $opac_editable, staff_searchable => $staff_searchable, @@ -219,12 +220,29 @@ sub edit_attribute_type_form { my $code = shift; my $attr_type = Koha::Patron::Attribute::Types->find($code); - $template->param(attribute_type => $attr_type); my $patron_categories = Koha::Patron::Categories->search({}, {order_by => ['description']}); + + my $can_be_set_to_nonrepeatable = 1; + if ( $attr_type->repeatable == 1 ) { + $attr_type->repeatable(0); + eval {$attr_type->check_repeatables}; + $can_be_set_to_nonrepeatable = 0 if $@; + $attr_type->repeatable(1); + } + my $can_be_set_to_unique = 1; + if ( $attr_type->unique_id == 0 ) { + $attr_type->unique_id(1); + eval {$attr_type->check_unique_ids}; + $can_be_set_to_unique = 0 if $@; + $attr_type->unique_id(0); + } $template->param( + attribute_type => $attr_type, attribute_type_form => 1, edit_attribute_type => 1, + can_be_set_to_nonrepeatable => $can_be_set_to_nonrepeatable, + can_be_set_to_unique => $can_be_set_to_unique, confirm_op => 'edit_attribute_type_confirmed', categories => $patron_categories, ); 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 dd1937604e..74db9cd1dd 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 @@ -79,29 +79,34 @@
  • [% IF attribute_type %] - [% IF attribute_type.repeatable %] + [% IF attribute_type.repeatable AND NOT can_be_set_to_nonrepeatable %] + + [% ELSIF attribute_type.repeatable %] + [% ELSE %] - + [% END %] [% ELSE %] [% END %] - Check to let a patron record have multiple values of this attribute. - This setting cannot be changed after an attribute is defined. + Check to let a patron record have multiple values of this attribute.
  • [% IF attribute_type %] [% IF attribute_type.unique_id %] - + + [% ELSIF can_be_set_to_unique %] + [% ELSE %] + [% END %] [% ELSE %] [% END %] 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. + cannot be given to a different record.
  • [% IF attribute_type AND attribute_type.opac_display %] diff --git a/t/db_dependent/Koha/Patron/Attribute/Types.t b/t/db_dependent/Koha/Patron/Attribute/Types.t index 7f47652536..a8fd71d07f 100755 --- a/t/db_dependent/Koha/Patron/Attribute/Types.t +++ b/t/db_dependent/Koha/Patron/Attribute/Types.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 6; +use Test::More tests => 7; use t::lib::TestBuilder; use t::lib::Mocks; @@ -69,6 +69,106 @@ subtest 'new() tests' => sub { $schema->storage->txn_rollback; }; +subtest 'store' => sub { + + plan tests => 4; + + $schema->storage->txn_begin; + + # Create 2 attribute types without restrictions: + # Repeatable and can have the same values + my $attr_type_1 = $builder->build_object( + { + class => 'Koha::Patron::Attribute::Types', + value => { repeatable => 1, unique_id => 0 } + } + ); + my $attr_type_2 = $builder->build_object( + { + class => 'Koha::Patron::Attribute::Types', + value => { repeatable => 1, unique_id => 0 } + } + ); + + # Patron 1 has twice the attribute 1 and attribute 2 + # Patron 2 has attribute 1 and attribute 2="42" + # Patron 3 has attribute 2="42" + # Attribute 1 cannot remove repeatable + # Attribute 2 cannot set unique_id + my $patron_1 = $builder->build_object( { class => 'Koha::Patrons' } ); + my $patron_2 = $builder->build_object( { class => 'Koha::Patrons' } ); + my $patron_3 = $builder->build_object( { class => 'Koha::Patrons' } ); + + my $attribute_111 = $builder->build_object( + { + class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron_1->borrowernumber, + code => $attr_type_1->code + } + } + ); + my $attribute_112 = $builder->build_object( + { + class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron_1->borrowernumber, + code => $attr_type_1->code + } + } + ); + + my $attribute_211 = $builder->build_object( + { + class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron_2->borrowernumber, + code => $attr_type_1->code + } + } + ); + my $attribute_221 = $builder->build_object( + { + class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron_2->borrowernumber, + code => $attr_type_2->code, + attribute => '42', + } + } + ); + + my $attribute_321 = $builder->build_object( + { + class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron_3->borrowernumber, + code => $attr_type_2->code, + attribute => '42', + } + } + ); + + throws_ok { + $attr_type_1->repeatable(0)->store; + } + 'Koha::Exceptions::Patron::Attribute::Type::CannotChangeProperty', ""; + + $attribute_112->delete; + ok($attr_type_1->set({ unique_id => 1, repeatable => 0 })->store); + + throws_ok { + $attr_type_2->unique_id(1)->store; + } + 'Koha::Exceptions::Patron::Attribute::Type::CannotChangeProperty', ""; + + $attribute_321->attribute(43)->store; + ok($attr_type_2->set({ unique_id => 1, repeatable => 0 })->store); + + $schema->storage->txn_rollback; + +}; + subtest 'library_limits() tests' => sub { plan tests => 13; -- 2.39.5