Skip to:
Content

#4600 closed defect (bug) (fixed)

children of activity comments not populated with BP_Activity_Activity::get()

Reported by: sboisvert Owned by:
Milestone: 1.7 Priority: normal
Severity: minor Version: 1.7
Component: Activity Keywords: commit
Cc: stboisvert@…, PeterHatch

Description

When using get with activity comments the children don't come in like they do with activity posts. So for example if I have a activity post with children comments, I will see then as children in the object, sadly when getting comments of activity comments its not there.

Attachments (2)

bp-activity-classes.php.patch (2.9 KB) - added by sboisvert 18 months ago.
4600.02.patch (548 bytes) - added by r-a-y 18 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 boonebgorges18 months ago

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

(In [6410]) Ensure that BP_Activity_Activity::get() pulls up comments for non-top-level items

There was previously an exception in BP_Activity_Activity::append_comments()
that would make it impossible to load activity comments for activity items
that were themselves activity comments. This changeset removes the restriction,
and adds the necessary top_level_parent_id parameter to the get_comments()
method so that the comment tree is pulled up correctly in all cases.

Fixes #4600

Props sboisvert

comment:2 r-a-y18 months ago

  • Keywords needs-patch removed
  • Milestone changed from Awaiting Review to 1.7

comment:3 sboisvert18 months ago

  • Cc stboisvert@… added

Small modification to the change proposed, it seems that:
https://buddypress.trac.wordpress.org/changeset/6410
the line 458 is still:
BP_Activity_Activity::get_activity_comments( $activity->id, $activity->mptt_left, $activity->mptt_right, $spam );
while it should be passing the new $top_level_parent_id param like such:
$activity_comments[$activity->id] = BP_Activity_Activity::get_activity_comments( $activity->id, $activity->mptt_left, $activity->mptt_right, $spam, $top_level_parent_id );

comment:4 luccame18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm reopening this so devs can notice your update.

comment:5 boonebgorges18 months ago

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

(In [6431]) Pass missing param to BP_Activity_Activity::get_comments()

Fixes #4600

Props sboisvert

r-a-y18 months ago

comment:6 follow-up: r-a-y18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to 1.7

I came across a little bug during the fetching of activity comment children.

Try doing something like this:
http://i.imgur.com/xwW5i.png

Then, head to your profile and view your own activity stream. Look for activity comment entries and you'll notice that the activity comment is duplicated:

http://i.imgur.com/Yok1b.png

I've attached a patch that fixes this and wanted to know if this is a good, enough solution.

comment:7 in reply to: ↑ 6 johnjamesjacoby18 months ago

Replying to r-a-y:

I came across a little bug during the fetching of activity comment children.

I've attached a patch that fixes this and wanted to know if this is a good, enough solution.

Unsure if it's a bug, but it's a little weird. Should probably not show comments if the activity stream item type is already a comment.

comment:8 sboisvert18 months ago

something is wrong, I recall doing those exact tests with Boone and this was not happening... So while r-a-y's fix is good, it wasn't needed before (although I agree that it is currently needed with the patch as it stands, which is not expected behavior)

Last edited 18 months ago by sboisvert (previous) (diff)

comment:9 boonebgorges18 months ago

Ah. The problem is that the patch was only tested against the main, sitewide activity streams. On those streams, we don't show comments as top-level items. The behavior on user-specific activity streams is different: comments *are* shown as top-level items, as well as being shown in the context of their correspending top-level parent.

Personally, I think that our method of showing activity comments on the user activity stream is a little bit wonky anyway - even aside from the bug noted by r-a-y, activity comments will show up twice (in the top-level parent tree, as well as in the chronological flow). This is probably not the time or place to fix this UX issue, but it does at least show that there's no perfect way to accomplish what we want.

Instead of r-a-y's brute-force patch, maybe we could do something like the following: BP_Activity_Activity::get() could take another parameter (call it 'recurse_comments' or something like that). When true, comments are *always* pulled up for each item in the stream, even if the item is an activity_comment; when false, comments are skipped for activity_comments. The value would default to false, and in the context of the user activity page, we would manually set it to false (because we are showing activity_comments in the chronological flow). This is not incredibly elegant, but it's flexible, and allows us to maintain backward compatibility while also fixing the (IMO more serious) bug that was the original cause of the ticket.

comment:10 sboisvert18 months ago

Perhaps I'm not understanding fully what you want to do, but would the current 'display_comments' argument not achieve this currently?

// Alter the query based on whether we want to show activity item
// comments in the stream like normal comments or threaded below
// the activity.
if ( false === $display_comments || 'threaded' === $display_comments )
	$where_conditions[] = "a.type != 'activity_comment'";		

comment:11 r-a-y18 months ago

Personally, I think that our method of showing activity comments on the user activity stream is a little bit wonky anyway - even aside from the bug noted by r-a-y, activity comments will show up twice (in the top-level parent tree, as well as in the chronological flow).

Agreed. We should remove nested activity comments whenever bp_has_activities( 'display_comments=stream' ) is used.

comment:12 sboisvert18 months ago

I think I might of come to the root of the problem uncovered by r-a-y of the child comments finding themselves.

The way we find child comments is by looking for items that have lefts that are in between the left and the right of that node and whose top level parent is the same as our node.
The reason we were pulling ourselves in the query is that the SQL BETWEEN word uses the >= logic and not > logic therefore when we ask it to find values between the left and the right of our node, our node shows up since its >= in the between.

Changing from

AND a.mptt_left BETWEEN %d AND %d 

to

AND a.mptt_left > %d AND a.mptt_left < %d

Should fix that. That seems to be working and then we don't need to add a if inside our loop.
Thoughts?

comment:13 r-a-y18 months ago

  • Keywords commit added

Good catch, sboisvert!

I think this is ready to be committed. Boone, any thoughts?

comment:14 boonebgorges18 months ago

Hm, this seems right to me. Only question is whether there might be any existing implementations of BP_Activity_Activity::append_comments() that are taking advantage of the current, incorrect behavior. Can you imagine such a case?

comment:15 PeterHatch18 months ago

  • Cc PeterHatch added

If I get this right Boone you are trying to fix the problem of comments made to group activity posts disappearing when you refresh the page. This problem only occurs when commenting in the group activity stream and not when commenting in the sitewide activity stream or if you view the post individually first. Odd - so I looked at what was happening in the database and it appears that when commenting in the group activity stream the mptt_left and mptt_right are not updated in the comment or the post and are all left as zero. Posting a second comment in the sitewide activity stream or to an individual post will correct the fault and reveal the missing comment - or you can edit the database mptt_fields accordingly. I'm still using BP Version 1.2.6 with Custom Community theme.

So if you can work out why the mptt field is not updated when commentimng in the group that would cure the problem I guess

Sorry if I am barking up the wrong tree

comment:16 boonebgorges18 months ago

PeterHatch - It sounds like you're describing a different issue. The problem in this ticket is whether accessing a specific activity comment will also pull in its child comments. It's a problem of retreival, not storage.

Please download the latest development version of BuddyPress to a local dev environment to see if you can reproduce your problem there. If not, it's been fixed since 1.2.6. If so, then you should open a new ticket with your report. Thanks.

comment:17 johnjamesjacoby17 months ago

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

(In [6591]) Fix logic in get_activity_comments():

  • Use more accurate bounds when comparing mptt_left.
  • Fixes bug where activity comments would not appear on user profile pages where child comments exist in a hierarchical stream.
  • Fixes bug with spam SQL variable in same query.
  • Props sboisvert.
  • Fixes #4600.
Note: See TracTickets for help on using tickets.