Bug 29110

Created attachment 125256 [details] [review]
Bug 29110: Unit tests

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

Created attachment 125284 [details] [review]
Bug 29110: Unit tests

Signed-off-by: David Nind <david@davidnind.com>

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>

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 };

(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.

+    $deleted_patron->delete; # Haha

 Koha/ArticleRequest.pm               | 187 ++++++++++++++-------------
 Koha/Schema/Result/ArticleRequest.pm |  38 +++++-

Strictly forbidden. Even for Tomas.

+        # 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?

+    $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.

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.

(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

(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!

(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

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.

(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.