Problem/Motivation

Ran into this when installing Comment Notify on an existing site and reproduced it on a fresh D8 install. I installed Comment Notify and allowed users to choose whether to subscribe to "All Comments" or "Replies to my comment." I then made a test comment, choosing "Replies to my comment" and replied to it via a different user account. No email was sent.

Proposed resolution

The issue seems to be that the thread is returned without the trailing '/', i.e. "05" instead of "05/". Using rtrim to remove the trailing slash only if present, as in Comment::preSave(), fixed the issue for me.

CommentFileSizeAuthor
#6 2879914-6.patch14.56 KBgnuget
#6 2879914-6-test-only.patch13.59 KBgnuget
#2 not_recognizing_replies-2879914-2.patch993 byteskmonahan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kmonahan created an issue. See original summary.

kmonahan’s picture

kmonahan CreditAttribution: kmonahan commented
FileSize
not_recognizing_replies-2879914-2.patch993 bytes

Patch to trim the $alert->getThread() with rtrim().

kmonahan’s picture

kmonahan CreditAttribution: kmonahan commented
Status: Active » Needs review
toamit’s picture

toamit CreditAttribution: toamit commented

Tested, this seems to work. thanks for sharing the patch.

wturrell’s picture

wturrell CreditAttribution: wturrell as a volunteer commented

Patch also working for me in 8.6.1.

gnuget’s picture

gnuget
Primary language Spanish
CreditAttribution: gnuget as a volunteer and at Agaric commented
FileSize
2879914-6-test-only.patch13.59 KB
2879914-6.patch14.56 KB

I wrote a test for this (actually I refactored a bit the tests).

Patch attached.

The last submitted patch, 6: 2879914-6-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gnuget’s picture

gnuget
Primary language Spanish
CreditAttribution: gnuget as a volunteer and at Agaric commented
Priority: Normal » Critical

And I just pump this to critical, the notification is not working without this patch.

  • greggles committed 707b964 on 8.x-1.x authored by gnuget 
    Issue #2879914 by gnuget, kmonahan: Notifications not sent when user...
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at Morris Animal Foundation commented
Status: Needs review » Fixed

Thanks for the work on this and especially for the tests.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.