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