CUNY Academic Commons

Note from Commons meeting: If a message is sent to more than one group that you are a member of, you should only get one email notification/entry in your activity stream. (Like Twitter retweets)

Just want to flag that I'm excited to work on this, and starting in on it today!

So I think I have some of the basic functionality working: https://github.com/cuny-academic-commons/cac/commit/787581e49f06b6a8c18bc6a85a68f1051f4b51ea

I produce a list of groups based on the user's membership in them, whether they're public or hidden (based on the description at the start of the ticket), and whether their forum is open or not. I'm realizing now though that I also need to check the user's permission in that group, because they could be a member without having permission to post.

I use the function bbp_insert_topic to create the duplicate topics (which seems to be basically working!), which I think is good because of how much simpler it is than bbp_new_topic_handler, and also because it takes an array of arguments that I can provide instead of having try and figure out how to get around bbp_new_topic_handler's reliance on $_POST variables. The drawback is that bbp_insert_topic doesn't check anything about the user, the post, etc., so I'd appreciate some feedback on what kinds of things I should be checking for myself that I'm not right now.

The other thing I like about bbp_insert_topic is that it doesn't seem to automatically create activities, so nothing about the duplicate posts shows up automatically in My Commons, email notifications, etc. It also means though, that the topic creation doesn't show up in the Group's own feed, either, and I think we still want to show that the duplicates were created in My Commons, the activity feed, and emails anyway, just collapse it. Still, I think it's better to be creating those activities ourselves with the information we want than to be trying to interfere with the processes once they're moving.

Also, right now the only things that get duplicated are content, title, and author for the posts. I still need to add in tags, attachments, regular vs. sticky, and status, which I'm guessing will all be a little more difficult from looking at how bbp_insert_topic works, but not too hard.

I just wanted to check in about what I have so far before I went further, in case I need to rethink some of the basics here. Let me know what you think!

The way I'm thinking of dealing with the activities is adding a meta to the duplicates that saves the activity ID of the topic-creation activity they're a duplicate of, and adding a meta value to the original that's an array of activity IDs that are duplicates of it, and using that information for the feeds & emails. Does that sound right?

Hi Dan - Great work so far!! A few notes on https://github.com/cuny-academic-commons/cac/commit/787581e49f06b6a8c18bc6a85a68f1051f4b51ea -

1. Use `groups_get_current_group()` to get the current group object, instead reaching into the `$groups_template` global.
2. Use `bp_loggedin_user_id()` instead of reaching into the `$bp` global for the current user. In general, use `buddypress()->foo` instead of `global $bp; $bp->foo`.
3. Make sure to use a nonce when processing the field. See `wp_nonce_field()` for how to print the necessary fields in the form; you already know how to verify them.
4. Pass a third argument `true` to `groups_get_groupmeta()` to get a single value back. Then you won't have to grab the 0th item.
5. Don't call `groups_get_groupmeta()` twice in exactly the same way. Cache it in a local variable.

More generally responding to your points above:

I'm realizing now though that I also need to check the user's permission in that group, because they could be a member without having permission to post.

Yes. I don't know off the top of my head how bbPress checks to see whether a user can post in a group forum. Check out bbpress/includes/extend/buddypress/groups.php. On a related note, I'm not sure about the group status check you're doing (!= 'public). I assume you're trying to prevent the accidental leaking of private content, and this is definitely a good thing, but I don't think we want to prevent this from happening in such a definitive way. Let's do something on the front-end with JS: eg, when you're in a private group and check to repost to a public group, a notice "Some of the groups you've selected are not private" or something like this.

I use the function bbp_insert_topic to create the duplicate topics (which seems to be basically working!), which I think is good because of how much simpler it is than bbp_new_topic_handler, and also because it takes an array of arguments that I can provide instead of having try and figure out how to get around bbp_new_topic_handler's reliance on $_POST variables. The drawback is that bbp_insert_topic doesn't check anything about the user, the post, etc., so I'd appreciate some feedback on what kinds of things I should be checking for myself that I'm not right now.

