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.
@Joubu: Is everything correct?
(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.
===== 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.»
===== 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.
Created attachment 69824 [details] [review] Fix opac-detail being broken by a review from a deleted patron 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
Thanks Julian Maurice for the help to fix all the tricky cases and clean the code :)
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
Created attachment 69828 [details] [review] Bug 19808: Fix opac-detail breaking on a review from a deleted patron 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>
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 ;)
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.
(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).
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>
Created attachment 69854 [details] [review] Bug 19808: (follow-up) Handle deleted reviewers gracefully - opac-detail Make the tests easier to read
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
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 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.
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.
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)
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"
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.
(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
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Pushed to master for 18.05, thanks to everybody involved!
All but last patch (translation) have been pushed to stable for 17.11.01
Bug 17829 not in 17.05.x