Bug 29110

Summary: Refactor Koha::ArticleRequest->store
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED INVALID QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 27949    
Bug Blocks:    
Attachments: Bug 29110: Unit tests
Bug 29110: Refactor Koha::ArticleRequest
Bug 29110: Unit tests
Bug 29110: Refactor Koha::ArticleRequest

Description Tomás Cohen Arazi 2021-09-24 14:48:05 UTC
We should revisit the file altogether, and add more checks to ->store. This way the code will be easier to follow are more similar to what we do everywhere else.
Comment 1 Tomás Cohen Arazi 2021-09-24 15:08:02 UTC
Created attachment 125256 [details] [review]
Bug 29110: Unit tests
Comment 2 Tomás Cohen Arazi 2021-09-24 15:08:06 UTC
Created attachment 125257 [details] [review]
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
Comment 3 David Nind 2021-09-24 16:00:37 UTC
Created attachment 125284 [details] [review]
Bug 29110: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-09-24 16:00:46 UTC
Created attachment 125285 [details] [review]
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>
Comment 5 David Nind 2021-09-24 16:15:57 UTC
I singed this off, but then thought I should check that the article requests system is still working.

When adding a request or going to "Your account" (all the other user account ages seem fine) I get:

Template process failed: undef error - DBIC result _type  isn't of the _type Item at /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line 751.
 at /kohadevbox/koha/C4/Templates.pm line 121

    in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 121

      118: 
      119:     my $data;
      120:     binmode( STDOUT, ":encoding(UTF-8)" );
      121:     $template->process( $self->filename, $vars, \$data )
      122:       || die "Template process failed: ", $template->error();
      123:     return $data;
      124: }

    Show function arguments
    in (eval) at /kohadevbox/koha/opac/opac-user.pl line 421

      418:     );
      419: }
      420: 
      421: output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 };
Comment 6 Tomás Cohen Arazi 2021-09-24 17:23:49 UTC
(In reply to David Nind from comment #5)
> I singed this off, but then thought I should check that the article requests
> system is still working.
> 
> When adding a request or going to "Your account" (all the other user account
> ages seem fine) I get:
> 
> Template process failed: undef error - DBIC result _type  isn't of the _type
> Item at
> /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line
> 751.
>  at /kohadevbox/koha/C4/Templates.pm line 121
> 
>     in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 121
> 
>       118: 
>       119:     my $data;
>       120:     binmode( STDOUT, ":encoding(UTF-8)" );
>       121:     $template->process( $self->filename, $vars, \$data )
>       122:       || die "Template process failed: ", $template->error();
>       123:     return $data;
>       124: }
> 
>     Show function arguments
>     in (eval) at /kohadevbox/koha/opac/opac-user.pl line 421
> 
>       418:     );
>       419: }
>       420: 
>       421: output_html_with_http_headers $query, $cookie, $template->output,
> undef, { force_no_caching => 1 };

Are you sure you restarted all the things? I have just applied the whole tree and reset_all, and things keep working as they used to.
Comment 7 Marcel de Rooy 2021-09-27 08:34:19 UTC
+    $deleted_patron->delete; # Haha
Comment 8 Marcel de Rooy 2021-09-27 08:37:10 UTC
 Koha/ArticleRequest.pm               | 187 ++++++++++++++-------------
 Koha/Schema/Result/ArticleRequest.pm |  38 +++++-

Strictly forbidden. Even for Tomas.
Comment 9 Marcel de Rooy 2021-09-27 08:39:20 UTC
+        # this is to avoid calling ->discard_changes to retrieve
+        # the default value from the DB
+        $self->status( Koha::ArticleRequest::Status::Requested )
+            unless $self->status;

Hmm. But if you didnt provide a status, we should be able to rely on the default value from the DB engine, isnt it?
Comment 10 Marcel de Rooy 2021-09-27 08:41:25 UTC
+    $self->SUPER::store;
+    $self->notify;

Not sure if this is an improvement. When you store something, you call notify? This action begs for another code location imo.
Comment 11 Marcel de Rooy 2021-09-27 08:51:43 UTC
Note that I also saw the problem David mentioned:

Error trace

Template process failed: undef error - DBIC result _type  isn't of the _type Item at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line 751.
 at /usr/share/koha/C4/Templates.pm line 121

Yes, I restarted Plack.
Comment 12 Tomás Cohen Arazi 2021-10-06 17:24:19 UTC
(In reply to Marcel de Rooy from comment #7)
> +    $deleted_patron->delete; # Haha

It makes sense :-D I want a patron I'm sure doesn't exist on the DB :-D
Comment 13 Tomás Cohen Arazi 2021-10-06 17:24:58 UTC
(In reply to Marcel de Rooy from comment #8)
>  Koha/Schema/Result/ArticleRequest.pm |  38 +++++-

That's weird. Probably didn't notice on git add.

> Strictly forbidden. Even for Tomas.

Specially for any QA team member!
Comment 14 Tomás Cohen Arazi 2021-10-06 17:25:28 UTC
(In reply to Marcel de Rooy from comment #9)
> +        # this is to avoid calling ->discard_changes to retrieve
> +        # the default value from the DB
> +        $self->status( Koha::ArticleRequest::Status::Requested )
> +            unless $self->status;
> 
> Hmm. But if you didnt provide a status, we should be able to rely on the
> default value from the DB engine, isnt it?

True
Comment 15 Tomás Cohen Arazi 2021-10-06 17:36:47 UTC
I don't think we need this for now. The only valuable addition is the schema changes, adding relationships with better names, but it falls short as it is not renaming the methods to access those.

My original idea was to rename the methods to allow embedding things in an API route for article requests e.g.

GET /article_requests
x-koha-embed: patron, library

so we can use it to render datatables.
Comment 16 Tomás Cohen Arazi 2021-10-06 17:37:27 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> I don't think we need this for now. The only valuable addition is the schema
> changes, adding relationships with better names, but it falls short as it is
> not renaming the methods to access those.
> 
> My original idea was to rename the methods to allow embedding things in an
> API route for article requests e.g.
> 
> GET /article_requests
> x-koha-embed: patron, library
> 
> so we can use it to render datatables.

That said, I'm closing this bug, and opening a new one, once I can start coding such API route.