You're doing it right. `bbp_insert_topic()` is designed to be a "business" function, something closer to a "model"-level function, while `bbp_new_topic_handler()` is a "controller"-ish function. I assume that the latter function doesn't do permission checks, etc, because it wants to leave open the possibility that third-party customizations may want to create topics in automated ways (like you're doing here). Checking that the user has the capability to post a new topic in the group seems like enough for me.

The other thing I like about bbp_insert_topic is that it doesn't seem to automatically create activities, so nothing about the duplicate posts shows up automatically in My Commons, email notifications, etc. It also means though, that the topic creation doesn't show up in the Group's own feed, either, and I think we still want to show that the duplicates were created in My Commons, the activity feed, and emails anyway, just collapse it. Still, I think it's better to be creating those activities ourselves with the information we want than to be trying to interfere with the processes once they're moving.

+1. Best to handle this ourselves.

The way I'm thinking of dealing with the activities is adding a meta to the duplicates that saves the activity ID of the topic-creation activity they're a duplicate of, and adding a meta value to the original that's an array of activity IDs that are duplicates of it, and using that information for the feeds & emails. Does that sound right?

Yeah, this sounds pretty good. I'd suggest storing "duplicate of" data with the duplicates, but not bothering with storing an array of duplicates with the original. That array will be serialized in the database, which means we can't do anything interesting with it in a query; if we need a list of items that are duplicates of x, we can do a separate query for it - it shouldn't introduce too much overhead. Mirroring the data for the purposes of quicker processing is a premature optimization (IMO). When it comes time to start working on this, note that the BP activity fetcher (like `BP_Activity_Activity::get()`) take a `meta_query` parameter, which will be perfect for what you're trying to do.

You're definitely on the right track, so keep marching on!

I think I have just the manual activity item creation working, but not the collapsing yet: https://github.com/cuny-academic-commons/cac/commit/890f8dd3ac41172686fa5423c32366ee6fe89fad

The thing that took a long time, and a very exciting journey through the bowels of the BP activity code, was that the new activities I was creating for duplicates were getting associated with the current group by default and showing up in its activity feed, instead of with the group that was getting the duplicate. Was hard to figure out because I couldn't find anywhere where any group ID was getting added to the activity, but eventually I found a function hooked onto a filter in bp_parse_args for recording a new topic that messed with the arguments before they went to bp_add_activity(). I decided the easiest thing would be to just go into the activity after it was saved and change its group manually ('item_id'), instead of trying to override the filter. If that's okay, I think now I can start working on adding the meta values and collapsing the notifications.

I also made some of the other suggestions you made above - the one issue I ran into was that even when I passed true as a 3rd argument to bp_get_groupmeta, it still returned an array, so I've left it as is for now but will try again soon.

On the hiding private groups from public group contexts, I was just going off the original description - "You should only be able to post public group to public group or public group to private group." I think your suggestion makes sense though, just didn't get to it this round.

Let me know what you think of all this.

Hey Dan - Great work so far. Being familiar with bp-activity, I probably would've just written the code to create the activity item myself - using `bp_activity_add()`. I wouldn't even have thought to use `BBP_BuddyPress_Activity`, but it's a clever idea, and it definitely reduces the amount of code you have to write.

Changing the activity's item_id after creation is a good idea, but I think it's going to have negative consequences elsewhere. For example, our email notifications are triggered when activity items are first created, which means that the initial ones will be sent to the wrong group - which is to say that when posting in group A and reposting to B and C, members of group A will get triple notifications. So we'll need to intervene earlier, before the activity item is created. Here, you should mimic what bbPress is doing. See https://bbpress.trac.wordpress.org/browser/trunk/src/includes/extend/buddypress/groups.php#L1431. bbPress hooks to 'bbp_before_record_activity_parse_args' to modify the item_id before the item is saved https://bbpress.trac.wordpress.org/browser/trunk/src/includes/extend/buddypress/groups.php?marks=131,132#L108 They use `groups_get_current_group()`, but of course you will use your `$group_id` inside of the foreach loop here https://github.com/cuny-academic-commons/cac/commit/890f8dd3ac41172686fa5423c32366ee6fe89fad.

the one issue I ran into was that even when I passed true as a 3rd argument to bp_get_groupmeta, it still returned an array, so I've left it as is for now but will try again soon.

So, I'm looking at this again, and I think the problem is not `groups_get_groupmeta()`, but it's that bbPress intentionally stores this data in an array, to accommodate cases where groups are associated with multiple forums. So there's no way around doing what you're doing (though I'd still put it in a local variable to avoid the overhead of running through all its filters more than once:

$forum_ids = groups_get_groupmeta( $group_id, 'forum_id' );

So I think the topic creation and activity parts are working well now: https://github.com/cuny-academic-commons/cac/commit/eea8ad5cc9e485e87c13c7d8286eaf65a297abd3

I have it so that the activities display like normal on groups' own activity feeds, but duplicates are hidden in other contexts (like user's own feed and in My Commons), and the other forums' names are added to the activity action description. There's one weird issue where what looks like a space is showing up between the original forum and the ones that got duplicates - as far as I can tell it's actually an result of the fact that HTML for the activity time since bit come after the first group and before the others. I'll look into how to get around this.

I also had to spend some time working on making sure that the group and forum last activity time was updated correctly, and that the topic count for the forum was updated correctly as well. There was a weird thing happening where the forum description was listing one number for total topics, but the pagination text was listing a different (and correct) total number. Turned out it was an issue with object caching for the meta information about the forum, but I think I fixed it. Let me know if I should do something else.

If this is all okay, then I think the next thing is to start working on email notifications, right?

Nice work on this, Dan!

Small thing: the way you're modifying the "action" string - manually replacing "forum" with "forums", concatenating a list of "x, y, and z" - is not going to be translatable. So if we ever decide to release this publicly, we'll need to change the way these strings are built. (In a nutshell: use an entire string with %s placeholders, and then use `sprintf()` to insert the dynamic values.) No need to do this now - what you've done here will work fine for the Commons.

The topic count and last_activity stuff is well done - I hadn't thought about that, but it looks like you got it figured out on your own :)

The duplicate-hiding in the activity streams is a good start, but I think we need to rethink it a little bit. You've done the hiding in activity-loop.php, but this means that the "heartbeat" feature on My Commons - which dynamically shows a "Load Newest" button at the top of the page when it detects "new" items - does not properly exclude the duplicates. This is, in part, a BP bug - BP should be using activity-loop.php for the heartbeat. See https://buddypress.trac.wordpress.org/ticket/6234. But we can skirt the entire issue by enforcing the duplicate-hiding at the level of the query, rather than the template. At first glance, the way to do this is with a filter like this:

function bpmfp_filter_duplicates_from_activity_queries( $args ) {
    if ( bp_is_group() ) {
        return $args;
    }

    $args['meta_query'][] = array(
        'key' => '_duplicate_of',
        'compare' => 'NOT EXISTS',
    );

    return $args;
}
add_filter( 'bp_after_has_activities_parse_args', 'bpmfp_filter_duplicates_from_activity_queries' );

But. (Always a "but".) This is not going to work in all cases. Say I'm a member of Foo, but not of Bar. Someone creates a forum post in Bar and copies it to Foo. If I don't see items with '_duplicate_of', I won't see this post in my stream. Ideally, what we want is something like: hide '_duplicate_of' items from my stream as long as the original will show up in the same stream. I don't think there's a way to write a single query that does this, which suggests that we'd need to do some post-query filtering:

function bpmfp_filter_duplicates_from_activity_results( $has_activities ) {
    global $activities_template;
    if ( ! bp_is_group() ) {
        foreach ( $activities_template->activities as $activity_index => $activity ) {
            // If $activity is a duplicate, check to see that the original is also included in the activities array
            // If yes, unset( $activities_template->activities[ $activity_index ] )
            // Otherwise, do nothing
        }
    }

    return $has_activities;
}
add_filter( 'bp_has_activities', 'bpmfp_filter_duplicates_from_activity_results' );

This will, in turn, throw off some activity counts, since there won't be 20 new results, so I guess at that point you'll have to backfill the proper number using another `bp_has_activities()` query, probably using an 'offset' param. (And in fact, you may have to do a `while` loop, since the backfilled items may also contain duplicates.) It's pretty ugly, but I can't think of a more reliable way to do what needs to be done.

Ray, if you have any thoughts on the above, they'd be most welcome.

A related, but smaller, point. We shouldn't show that a forum post has been cross-posted to a group if that group is private/hidden and the current user is not a member of it.

Thanks so much for the guidance on this one - I took another crack at it that I think addresses most of what you put out: https://github.com/cuny-academic-commons/cac/commit/b25269299e2f0039bf7a121ea7359dd31df7482e

The one issue I see still outstanding is back-filling the number of activities to get back up to 20. I'm really not sure off the top of my head of the best way to go about it, but will poke around some soon. Until then though if either you or Ray have any suggestions I'd love to hear them.

Hey Dan - This is looking outstanding. Exactly what I had in mind. Nice work!

The process of backfilling is going to be ugly, and may require the abstraction (or duplication) of some of your code. Here's what I'd suggest. At the end of `bpmfp_filter_activities()`, after you've reset your indexes (`array_values()`...), do something like this:

- Calculate how many items you've removed with the preceding logic. You can do this either by keeping a counter when removing, or by subtracting the new activity count from `$activities_template->activity_count`.
- So now, we have a strategy question. Say the backfill count is 5. We'll need to filter the backfilled items using the same deduplication logic that you used for the first 20 activities. That could leave us with only, say, 3 items. So then we'd have to do another backfill for 2 additional items. Etc. This is not going to perform very well. So I'd suggest taking the backfill count and adding some fairly generous amount - say, 20 - which will more or less guarantee that we'll get enough backfill items that we won't need to do another query. This is the ugly part :)
- Then, do an activity query. (Using `bp_activity_get()`, I suppose. Use the third param passed to the 'bp_has_activities' filter - `$template_args` - to get the necessary parameters.) Then, run the same deduplication logic on it. This suggests that you'll want to separate out the bulk of the logic of `bpmfp_filter_activities()` into its own function, which would accept an array of activity items and maybe a current user ID as its arguments. For efficiency's sake, you may also want it to accept something like a "number needed" argument; while looping through to identify duplicates, you can `break` out of the `foreach` once you've reached the proper number of backfill items.
- Then tack these items onto the end of `$activities_template->activities`, and be sure to decrement the `$activities_template->total_activity_count` property for any removed items.

Alright so I got something like this working: https://github.com/cuny-academic-commons/cac/commit/c080bef634496052c22299f7f10ed8f3a5ab91ad

There's a few issues with it though that I can see:

1- There could be a case where one of the activities we pull in when we backfill is actually itself a duplicate of one that's already in there, meaning that the user would see duplicates. To avoid this, I think we might have to run the function recursively, something like:

-In the function attached to the filter hook, check to make sure we aren't on a group page, then call some function like recursively_filter_activities(), then return $has_activities
-In recurseively_filter_activities(), first check to see if we have 20 activities, if we do, then run the de-duplication routine. If after the de-duplication routine we're below 20 again, then call recursively_filter_activities() once more
-If at the top of recursively_filter_activities, we don't have 20 activities, call for some new ones, then run the de-duplication routine, then check to see if we're at 20 and if we aren't, then call the function again.

I haven't been able to find something that works like an "offset" argument for bp_activity_get(), so instead I built up the 'exclude' argument from what was originally passed in $template_args + the IDs of all the activities in $activities_template->activities. I think that the recursive function would have to get passed an array of activities, as well as an array of activity IDs to be excluded in the call to bp_activity_get(), unless you can think of a better way, maybe using the date parameter? I couldn't find good documentation for WP_Date_Query and had a hard time figuring out how to use it.

Another option, which might be preferable, is to be okay with having less than 20 activities shown initially, and to just adjust $activities_template->activity_count downward based on how many duplicates get excluded the first time around.

The reason I think that might be preferable is that it's much cleaner and faster, and also because right now the way we're backfilling the first 20 activities is messing up the "Load More" functionality, which is showing some of the same activities that we backfilled. I have a feeling that fixing that might be a real headache, but I could be wrong on that front.

Let me know what you think about all this.

This is pretty great, Dan - thanks so much.

So, it so happens that I needed to do something very similar to this last week, for our upcoming Calendar function. I also thought of the recursive issue, and I solved it like this: https://github.com/cuny-academic-commons/bp-event-organiser/blob/master/includes/activity.php#L163 See from there to the end of the file. It's essentially what you describe, in terms of strategy, the tricks being as follows:

- The filter is on 'bp_activity_get', instead of on `bp_has_activities()`. This allows me to use `bp_activity_get()` internally, and automatically have the results filtered properly.
- But you'll notice that I only hook to 'bp_activity_get' during a `bp_has_activities()` loop. That way, "business" functions that need raw data can still get it.
- Like you, I'm using an 'exclude' param rather than an offset https://github.com/cuny-academic-commons/bp-event-organiser/blob/master/includes/activity.php#L226

Given that we've arrived at pretty much the same place independently of each other - great minds think alike :) - maybe you could look over what I've done and see if it's easily adaptable to what you're doing? I think you could copy it pretty much wholesale, and just replace my specific de-duplication logic (lines 171-217) with your own https://github.com/cuny-academic-commons/bp-event-organiser/blob/master/includes/activity.php#L171.

Dan - When you get a couple minutes, would you mind following up on this ticket? From my point of view, the stuff we've been working on - the dedupliation and backfilling - is not critical to launching this in 1.8. That is, if we had to ship it as-is, we could do so. Does that seem right to you?

Hey just wanted to flag I'm going to be jumping back on top of this this week. I think it's okay to ship without the deduplication, but right now I haven't done anything with the email notifications yet, so that might be a deal-breaker.

Thanks, Dan. I am working on the email notification stuff right now. I'll have an update shortly. (I'm now working in the 1.8.x branch.)

