Attachment 118831 Details for Bug 28031 – Bug 28031: (follow-up) Clarify check methods

[patch] Bug 28031: (follow-up) Clarify check methods

Bug-28031-follow-up-Clarify-check-methods.patch (text/plain), 4.73 KB, created by Martin Renvoize (ashimema) on 2021-03-25 17:26:18 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2021-03-25 17:26:18 UTC

Size: 4.73 KB

patch

obsolete

>From 22a215e58811a5a34420550effc8a4ab3d32b420 Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Thu, 25 Mar 2021 17:22:29 +0000
>Subject: [PATCH] Bug 28031: (follow-up) Clarify check methods
>
>The check methods were positioned under the 'Internal methods' section
>of the meodule but are used externally.
>
>It also felt strange to have a noop or die method. Instead, I propose
>renaming them to `repeatable_ok` and `unique_ok` and returning a
>boolean denoting their state.
>---
> Koha/Patron/Attribute.pm | 42 +++++++++++++++++++++-------------------
> members/memberentry.pl   |  3 +--
> opac/opac-memberentry.pl |  3 +--
> 3 files changed, 24 insertions(+), 24 deletions(-)
>
>diff --git a/Koha/Patron/Attribute.pm b/Koha/Patron/Attribute.pm
>index e8b0faa673..a8dcd64b92 100644
>--- a/Koha/Patron/Attribute.pm
>+++ b/Koha/Patron/Attribute.pm
>@@ -51,8 +51,11 @@ sub store {
>     Koha::Exceptions::Patron::Attribute::InvalidType->throw( type => $self->code )
>         unless $type;
> 
>-    $self->check_repeatable($type);
>-    $self->check_unique_id($type);
>+    Koha::Exceptions::Patron::Attribute::NonRepeatable->throw( attribute => $self )
>+        unless $self->repeatable_ok($type);
>+
>+    Koha::Exceptions::Patron::Attribute::UniqueIDConstraint->throw( attribute => $self )
>+        unless $self->unique_ok($type);
> 
>     return $self->SUPER::store();
> }
>@@ -133,20 +136,18 @@ sub to_api_mapping {
>     };
> }
> 
>-=head2 Internal methods
>-
>-=head3 check_repeatable
>+=head3 repeatable_ok
> 
>-check_repeatable checks if the attribute type is repeatable and throws and exception
>-if the attribute type isn't repeatable and there's already an attribute with the same
>-code for the given patron.
>+Checks if the attribute type is repeatable and returns a boolean representing 
>+whether storing the current object state would break the repeatable constraint.
> 
> =cut
> 
>-sub check_repeatable {
>+sub repeatable_ok {
> 
>     my ( $self, $type ) = @_;
> 
>+    my $ok = 1;
>     if ( !$type->repeatable ) {
>         my $params = {
>             borrowernumber => $self->borrowernumber,
>@@ -156,25 +157,24 @@ sub check_repeatable {
>         $params->{id} = { '!=' => $self->id }
>             if $self->in_storage;
> 
>-        Koha::Exceptions::Patron::Attribute::NonRepeatable->throw( attribute => $self )
>-            if Koha::Patron::Attributes->search($params)->count > 0;
>+        $ok = 0 if Koha::Patron::Attributes->search($params)->count > 0;
>     }
> 
>-    return $self;
>+    return $ok;
> }
> 
>-=head3 check_unique_id
>+=head3 unique_ok
> 
>-check_unique_id checks if the attribute type is marked as unique id and throws and exception
>-if the attribute type is a unique id and there's already an attribute with the same
>-code and value on the database.
>+Checks if the attribute type is marked as unique and returns a boolean representing
>+whether storing the current object state would break the unique constraint.
> 
> =cut
> 
>-sub check_unique_id {
>+sub unique_ok {
> 
>     my ( $self, $type ) = @_;
> 
>+    my $ok = 1;
>     if ( $type->unique_id ) {
>         my $params = { code => $self->code, attribute => $self->attribute };
> 
>@@ -184,13 +184,15 @@ sub check_unique_id {
>         my $unique_count = Koha::Patron::Attributes
>             ->search( $params )
>             ->count;
>-        Koha::Exceptions::Patron::Attribute::UniqueIDConstraint->throw( attribute => $self )
>-            if $unique_count > 0;
>+
>+        $ok = 0 if $unique_count > 0;
>     }
> 
>-    return $self;
>+    return $ok;
> }
> 
>+=head2 Internal methods
>+
> =head3 _type
> 
> =cut
>diff --git a/members/memberentry.pl b/members/memberentry.pl
>index e6be7854a0..4a1a4316cc 100755
>--- a/members/memberentry.pl
>+++ b/members/memberentry.pl
>@@ -411,8 +411,7 @@ if ($op eq 'save' || $op eq 'insert'){
>       for my $attr ( @$extended_patron_attributes ) {
>           $attr->{borrowernumber} = $borrowernumber if $borrowernumber;
>           my $attribute = Koha::Patron::Attribute->new($attr);
>-          eval {$attribute->check_unique_id};
>-          if ( $@ ) {
>+          if ( !$attribute->unique_ok ) {
>               push @errors, "ERROR_extended_unique_id_failed";
>               my $attr_type = Koha::Patron::Attribute::Types->find($attr->{code});
>               $template->param(
>diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
>index dbfd6f83dd..015c56a3da 100755
>--- a/opac/opac-memberentry.pl
>+++ b/opac/opac-memberentry.pl
>@@ -107,8 +107,7 @@ my $conflicting_attribute = 0;
> 
> foreach my $attr (@$attributes) {
>     my $attribute = Koha::Patron::Attribute->new($attr);
>-    eval {$attribute->check_unique_id};
>-    if ( $@ ) {
>+    if ( !$$attribute->unique_ok ) {
>         my $attr_type = Koha::Patron::Attribute::Types->find($attr->{code});
>         $template->param(
>             extended_unique_id_failed_code => $attr->{code},
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 28031: 118792 | 118793 | 118794 | 118828 | 118829 | 118830 | 118831 | 118833 | 118859 | 118860 | 118861 | 118862 | 118863 | 119377 | 119378 | 119379 | 119380 | 119382