Attachment 125285 Details for Bug 29110 – Bug 29110: Refactor Koha::ArticleRequest

[patch] Bug 29110: Refactor Koha::ArticleRequest

Bug-29110-Refactor-KohaArticleRequest.patch (text/plain), 10.03 KB, created by David Nind on 2021-09-24 16:00:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2021-09-24 16:00:46 UTC

Size: 10.03 KB

patch

obsolete

>From 7c5d91b3f0654f40c087fa8e26dc451c5a4adcb6 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
>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> 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.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 29110: 125256 | 125257 | 125284 | 125285