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