OK, I've made some improvements on this today:

• The list of checkboxes was getting long, so I'm hiding it with JS if it's longer than 5 items. A "Show More" link will expand the list.
• I wrote the logic to prevent duplicate BPGES notifications. This involved, among other things, storing "duplicate" metadata along with the forum topics themselves as opposed to just the activity items. In order for GES deduplication to work properly, the duplicate data needs to be available during the first activity item creation in a batch, which means that storing the duplicate info only with activity items doesn't work.

I think we're in a pretty good spot to ship this feature for 1.8, but I'll leave the ticket open in case any important bugfixes come in during testing.

Just want to flag that I think this weekend I can work on the duplicate email notifications and also (and sorry I didn't mention this earlier) making sure that topic tags and attachments are added to the duplicate topics.

Boone - on the attachments, do you have any initial guidance on the best way to do that/how attachments are stored and connected to topics? Should I try and find a way to just link to the original attachment or do I have to duplicate the attachments themselves.

Boone - on the attachments, do you have any initial guidance on the best way to do that/how attachments are stored and connected to topics? Should I try and find a way to just link to the original attachment or do I have to duplicate the attachments themselves.

Ugh, hadn't thought about this. I have no idea. For now, link to the original. In the medium term, we'll need to copy the attachment post (because attachments can only have a single post_parent) but probably have it point to the same file on the server. Check out functions like wp_update_attachment_metadata() and wp_generate_attachment_metadata() to see how this will have to be faked (or maybe just copied from the primary post).

One other thing - it looks like right now the email notifications de-duplication is only hooking into the case where a user is signed up for "All Email". Should we also hook into the process of adding activities to the digests? That might mean needing to hook into the "is this activity important?" filter, too.

Daniel Jones wrote:

One other thing - it looks like right now the email notifications de-duplication is only hooking into the case where a user is signed up for "All Email". Should we also hook into the process of adding activities to the digests? That might mean needing to hook into the "is this activity important?" filter, too.

No - this behavior is intentional. Digest content is organized by group, so if an event were listed only under one group, it'd be a kind of misinformation. Plus, having multiple entries for the same event in a single email is not really inbox flooding in the same way as multiple immediate emails would be.

Yeah that just occurred to me after I added the comment - thanks for clarifying.

So I think I got the deduplication working - thanks Boone for pointing me to your code! https://github.com/cuny-academic-commons/cac/commit/65e3f8a1917941f55c42afe2ec9cfffd4fa898f6

I ended up doing things a little bit differently, I'd like to hear what you think. The main thing I found myself wanting to avoid was calling bp_activity_get() from within the hooked function, and calling the function itself explicitly later. The main reason for that was that the bp_activity_get() call ended up calling the hooked function again, because it wasn't unhooked yet. Does that make sense?

So what I ended up doing instead was just abstracting my code for getting the IDs of duplicate activities into its own function, and calling that in the while loop after merging the backfilled activities with the original set. I also call BP_Activity_Activity::get directly instead of bp_activity_get() to avoid weird infinite recursion problems caused by how the hooking and unhooking work. I ended up avoiding recursion altogether here- letting the while loop do its job and running the deduplication logic in its own function.

I think the code could get DRY-ed out some more using a do-while loop but I haven't taken a shot at that yet.

One other thing is that when the user clicks "Load More" they do see some duplicates of activities that have already been shown. I'm not totally clear on how that AJAX request works - do you think there's an easy way around it? Is there some way to pass the list of IDs to exclude along to that request?

Let me know what you think of all this! I'll work on the email notifications issue, the problem with some groups not showing up, and the attachments and tags now.

I think I fixed the issue with email notifications: https://github.com/cuny-academic-commons/cac/commit/7c44956e89c8e2852582bd8ad0f15769bdbbd63d

Also fixed one other small issue, where people only saw the option to post to other groups is they belonged to at least three groups, when it should have been at least 2.

Let me know if this looks good! Still need to take a crack at attachments and tags. My bet is that tags are going to be much easier - for 1.8 would it be viable to just do tags and include a warning that attachments won't duplicate for now? If not that's okay too I'm sure it won't be too too difficult.

Sorry to spam here! Tags should be working now: https://github.com/cuny-academic-commons/cac/commit/7884f515dd75ff8ef54b76eb05d1e3e31e7a3689

The plugin we use for attachments is gd-bbpress-attachments, right? I can start looking at that today and see how hard it'll be. My guess is a lot will depend on what hooks & filters they're using.

The one other thing I just realized is that right now we aren't copying over the sticky or not status either. That should be easy to do, but I'm not sure it's what we want.

Okay hopefully this is the last one for today. Looks like attachments are working now - was much easier than I had thought it would be. Mostly just copied and modified code from the GD bbPress Attachments plugin - https://github.com/cuny-academic-commons/cac/commit/50a80070e73c35e7fe62d387826b0b49773cce7f

Thanks, Dan! I'm traveling and I'm unable to look at these changes in detail in time for the release, but they sound great to me. I'm going to pull them to cdev so others can check on their functionality.

Just to let everyone know from today's dev call that Dan's updated changes are on cdev and are ready for testing.

Dan - I made a slight adjustment to your code to check for BuddyPress and GD bbPress Attachments before calling some of their functions to prevent fatal errors in some instances. Other than that, I just tested a cross-post and tags and attachments are working for me!

Sorry to miss the call today! Thanks for making that change - makes sense. Besides tags and attachments the main issue was getting spammed with duplicate email notifications. I can try and test that today, but it'd be great if others could take a crack as well.

Congrats and thanks, Boone and Dan!