Bug 19808 - Reviews from deleted patrons make few scripts to explode
Summary: Reviews from deleted patrons make few scripts to explode
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
Depends on: 15839 17829
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-13 20:30 UTC by Victor Grousset/tuxayo
Modified: 2017-12-22 10:55 UTC (History)
6 users (show)

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


Attachments
Fix opac-detail being broken by a review from a deleted patron (6.94 KB, patch)
2017-12-15 14:30 UTC, Victor Grousset/tuxayo
Details | Splinter Review
Bug 19808: Fix opac-detail breaking on a review from a deleted patron (6.95 KB, patch)
2017-12-15 18:32 UTC, M. Tompsett
Details | Splinter Review
Bug 19808: Handle deleted reviewers gracefully - opac-detail (6.95 KB, patch)
2017-12-18 16:36 UTC, Jonathan Druart
Details | Splinter Review
Bug 19808: (follow-up) Handle deleted reviewers gracefully - opac-detail (1.43 KB, patch)
2017-12-18 16:36 UTC, Jonathan Druart
Details | Splinter Review
Bug 19808: Handle deleted reviewers gracefully - opac-showreview (2.50 KB, patch)
2017-12-18 16:36 UTC, Jonathan Druart
Details | Splinter Review
Bug 19808: Handle deleted reviewers gracefully - reviews/reviewswaiting.pl (1.65 KB, patch)
2017-12-18 16:36 UTC, Jonathan Druart
Details | Splinter Review
Bug 19808: Do not display 'Added' alone (2.03 KB, patch)
2017-12-19 19:29 UTC, Jonathan Druart
Details | Splinter Review
[SIGNED-OFF] Bug 19808: Handle deleted reviewers gracefully - opac-detail (7.00 KB, patch)
2017-12-19 22:38 UTC, Jon Knight
Details | Splinter Review
[SIGNED-OFF] Bug 19808: (follow-up) Handle deleted reviewers gracefully - opac-detail (1.48 KB, patch)
2017-12-19 22:38 UTC, Jon Knight
Details | Splinter Review
[SIGNED-OFF] Bug 19808: Handle deleted reviewers gracefully - opac-showreview (2.56 KB, patch)
2017-12-19 22:38 UTC, Jon Knight
Details | Splinter Review
[SIGNED-OFF] Bug 19808: Handle deleted reviewers gracefully - reviews/reviewswaiting.pl (1.71 KB, patch)
2017-12-19 22:38 UTC, Jon Knight
Details | Splinter Review
[SIGNED-OFF] Bug 19808: Do not display 'Added' alone (2.09 KB, patch)
2017-12-19 22:38 UTC, Jon Knight
Details | Splinter Review
Bug 19808: Handle deleted reviewers gracefully - opac-detail (7.05 KB, patch)
2017-12-21 10:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19808: (follow-up) Handle deleted reviewers gracefully - opac-detail (1.53 KB, patch)
2017-12-21 10:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19808: Handle deleted reviewers gracefully - opac-showreview (2.60 KB, patch)
2017-12-21 10:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19808: Handle deleted reviewers gracefully - reviews/reviewswaiting.pl (1.75 KB, patch)
2017-12-21 10:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19808: Do not display 'Added' alone (2.13 KB, patch)
2017-12-21 10:46 UTC, Kyle M Hall
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Grousset/tuxayo 2017-12-13 20:30:54 UTC
Found by Joubu.

===== Cause =====
plack-error.log:
> Can't call method "title" on an undefined value at /home/koha/src/opac/opac-detail.pl line 847.

code:
> # setting some borrower info into this hash
> $review->{title}     = $patron->title;

db:
> CONSTRAINT `reviews_ibfk_1`
> FOREIGN KEY (`borrowernumber`)
> REFERENCES `borrowers` (`borrowernumber`)
> ON DELETE SET NULL ON UPDATE CASCADE,

The reviews are kept even if the user was deleted

BZ 15839 is suspected to have introduced this bug.

