Bug 18477 - AR_PENDING notice does not populate values from article_requests table
Summary: AR_PENDING notice does not populate values from article_requests table
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-21 16:27 UTC by Barton Chittenden
Modified: 2018-06-04 20:15 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18477 - AR_PENDING notice does not populate values from article_requests table (1.26 KB, patch)
2017-05-08 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18477 - AR_PENDING notice does not populate values from article_requests table (1.32 KB, patch)
2017-05-08 22:41 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 18477: Use ->store to update the timestamp (690 bytes, patch)
2018-01-02 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18477: Add tests (2.74 KB, patch)
2018-01-02 17:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18477: Add tests (2.79 KB, patch)
2018-01-17 18:17 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18477 - AR_PENDING notice does not populate values from article_requests table (1.37 KB, patch)
2018-01-17 18:17 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2017-04-21 16:27:55 UTC
The default AR_PENDING letter does not print populate <<article_requests.*>> fields.

To replicate:

1) Enable article requests ( syspref: ArticleRequests => Enable, Circ and fine rules ALL/ALL (or a given category/itemtype): Article requests => Yes )
2) Search for a title, click in to catalog detail, click 'Request Article' button.
3) Select Patron
4) Fill out request information
5) Click 'Pplace request'
6) Go to notices in patron record, inspect 'Article Request Received' notice. You should see the request information from step 4, but it doesn't display.

Examples below:

AR_PENDING

===================================
<<borrowers.firstname>> <<borrowers.surname>> (<<borrowers.cardnumber>>)

We have received your request for an article from <<biblio.title>>.

Article requested:
Title: <<article_requests.title>>
Author: <<article_requests.author>>
Volume: <<article_requests.volume>>
Issue: <<article_requests.issue>>
Date: <<article_requests.date>>
Pages: <<article_requests.pages>>
Chapters: <<article_requests.chapters>>
Notes: <<article_requests.patron_notes>>
===================================


Displays as

===================================
Barton Chittenden (barton)

We have received your request for an article from Bws test record 2 ().

Article requested:
Title: 
Author: 
Volume: 
Issue: 
Date: 
Pages: 
Chapters: 
Notes: 


Thank you!
===================================


The 'processing' and 'completed' notices work work, here's the output from 'Article Request Completed'

===================================
Barton Chittenden (barton)

We are have completed your request for an article from Bws test record 2 ().

Article requested:
Title: TEST TEST TEST
Author: asdfsadf
Volume: foo
Issue: bar
Date: baz
Pages: 123234
Chapters: 9
Notes: Raaaaaagh!

You may pick your article up at Midway.

Thank you!
===================================
Comment 1 Kyle M Hall 2017-05-08 16:49:26 UTC
Created attachment 63251 [details] [review]
Bug 18477 - AR_PENDING notice does not populate values from article_requests table

The default AR_PENDING letter does not print populate <<article_requests.*>> fields.

Test Plan:
1) Enable article requests ( syspref: ArticleRequests => Enable, Circ and fine rules ALL/ALL (or a given category/itemtype): Article requests => Yes )
2) Search for a title, click in to catalog detail, click 'Request Article' button.
3) Select Patron
4) Fill out request information
5) Click 'Place request'
6) Go to notices in patron record, inspect 'Article Request Received' notice. You should see the request information from step 4, but it doesn't display.
7) Apply this patch
8) Repeat steps 2-6, not the fields now display!
Comment 2 Eric Phetteplace 2017-05-08 22:41:46 UTC
Created attachment 63261 [details] [review]
Bug 18477 - AR_PENDING notice does not populate values from article_requests table

The default AR_PENDING letter does not print populate <<article_requests.*>> fields.

Test Plan:
1) Enable article requests ( syspref: ArticleRequests => Enable, Circ and fine rules ALL/ALL (or a given category/itemtype): Article requests => Yes )
2) Search for a title, click in to catalog detail, click 'Request Article' button.
3) Select Patron
4) Fill out request information
5) Click 'Place request'
6) Go to notices in patron record, inspect 'Article Request Received' notice. You should see the request information from step 4, but it doesn't display.
7) Apply this patch
8) Repeat steps 2-6, not the fields now display!

