From 876d0597f422129d28a369e624b7aa133555549d Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
Date: Fri, 24 Sep 2021 12:02:16 -0300
Subject: [PATCH] Bug 29110: Refactor Koha::ArticleRequest

This patchset is simple to test. It adds lots of tests for the existing
routines, and then tweaks them.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/ArticleRequests.t \
           t/db_dependent/Koha/ArticleRequest*
=> SUCCESS: Tests pass!
3. Sign off :-D
---
 Koha/ArticleRequest.pm               | 187 ++++++++++++++-------------
 Koha/Schema/Result/ArticleRequest.pm |  38 +++++-
 2 files changed, 133 insertions(+), 92 deletions(-)

diff --git a/Koha/ArticleRequest.pm b/Koha/ArticleRequest.pm
index 21b3751ac5..d23a9aee2f 100644
--- a/Koha/ArticleRequest.pm
+++ b/Koha/ArticleRequest.pm
@@ -27,6 +27,7 @@ use Koha::Items;
 use Koha::Libraries;
 use Koha::DateUtils qw( dt_from_string );
 use Koha::ArticleRequest::Status;
+use Koha::Exceptions;
 use Koha::Exceptions::ArticleRequest;
 
 use base qw(Koha::Object);
@@ -39,8 +40,45 @@ Koha::ArticleRequest - Koha Article Request Object class
 
 =head2 Class methods
 
+=head3 store
+
+Overridden I<store> method that performs some checks and triggers.
+
 =cut
 
+sub store {
+    my ($self) = @_;
+
+    if ( !$self->in_storage ) {
+        # new request
+        Koha::Exceptions::MissingParameter->throw(
+            "Missing mandatory parameter: borrowernumber")
+          unless $self->borrowernumber;
+
+        my $patron = Koha::Patrons->find( $self->borrowernumber );
+
+        Koha::Exceptions::Object::FKConstraint->throw(
+            broken_fk => 'borrowernumber',
+            value     => $self->borrowernumber
+        ) unless $patron;
+
+        Koha::Exceptions::ArticleRequest::LimitReached->throw(
+            error => 'Patron cannot request more articles for today' )
+          unless $patron->can_request_article;
+
+        $self->created_on( dt_from_string() );
+        # this is to avoid calling ->discard_changes to retrieve
+        # the default value from the DB
+        $self->status( Koha::ArticleRequest::Status::Requested )
+            unless $self->status;
+    }
+
+    $self->SUPER::store;
+    $self->notify;
+
+    return $self;
+}
+
 =head3 request
 
 =cut
@@ -48,14 +86,8 @@ Koha::ArticleRequest - Koha Article Request Object class
 sub request {
     my ($self) = @_;
 
-    Koha::Exceptions::ArticleRequest::LimitReached->throw(
-        error => 'Patron cannot request more articles for today'
-    ) unless $self->borrower->can_request_article;
-
     $self->status(Koha::ArticleRequest::Status::Requested);
-    $self->SUPER::store();
-    $self->notify();
-    return $self;
+    return $self->store;
 }
 
 =head3 set_pending
@@ -66,9 +98,7 @@ sub set_pending {
     my ($self) = @_;
 
     $self->status(Koha::ArticleRequest::Status::Pending);
-    $self->SUPER::store();
-    $self->notify();
-    return $self;
+    return $self->store;
 }
 
 =head3 process
@@ -79,9 +109,7 @@ sub process {
     my ($self) = @_;
 
     $self->status(Koha::ArticleRequest::Status::Processing);
-    $self->store();
-    $self->notify();
-    return $self;
+    return $self->store;
 }
 
 =head3 complete
@@ -92,9 +120,7 @@ sub complete {
     my ($self) = @_;
 
     $self->status(Koha::ArticleRequest::Status::Completed);
-    $self->store();
-    $self->notify();
-    return $self;
+    return $self->store;
 }
 
 =head3 cancel
@@ -107,59 +133,7 @@ sub cancel {
     $self->status(Koha::ArticleRequest::Status::Canceled);
     $self->cancellation_reason($cancellation_reason) if $cancellation_reason;
     $self->notes($notes) if $notes;
-    $self->store();
-    $self->notify();
-    return $self;
-}
-
-=head3 notify
-
-=cut
-
-sub notify {
-    my ($self) = @_;
-
-    my $status = $self->status;
-    my $reason = $self->notes;
-    if ( !defined $reason && $self->cancellation_reason ) {
-        my $av = Koha::AuthorisedValues->search(
-            {
-                category            => 'AR_CANCELLATION',
-                authorised_value    => $self->cancellation_reason
-            }
-        )->next;
-        $reason = $av->lib_opac ? $av->lib_opac : $av->lib if $av;
-    }
-
-    require C4::Letters;
-    if (
-        my $letter = C4::Letters::GetPreparedLetter(
-            module                 => 'circulation',
-            letter_code            => "AR_$status", # AR_REQUESTED, AR_PENDING, AR_PROCESSING, AR_COMPLETED, AR_CANCELED
-            message_transport_type => 'email',
-            lang                   => $self->borrower->lang,
-            tables                 => {
-                article_requests => $self->id,
-                borrowers        => $self->borrowernumber,
-                biblio           => $self->biblionumber,
-                biblioitems      => $self->biblionumber,
-                items            => $self->itemnumber,
-                branches         => $self->branchcode,
-            },
-            substitute => {
-                reason => $reason,
-            },
-        )
-      )
-    {
-        C4::Letters::EnqueueLetter(
-            {
-                letter                 => $letter,
-                borrowernumber         => $self->borrowernumber,
-                message_transport_type => 'email',
-            }
-        ) or warn "can't enqueue letter " . $letter->{code};
-    }
+    return $self->store;
 }
 
 =head3 biblio
