Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 9 years ago

#6057 closed enhancement (fixed)

Comment notification NOT notified

Reported by: sebacar's profile sebacar Owned by: imath's profile imath
Milestone: 2.6 Priority: normal
Severity: normal Version: 2.0.3
Component: Toolbar & Notifications Keywords: has-patch 2nd-opinion
Cc: scott.m.seitz@…, tafadzwamakura@…

Description

This is the scenario. Fresh Wordpress Installation with only BuddyPress as installed and active plugin and Thirtheen as theme.

We have 1 administrator (A) and 2 subscribers (B and C).

B posts a status update. C Reply to it with a comment.

Email notification send correctly. Notification inside the notification page (and Top Admin Bar) is not displayed. Both in Read and Unread. It just don't work.

Don't have the time to inquire inside the code but I'm quite sure the problem is not a theme/plugin incompatibility.

Attachments (4)

6057.01.patch (30.1 KB) - added by imath 10 years ago.
6057.02.patch (34.6 KB) - added by imath 9 years ago.
6057.03.patch (19.5 KB) - added by imath 9 years ago.
6057.04.patch (19.5 KB) - added by imath 9 years ago.

Download all attachments as: .zip

Change History (39)

#1 @sebacar
10 years ago

  • Priority changed from normal to high

#2 @imath
10 years ago

  • Keywords dev-feedback added

Hi sebacar,

Thanks for your feedback. I confirm that no screen notifications are recorded for any user as the only activity action that is supported so far by the notications component is the "at_mention" one.

The function bp_activity_new_comment_notification() is only sending email notifications. This means, we need to create the code for :

  • formatting comment notifications
  • creating screen notifications
  • marking them as read / deleting them ..

I don't know if it's something we're doing on purpose, as i can imagine recording such notifications can potentially add many, many lines in the notifications table, or if it's something that is missing.

#3 @imath
10 years ago

After a quick search in trac, found relative tickets :

  • #1398
  • #5430
  • #5395 (boonebgorges has been working on a patch on this ticket)

#5395 seems to only focus on email notifications, so i'm not sure about what needs to be done about screen notifications.

#4 @boonebgorges
10 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Priority changed from high to normal
  • Severity changed from major to normal
  • Type changed from defect (bug) to enhancement

I don't know if it's something we're doing on purpose, as i can imagine recording such notifications can potentially add many, many lines in the notifications table, or if it's something that is missing.

I don't think there's a particular reason; it was just never built. Let's see what a patch would look like.

#5 @imath
10 years ago

I've explored it more deeply. Building the notifications process is not a big deal, but the problem is : what should be the link of the notification in the WP Admin Bar if more than one comment/reply are added to more than one activity.

eg: __( 'You have 4 new replies to one of your updates', 'buddypress' )

We cannot use the activity permalink as, more than one activity can be involved and we currently do not provide a member's activity subnav to list all replies that were added to the displayed member updates.

If you take the @-mention notifications for instance, in case the user received one or more mentions, the link is not the activity permalink but the member's mentions subnav

I don't think multiscope activity queries can be helpful. As an example of suggestion i've build this plugin to create a new "replies" member subnav and the notifications. But:
a) the query i'm using to get the replies would surely need some improvements.
b) i'm not sure we want to add a new member's subnav to list all replies

I think before building the patch, we need to choose the best way to handle the case when $total_items > 1

#6 @dtc7240
10 years ago

  • Cc scott.m.seitz@… added

I see this issue has gone kind of cold because no one has offered input on how to handle the case of a single notification alluding to multiple replies, and I'd like to weigh in. I currently have great need of reply notifications entering the wp_bp_notifications table (reference ticket #6375 that I just created for an explanation of why), and I see that many others desire it strongly as well.

As @imath says, the solution is pretty straight forward if we just knew where to point the notification anchor link in the event more than one comment/reply is added to more than one activity. Frankly, after looking through the code, I can't find a case where a "multiple" might occur!

I know we were looking at the @-mention notifications as a reference point, and I can see that that code accounts for multiples, but I can't think of a single condition in which a multiple might be generated within the @-mention notifications system, as these notifications (as well as replies) are generated as a single activity/comment is saved. Perhaps someone was planning for the future and the ability to allow users to choose just one notification a day with everything for the day in it? Or there could well be something I am just missing.