Signed-off-by: Eric Phetteplace <phette23@gmail.com>
Comment 3 Jonathan Druart 2017-05-11 15:50:30 UTC
Please provide tests!
Comment 4 Katrin Fischer 2017-10-14 12:43:07 UTC
This is critical as without the patch you can't tell what has been ordered.
Can we please get the tests requested?
Comment 5 Jonathan Druart 2017-10-16 12:18:33 UTC
Upping severity
Comment 6 Jonathan Druart 2017-10-16 13:23:43 UTC
(In reply to Jonathan Druart from comment #3)
> Please provide tests!

Reading the code the change is obvious (the same call exists for other methods), but why do we call $self->SUPER::store instead of $self->store?

Back to NQA
Comment 7 Marcel de Rooy 2017-10-19 11:07:06 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Jonathan Druart from comment #3)
> > Please provide tests!
> 
> Reading the code the change is obvious (the same call exists for other
> methods), but why do we call $self->SUPER::store instead of $self->store?
> 
> Back to NQA

Same question for me. Inconsistent.
Comment 8 Katrin Fischer 2018-01-02 07:42:07 UTC
This is still quite a problem for users of article requests - can we agree on a fix?
Comment 9 Jonathan Druart 2018-01-02 16:08:16 UTC
Created attachment 70228 [details] [review]
Bug 18477: Use ->store to update the timestamp
Comment 10 Jonathan Druart 2018-01-02 16:09:27 UTC
There is a big lack of test and add a test here will take me too much time. The fix sounds trivial and the ->store method is obviously missing.
I am willing to push these patches as it, if QA is ok with that.
Comment 11 Jon Knight 2018-01-02 16:28:42 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Jonathan Druart from comment #3)
> > Please provide tests!
> 
> Reading the code the change is obvious (the same call exists for other
> methods), but why do we call $self->SUPER::store instead of $self->store?
> 
> Back to NQA

I just had a look at this and it would appear to call the parent's store method because if it calls its own there's a loop: the $self->store() method in Koha/ArticleRequest.pm can call its $self->open() method.  I tested it by doing the obvious: replacing $self-SUPER::store with $self->store and sure enough its a memory hogging loop that crashed by dev VM's web server.
Comment 12 Jonathan Druart 2018-01-02 17:37:35 UTC
Created attachment 70231 [details] [review]
Bug 18477: Add tests
Comment 13 Jonathan Druart 2018-01-02 17:38:12 UTC
Indeed Jon, so we need tests :) Thanks for the catch.
Comment 14 M. Tompsett 2018-01-17 18:17:06 UTC
Created attachment 70633 [details] [review]
Bug 18477: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 15 M. Tompsett 2018-01-17 18:17:09 UTC
Created attachment 70634 [details] [review]
Bug 18477 - AR_PENDING notice does not populate values from article_requests table

The default AR_PENDING letter does not print populate <<article_requests.*>> fields.

Test Plan:
1) Enable article requests ( syspref: ArticleRequests => Enable, Circ and fine rules ALL/ALL (or a given category/itemtype): Article requests => Yes )
2) Search for a title, click in to catalog detail, click 'Request Article' button.
3) Select Patron
4) Fill out request information
5) Click 'Place request'
6) Go to notices in patron record, inspect 'Article Request Received' notice. You should see the request information from step 4, but it doesn't display.
7) Apply this patch
8) Repeat steps 2-6, not the fields now display!

Signed-off-by: Eric Phetteplace <phette23@gmail.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 M. Tompsett 2018-01-17 18:18:14 UTC
Tests added trigger code change. Apply tests only. Test fail. Apply both patches. Test passes. This is expected.
Comment 17 Jonathan Druart 2018-01-23 19:07:35 UTC
Critical, in the queue for too long already. Skipping QA.
Comment 18 Katrin Fischer 2018-01-29 07:32:51 UTC
Pushed?
Comment 19 Kyle M Hall 2018-01-30 15:10:51 UTC
(In reply to Katrin Fischer from comment #18)
> Pushed?

Indeed! Commit id 85b0424eb704d416950510c30b41a10ce56cbc3b in master.
Comment 20 Nick Clemens 2018-02-02 17:02:39 UTC
Awesome work all! Pushed to stable for 17.11.03
Comment 21 Fridolin SOMERS 2018-02-08 09:50:43 UTC
Pushed to 17.05.x for v17.05.09