@@ -171,9 +145,8 @@ Returns the Koha::Biblio object for this article request
 sub biblio {
     my ($self) = @_;
 
-    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );
-
-    return $self->{_biblio};
+    my $biblio = $self->_result->biblio;
+    return Koha::Biblio->_new_from_dbic( $biblio );
 }
 
 =head3 item
@@ -185,9 +158,8 @@ Returns the Koha::Item object for this article request
 sub item {
     my ($self) = @_;
 
-    $self->{_item} ||= Koha::Items->find( $self->itemnumber() );
-
-    return $self->{_item};
+    my $item = $self->_result->item;
+    return Koha::Item->_new_from_dbic( $item );
 }
 
 =head3 borrower
@@ -199,9 +171,8 @@ Returns the Koha::Patron object for this article request
 sub borrower {
     my ($self) = @_;
 
-    $self->{_borrower} ||= Koha::Patrons->find( $self->borrowernumber() );
-
-    return $self->{_borrower};
+    my $patron = $self->_result->patron;
+    return Koha::Patron->_new_from_dbic( $patron );
 }
 
 =head3 branch
@@ -213,29 +184,63 @@ Returns the Koha::Library object for this article request
 sub branch {
     my ($self) = @_;
 
-    $self->{_branch} ||= Koha::Libraries->find( $self->branchcode() );
-
-    return $self->{_branch};
+    my $library = $self->_result->library;
+    return Koha::Library->_new_from_dbic( $library );
 }
 
-=head3 store
+=head2 Internal methods
 
-Override the default store behavior so that new opac requests
-will have notifications sent.
+=head3 notify
 
 =cut
 
-sub store {
+sub notify {
     my ($self) = @_;
-    if ( $self->in_storage ) {
-        return $self->SUPER::store;
-    } else {
-        $self->created_on( dt_from_string() );
-        return $self->request;
+
+    my $status = $self->status;
+    my $reason = $self->notes;
+    if ( !defined $reason && $self->cancellation_reason ) {
+        my $av = Koha::AuthorisedValues->search(
+            {
+                category            => 'AR_CANCELLATION',
+                authorised_value    => $self->cancellation_reason
+            }
+        )->next;
+        $reason = $av->lib_opac ? $av->lib_opac : $av->lib if $av;
+    }
+
+    require C4::Letters;
+    if (
+        my $letter = C4::Letters::GetPreparedLetter(
+            module                 => 'circulation',
+            letter_code            => "AR_$status", # AR_REQUESTED, AR_PENDING, AR_PROCESSING, AR_COMPLETED, AR_CANCELED
+            message_transport_type => 'email',
+            lang                   => $self->borrower->lang,
+            tables                 => {
+                article_requests => $self->id,
+                borrowers        => $self->borrowernumber,
+                biblio           => $self->biblionumber,
+                biblioitems      => $self->biblionumber,
+                items            => $self->itemnumber,
+                branches         => $self->branchcode,
+            },
+            substitute => {
+                reason => $reason,
+            },
+        )
+      )
+    {
+        C4::Letters::EnqueueLetter(
+            {
+                letter                 => $letter,
+                borrowernumber         => $self->borrowernumber,
+                message_transport_type => 'email',
+            }
+        ) or warn "can't enqueue letter " . $letter->{code};
     }
 }
 
-=head2 Internal methods
+
 
 =head3 _type
 
diff --git a/Koha/Schema/Result/ArticleRequest.pm b/Koha/Schema/Result/ArticleRequest.pm
index 13748ba8c8..456e53868e 100644
--- a/Koha/Schema/Result/ArticleRequest.pm
+++ b/Koha/Schema/Result/ArticleRequest.pm
@@ -293,6 +293,42 @@ __PACKAGE__->belongs_to(
 # Created by DBIx::Class::Schema::Loader v0.07049 @ 2021-08-18 10:41:01
 # DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:1q7Vlki3fRVBdww1zHLCRA
 
+__PACKAGE__->belongs_to(
+  "biblio",
+  "Koha::Schema::Result::Biblio",
+  { biblionumber => "biblionumber" },
+  { is_deferrable => 1, on_delete => "CASCADE", on_update => "CASCADE" },
+);
+
+__PACKAGE__->belongs_to(
+  "item",
+  "Koha::Schema::Result::Item",
+  { itemnumber => "itemnumber" },
+  {
+    is_deferrable => 1,
+    join_type     => "LEFT",
+    on_delete     => "SET NULL",
+    on_update     => "CASCADE",
+  },
+);
+
+__PACKAGE__->belongs_to(
+  "library",
+  "Koha::Schema::Result::Branch",
+  { branchcode => "branchcode" },
+  {
+    is_deferrable => 1,
+    join_type     => "LEFT",
+    on_delete     => "SET NULL",
+    on_update     => "CASCADE",
+  },
+);
+
+__PACKAGE__->belongs_to(
+  "patron",
+  "Koha::Schema::Result::Borrower",
+  { borrowernumber => "borrowernumber" },
+  { is_deferrable => 1, on_delete => "CASCADE", on_update => "CASCADE" },
+);
 
-# You can replace this text with custom code or comments, and it will be preserved on regeneration
 1;
-- 
2.30.2