===== Fix =====
The first step is to fix the 500 error.
And then study the relevance of "on delete set null".
And take care of the existing data.
Comment 1 Victor Grousset/tuxayo 2017-12-13 20:32:35 UTC
@Joubu: Is everything correct?
Comment 2 Jonathan Druart 2017-12-13 21:07:25 UTC
(In reply to Victor Grousset/tuxayo from comment #1)
> @Joubu: Is everything correct?

Yep, we will only take care of the existing data if we decide to remove the reviews from deleted patrons.
Comment 3 Victor Grousset/tuxayo 2017-12-14 15:05:21 UTC
===== Going further =====

Another bug caused by the root issue:
When a patron leaves a comment/review on a record
And this patron is deleted
Then the "Comments pending approval" page
  (staff:/cgi-bin/koha/reviews/reviewswaiting.pl)
  throws a 500 error
  «Can't call method "surname" on an undefined value at /home/koha/src/reviews/reviewswaiting.pl line 76.»
Comment 4 Victor Grousset/tuxayo 2017-12-14 15:29:28 UTC
===== Going further =====

(In reply to Victor Grousset/tuxayo from comment #3)
> ===== Going further =====
> 
> Another bug caused by the root issue:
> When a patron leaves a comment/review on a record
> And this patron is deleted
> Then the "Comments pending approval" page
>   (staff:/cgi-bin/koha/reviews/reviewswaiting.pl)
>   throws a 500 error
>   «Can't call method "surname" on an undefined value at
> /home/koha/src/reviews/reviewswaiting.pl line 76.»


Just trying to go the approved comments (staff:/cgi-bin/koha/reviews/reviewswaiting.pl?status=1) also triggers the same error. This case is more likely to happen than the above.
Comment 5 Victor Grousset/tuxayo 2017-12-15 14:30:28 UTC Comment hidden (obsolete)
Comment 6 Victor Grousset/tuxayo 2017-12-15 14:37:00 UTC
Thanks Julian Maurice for the help to fix all the tricky cases and clean the code :)
Comment 7 M. Tompsett 2017-12-15 17:43:25 UTC
Can't call method "title" on an undefined value at /home/koha/src/opac/opac-detail.pl line 847.

Successfully duplicated on old-school git install (not kohadevbox).
(In reply to Victor Grousset/tuxayo from comment #3)
> ===== Going further =====
> 
> Another bug caused by the root issue:
> When a patron leaves a comment/review on a record
> And this patron is deleted
> Then the "Comments pending approval" page
>   (staff:/cgi-bin/koha/reviews/reviewswaiting.pl)
>   throws a 500 error
>   «Can't call method "surname" on an undefined value at
> /home/koha/src/reviews/reviewswaiting.pl line 76.»

Did you bug report this? Because your patch only fixes the 847 issue in opac-detail.pl
Comment 8 M. Tompsett 2017-12-15 18:32:10 UTC Comment hidden (obsolete)
Comment 9 Josef Moravec 2017-12-17 08:12:16 UTC
Are you going to address the issue on reviews/reviewswaiting.pl too? If not please make a bug report for it. Thanks

Marking as Failed QA = waiting for answer here ;)
Comment 10 M. Tompsett 2017-12-18 01:15:30 UTC
As I alluded to in comment #7 based on comment #4's observation.

(In reply to Josef Moravec from comment #9)
> Are you going to address the issue on reviews/reviewswaiting.pl too?

If so, awaiting patch.


> If not
> please make a bug report for it.

-- and add it to the See Also.


> Thanks

-- Ditto. :)


> Marking as Failed QA = waiting for answer here ;)