Regardless of whether there exists a condition that would generate a notification referring to multiple @-mentions, the code that currently sends the email notifications regarding replies (the "bp_activity_new_comment_notification" function) does not appear to account for any case of multiple's anyway, thereby negating the need for the answer we are waiting on.

Therefore, it's my vote that @imath :-) go ahead and put a solution together!!

My apologies if I've simply missed something and shredded any credibility I might have had…

Thanks,
Scott

#7 @imath
10 years ago

@dtz7240 thanks for your feedback and vote. I guess we need more time to think about it because point a) & b) are annoying. In the meantime, feel free to test this plugin https://github.com/imath/bp-activity-replies

#8 @dtc7240
10 years ago

@imath, I absolutely agree that points a) and b) are annoying! I agree with you wholeheartedly that adding another subnav item is not really desirable. Thanks for all the hard work on the plugin, but I already use the BuddyBoss Wall plugin and don't use all the subnavs that are already there. Fortunately, I think that points a) and b) don't have any relevancy, however. I don't believe there is an existing case where there would be the need for a "multiple" scenario at all. Am I missing something?

If I'm right, this is a very simple solution that simply mirrors the reply email notifications currently being sent and the anchor link will always point to the individual activity permalink...

#9 @imath
10 years ago

Not exactly. Screen notifications are grouping notifications having the same type when there's more than one.

Well i think it's very possible to receive more than one comment on more than one activity. So multiple is not an edge case.

#10 @boonebgorges
10 years ago

what should be the link of the notification in the WP Admin Bar if more than one comment/reply are added to more than one activity.

Seems like we might be overthinking this. If a notification is tied to more than one response *to the same top-level activity item*, then the link should go to the permalink of that top-level activity item. If a notification is tied to more than one response to *multiple top-level activity items*, just send them to their own Notifications page or something like that. Adding a new subnav for this seems like way overkill, and it's not worth losing this valuable feature because we don't have the "ideal" place to point the link to.

#11 @dtc7240
10 years ago

Thanks for the description @imath. Given Boone's comments as well, I guess I'm not aware of what "screen notifications" are...that or I'm still missing even more. So let me see if I'm on the right track:

  1. The action of a user saving a reply should populate the wp_bp_notifications table with a single entry pertaining to just that individual "reply notice". Right? That's what the data in the table seems to be set up to handle...
  2. When a user navigates to their notifications tab, is that where the potential to group together multiple lines from the table comes in? And do we call this "screen notifications"? The individual entries that I actually see listed on my notifications page?
  3. Boone suggested that if a notification were tied to more than one response to *multiple top-level activity items*, the anchor link behind the notification could just send them to their notifications page. But isn't that where we already are?
  4. IF this is the case, why don't we only "group" notifications together that pertain to a single activity thread and just always point to that activity permalink?

I do agree strongly with Boone that this feature is valuable enough to come up with a solution in the very near term. I think that one of the most important things that a user of a social network would want to know is when someone has responded to something they authored...

Thanks again for your attention to this,
Scott

#12 @imath
10 years ago

Thanks a lot @boonebgorges the notifation unread page is a great idea, i'll look at it.

@dtc7240 in the WP Admin Bar, notifications are grouped by type and clicking on the link should bring you to a specific page that is listing the replies. See the image > 1 bubble for 2 comments

https://farm9.staticflickr.com/8598/15805015489_63fb35bb7a_b.jpg
It's a screen capture of the plugin i was talking about

#13 @dtc7240
10 years ago

Ahhh...thanks @imath! Now I've got it. And Boone's idea sounds just like the ticket to me!

#14 @imath
10 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

6057.01.patch is bringing notifications for activity replies and activity comments replies.

If it's about 1 activity or 1 activity comment, the notification link is the original activity permalink. If it's about more than one activity / activity comment, the notification link is the user's notification screen with an extra parameter to pass the activity type to mark as read.

@imath
10 years ago

#15 @DJPaul
9 years ago

  • Milestone changed from Future Release to 2.4

@imath if you add a patch to something in the future release milestone, best to move it into the latest release otherwise we will probably never think to include the patch. :)

#16 @imath
9 years ago

@DJPaul, absolutely: my bad thanks for this :)

Version 0, edited 9 years ago by imath (next)

#17 @Tafmakura
9 years ago

  • Cc tafadzwamakura@… added

#18 @DJPaul
9 years ago

  • Keywords needs-refresh added; has-patch 2nd-opinion removed
  • Milestone changed from 2.4 to Future Release

