Summary: | AR_PENDING notice does not populate values from article_requests table | ||
---|---|---|---|
Product: | Koha | Reporter: | Barton Chittenden <barton> |
Component: | Notices | Assignee: | Kyle M Hall (khall) <kyle> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | critical | ||
Priority: | P5 - low | CC: | ephetteplace, fridolin.somers, J.P.Knight, jonathan.druart, kyle, lmoran, m.de.rooy, mtompset, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Bug 18477 - AR_PENDING notice does not populate values from article_requests table
Bug 18477 - AR_PENDING notice does not populate values from article_requests table Bug 18477: Use ->store to update the timestamp Bug 18477: Add tests Bug 18477: Add tests Bug 18477 - AR_PENDING notice does not populate values from article_requests table |
Description
Barton Chittenden
2017-04-21 16:27:55 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! 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> Please provide tests! This is critical as without the patch you can't tell what has been ordered. Can we please get the tests requested? Upping severity (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 (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. This is still quite a problem for users of article requests - can we agree on a fix? Created attachment 70228 [details] [review] Bug 18477: Use ->store to update the timestamp 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. (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. Created attachment 70231 [details] [review] Bug 18477: Add tests Indeed Jon, so we need tests :) Thanks for the catch. Created attachment 70633 [details] [review] Bug 18477: Add tests Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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> Tests added trigger code change. Apply tests only. Test fail. Apply both patches. Test passes. This is expected. Critical, in the queue for too long already. Skipping QA. Pushed? (In reply to Katrin Fischer from comment #18) > Pushed? Indeed! Commit id 85b0424eb704d416950510c30b41a10ce56cbc3b in master. Awesome work all! Pushed to stable for 17.11.03 Pushed to 17.05.x for v17.05.09 |