I'm torn. The title doesn't suggest it will be fixed here.
However, there is no link to a bug elsewhere. I'll leave the status as is.
Comment 11 Victor Grousset/tuxayo 2017-12-18 15:38:07 UTC
(In reply to M. Tompsett from comment #7)
> Did you bug report this? Because your patch only fixes the 847 issue in
> opac-detail.pl

(In reply to Josef Moravec from comment #9)
> Are you going to address the issue on reviews/reviewswaiting.pl too? If not
> please make a bug report for it. Thanks

Thanks for following up on that part.

The idea was to write somewhere these findings to not forget to do something about that.

I planned to open a separate bug for this as it's wasn't the same priority. So I only have the time handle the first part.

After a discussion with Joubu, it's better to fix the rest in the same bug. So here you go (changing status and assignee).
Comment 12 Jonathan Druart 2017-12-18 16:36:39 UTC
Created attachment 69853 [details] [review]
Bug 19808: Handle deleted reviewers gracefully - opac-detail

And other display issues when the patron was NULL.
Which allows to keep the review even if it has no patron.
Because it might be useful.

For example when disconnected, the borrowernumber is null. So the
comments from deleted patrons were displayed as if the disconnected
user wrote them. So it had the edit button...

And fix borrowernumber not being passed to the template when
OpacStarRatings was false.

Test plan
1. Log in as a patron
2. Leave a comment/review on a record
3. Librarian: approve this comment
4. Delete the borrower
5. See the record (opac:/cgi-bin/koha/opac-detail.pl?biblionumber=RELEVANT_BIB_NUMBER)
6. Then you should see an error
7. Apply this patch
8. Refresh the page
9. Then you should see the record page with the comment

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 13 Jonathan Druart 2017-12-18 16:36:44 UTC
Created attachment 69854 [details] [review]
Bug 19808: (follow-up) Handle deleted reviewers gracefully - opac-detail

Make the tests easier to read
Comment 14 Jonathan Druart 2017-12-18 16:36:52 UTC
Created attachment 69855 [details] [review]
Bug 19808: Handle deleted reviewers gracefully - opac-showreview

Test plan:
- Create reviews
- Delete some reviewers
- Enable OpacShowRecentComments
- Go to opac/opac-showreviews.pl
- Go to opac-showreviews.pl?format=rss
Comment 15 Jonathan Druart 2017-12-18 16:36:57 UTC
Created attachment 69856 [details] [review]
Bug 19808: Handle deleted reviewers gracefully - reviews/reviewswaiting.pl

Test plan:
- Create reviews
- Delete some reviewers
- Go to Home › Tools › Comments › Comments awaiting moderation
Comment 16 M. Tompsett 2017-12-18 16:45:41 UTC
Comment on attachment 69856 [details] [review]
Bug 19808: Handle deleted reviewers gracefully - reviews/reviewswaiting.pl

Review of attachment 69856 [details] [review]:
-----------------------------------------------------------------

::: reviews/reviewswaiting.pl
@@ +67,4 @@
>      }
>  )->unblessed;
>  
> +for my $review ( @$reviews ) {

Love the explicit $review instead of $_ code.

@@ +72,1 @@
>      # setting some borrower info into this hash

Think the comment is misplaced.
Comment 17 Victor Grousset/tuxayo 2017-12-18 16:56:24 UTC
BTW any idea why it doesn't seem to affect 16.11?
A least for the explosion part. It still shows "Your comment" + the edit button when the patron is deleted.
Comment 18 Jonathan Druart 2017-12-18 17:10:02 UTC
Koha::Reviews is used since bug 15839 (in 16.11.x), but the patron's info are still retrieved from GetMember.
Since bug 17829 (17.11) they are using Koha::Patrons (which means we cannot call a method on an undefined object)
Comment 19 Jonathan Druart 2017-12-19 19:27:32 UTC
16:15 < oleonard> greenjimll: I was just testing that too but got interrupted. Looks okay to me except that in the OPAC the comment labels don't look right when there is no patron
16:15 < oleonard> Instead of "Added 2016-12-19 by John Smith" it says "Added"
Comment 20 Jonathan Druart 2017-12-19 19:29:17 UTC
Created attachment 69926 [details] [review]
Bug 19808: Do not display 'Added' alone

It seems that reviews.datereviewed ends with NULL in DB.
It is caused by another issue that will be fixed on another bug report.
Comment 21 Jonathan Druart 2017-12-19 19:53:35 UTC
(In reply to Jonathan Druart from comment #20)
> Created attachment 69926 [details] [review] [review]
> Bug 19808: Do not display 'Added' alone
> 
> It seems that reviews.datereviewed ends with NULL in DB.
> It is caused by another issue that will be fixed on another bug report.

See bug 15839
Comment 22 Jon Knight 2017-12-19 22:38:33 UTC
Created attachment 69934 [details] [review]
[SIGNED-OFF] Bug 19808: Handle deleted reviewers gracefully - opac-detail

And other display issues when the patron was NULL.
Which allows to keep the review even if it has no patron.
Because it might be useful.

For example when disconnected, the borrowernumber is null. So the
comments from deleted patrons were displayed as if the disconnected
user wrote them. So it had the edit button...

And fix borrowernumber not being passed to the template when
OpacStarRatings was false.

Test plan
1. Log in as a patron
2. Leave a comment/review on a record
3. Librarian: approve this comment
4. Delete the borrower
5. See the record (opac:/cgi-bin/koha/opac-detail.pl?biblionumber=RELEVANT_BIB_NUMBER)
6. Then you should see an error
7. Apply this patch
8. Refresh the page
9. Then you should see the record page with the comment

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 23 Jon Knight 2017-12-19 22:38:36 UTC
Created attachment 69935 [details] [review]
[SIGNED-OFF] Bug 19808: (follow-up) Handle deleted reviewers gracefully - opac-detail

Make the tests easier to read

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 24 Jon Knight 2017-12-19 22:38:40 UTC
Created attachment 69936 [details] [review]
[SIGNED-OFF] Bug 19808: Handle deleted reviewers gracefully - opac-showreview

Test plan:
- Create reviews
- Delete some reviewers
- Enable OpacShowRecentComments
- Go to opac/opac-showreviews.pl
- Go to opac-showreviews.pl?format=rss

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 25 Jon Knight 2017-12-19 22:38:43 UTC
Created attachment 69937 [details] [review]
[SIGNED-OFF] Bug 19808: Handle deleted reviewers gracefully - reviews/reviewswaiting.pl

Test plan:
- Create reviews
- Delete some reviewers
- Go to Home › Tools › Comments › Comments awaiting moderation

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 26 Jon Knight 2017-12-19 22:38:46 UTC
Created attachment 69938 [details] [review]
[SIGNED-OFF] Bug 19808: Do not display 'Added' alone

It seems that reviews.datereviewed ends with NULL in DB.
It is caused by another issue that will be fixed on another bug report.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 27 Kyle M Hall 2017-12-21 10:46:06 UTC
Created attachment 69989 [details] [review]
Bug 19808: Handle deleted reviewers gracefully - opac-detail

And other display issues when the patron was NULL.
Which allows to keep the review even if it has no patron.
Because it might be useful.

For example when disconnected, the borrowernumber is null. So the
comments from deleted patrons were displayed as if the disconnected
user wrote them. So it had the edit button...

And fix borrowernumber not being passed to the template when
OpacStarRatings was false.

Test plan
1. Log in as a patron
2. Leave a comment/review on a record
3. Librarian: approve this comment
4. Delete the borrower
5. See the record (opac:/cgi-bin/koha/opac-detail.pl?biblionumber=RELEVANT_BIB_NUMBER)
6. Then you should see an error
7. Apply this patch
8. Refresh the page
9. Then you should see the record page with the comment

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2017-12-21 10:46:14 UTC
Created attachment 69990 [details] [review]
Bug 19808: (follow-up) Handle deleted reviewers gracefully - opac-detail

Make the tests easier to read

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2017-12-21 10:46:16 UTC
Created attachment 69991 [details] [review]
Bug 19808: Handle deleted reviewers gracefully - opac-showreview

Test plan:
- Create reviews
- Delete some reviewers
- Enable OpacShowRecentComments
- Go to opac/opac-showreviews.pl
- Go to opac-showreviews.pl?format=rss

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2017-12-21 10:46:19 UTC
Created attachment 69992 [details] [review]
Bug 19808: Handle deleted reviewers gracefully - reviews/reviewswaiting.pl

Test plan:
- Create reviews
- Delete some reviewers
- Go to Home › Tools › Comments › Comments awaiting moderation

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2017-12-21 10:46:22 UTC
Created attachment 69993 [details] [review]
Bug 19808: Do not display 'Added' alone

It seems that reviews.datereviewed ends with NULL in DB.
It is caused by another issue that will be fixed on another bug report.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Jonathan Druart 2017-12-21 14:30:16 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 33 Nick Clemens 2017-12-22 10:55:11 UTC
All but last patch (translation) have been pushed to stable for 17.11.01