This needs a refreshed patch and some attention for it to get into 2.5.

#19 @imath
9 years ago

  • Milestone changed from Future Release to 2.5
  • Owner set to imath
  • Status changed from new to assigned

#6781 is a new ticket about it. I will refresh my patch so that we can try to include this feature in 2.5

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


9 years ago

#21 @imath
9 years ago

  • Keywords has-patch added; needs-refresh removed

6057.02.patch is doing the following:

  1. manage screen notifications for activity comments and activity comment replies
  2. if there's only one notification of the above types, the WP Admin Bar bubble links to the single activity. Clicking on the link will mark the notification as read.
  3. if there's more than one, we use the user's notifications screen and an extra parameter to filter the notifications according to the type so that only these notifications are displayed. The notification won't be marked as read at this step because the content of the notified comment or reply is not displayed from this screen. Clicking on the entry link will mark the notification as read as it's the activity permalink.
  4. add new unit tests to prevent regressions in bp_activity_format_notifications() and in bp_activity_new_comment_notification() and to include some new specific tests to activity comment notifications.

If we agree on the logic described in points 2 & 3, i think it's good to go :)

@imath
9 years ago

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


9 years ago

#23 @imath
9 years ago

  • Keywords 2nd-opinion added

Some feedbacks about 6057.02.patch would be great...

#24 @studiovpc
9 years ago

Feedback after patch1, did not test patch2

not working yet

but here is what I have found so far.

  1. Creator of a topic, will receive BuddyPress notification balloon and mail if he subscribes to that topic or click on notify me of updates via mail
  2. Every other subscriber can ONLY get mail notifications if they subscribe or click notify me via email. No Buddypress notifications.

So this is the point, can there be something done so SUBSCRIPTIONS (subscribed topics) can go to BuddyPress notifications (baloon notifications)?

This is the whole point of BiddyPress notifications not functioning.

Other stuff like mail notifications not functioning can be resolved using WP SMTP for example or other mail sending plugin. WP itself can't send mails to hotmail users while gmail ones get it always. Using the external mail plugin resolves this, but you need to have some registered gmail account for example and use that as a sending mail adrress.

#25 @imath
9 years ago

Hi @studiovpc

Thanks for your feedback, if i understand your comment well, you are expecting bbPress to add screen notifications (into the WP Admin Bar). Unfortunately this ticket is about comments made on activities. Could you create a ticket on bbPress trac to give the bbPress core team your feedback ?

#26 @imath
9 years ago

  • Keywords needs-refresh added; has-patch 2nd-opinion removed

Patch needs a refresh since the great BP_Email feature has been committed, a lot have changed into this area.

#27 @imath
9 years ago

  • Milestone changed from 2.5 to 2.6

Let's work on this for 2.6

#28 @imath
9 years ago

  • Keywords has-patch 2nd-opinion added; needs-refresh removed

We should really include this in 2.6! I've just updated the patch, tested it and phpunittested it and it seems to work fine, what do you think ?

@imath
9 years ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


9 years ago

#30 @dcavins
9 years ago

@imath This is working really well for me. The decisions on what link to use in single and multiple-reference situations makes sense.

The only change I'd request is to a string: '%1$s commented one of your updates' should be '%1$s commented on one of your updates'.

Great improvement!

#31 @imath
9 years ago

Thanks a lot for your feedback @dcavins here is .04.patch to fix the string :)

@imath
9 years ago

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


9 years ago

#33 @boonebgorges
9 years ago

For the short description of bp_activity_sent_reply_to_update_notification and bp_activity_sent_reply_to_reply_notification, could we do something more general? The hook is not just for bp-notifications. Something like: "Fires at the point that notifications should be sent for activity comments." ("comments on activity replies" for the second filter)

#34 @imath
9 years ago

Sure, thanks a lot for your feedback.

#35 @imath
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 10715:

Introduce screen notifications for activity comments and activity comment replies

Users will be notified about comments made on their activities and about replies made on their activity comments.

If there is only one notification of the above types, the WP Admin Bar bubble will link to the single activity. Clicking on this link will mark the notification as read.
If there are more than one, we use the notifications screen and an extra parameter to filter the notifications according to the type so that only these notifications are displayed. Clicking on the listed links will mark the notification as read.

Props dcavins, boonebgorges, DJPaul

Fixes #6057

Note: See TracTickets for help on using tickets.