Attachment 116857 Details for Bug 8326 – Bug 8326: Make repeatable and unique_id modifiable when editing patron attribute type

[patch] Bug 8326: Make repeatable and unique_id modifiable when editing patron attribute type

Bug-8326-Make-repeatable-and-uniqueid-modifiable-w.patch (text/plain), 13.19 KB, created by Katrin Fischer on 2021-02-14 14:42:44 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Katrin Fischer

Created: 2021-02-14 14:42:44 UTC

Size: 13.19 KB

patch

obsolete

>From 41ad2ced8d672257a65af183ab2c7f02de67e113 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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 <david@davidnind.com>
>
>Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
>---
> Koha/Exceptions/Patron/Attribute/Type.pm           |  31 +++++++
> Koha/Patron/Attribute/Type.pm                      |  85 +++++++++++++++++
> admin/patron-attr-types.pl                         |  24 ++++-
> .../prog/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 @@
>        </li>
>        <li><label for="repeatable">Repeatable: </label>
>             [% IF attribute_type %]
>-                [% IF attribute_type.repeatable %]
>+                [% IF attribute_type.repeatable AND NOT can_be_set_to_nonrepeatable %]
>                     <input type="checkbox" id="repeatable" name="repeatable" checked="checked" disabled="disabled" />
>+                    <input type="hidden" name="repeatable" value="1" />
>+                [% ELSIF attribute_type.repeatable %]
>+                    <input type="checkbox" id="repeatable" name="repeatable" checked="checked" />
>                 [% ELSE %]
>-                    <input type="checkbox" id="repeatable" name="repeatable" disabled="disabled" />
>+                    <input type="checkbox" id="repeatable" name="repeatable" />
>                 [% END %]
>             [% ELSE %]
>                 <input type="checkbox" id="repeatable" name="repeatable" />
>             [% END %]
>-            <span>Check to let a patron record have multiple values of this attribute.
>-                  This setting cannot be changed after an attribute is defined.</span>
>+            <span>Check to let a patron record have multiple values of this attribute.</span>
>        </li>
>        <li><label for="unique_id">Unique identifier: </label>
>             [% IF attribute_type %]
>                 [% IF attribute_type.unique_id %]
>-                    <input type="checkbox" id="unique_id" name="unique_id" checked="checked" disabled="disabled" />
>+                    <input type="checkbox" id="unique_id" name="unique_id" checked="checked" />
>+                [% ELSIF can_be_set_to_unique %]
>+                    <input type="checkbox" id="unique_id" name="unique_id" />
>                 [% ELSE %]
>                     <input type="checkbox" id="unique_id" name="unique_id" disabled="disabled" />
>+                    <input type="hidden" name="unique_id" value="0" />
>                 [% END %]
>             [% ELSE %]
>                 <input type="checkbox" id="unique_id" name="unique_id" />
>             [% END %]
>             <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>
>+                  cannot be given to a different record.</span>
>        </li>
>        <li><label for="opac_display">Display in OPAC: </label>
>           [% 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.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 8326: 104517 | 116193 | 116194 | 116294 | 116483 | 116484 | 116857 | 116858 | 116859