Skip to:
Content

Opened 3 years ago

Last modified 3 years ago

#6234 new defect (bug)

Multiple scopes in activity-loop.php doesn't work correctly when activity auto-refresh is checked

Reported by: bphelp Owned by:
Milestone: Awaiting Contributions Priority: normal
Severity: normal Version: 2.2.1
Component: Activity Keywords: reporter-feedback needs-refresh needs-testing
Cc:

Description

After some test with the latest version of WP and BP I have found a slight bug with using multiple scopes in the activity-loop.php. Below is the code I used in the loop:

<?php if ( bp_has_activities( bp_ajax_querystring( 'activity' ) . '&scope=just-me,friends' ) ) : ?>

The reason I say it is a slight bug is because I found if you go to dashoard/settings/buddypress/settings and then go under activty settings and check activity auto-refresh then even a user you are not friends with will get a notice to load newest and you can briefly see that activity of the user you are not friends with until the page is refreshed, then it goes away.
If you use multiple scopes in the activity-loop.php as I did above in the code example and you go to dashoard/settings/buddypress/settings and then under activty settings and un-check activity auto-refresh then it works as expected and you only will see you and your friends activity in the activity stream. Hopefully there will be a fix but until then just make sure activity auto-refresh is unchecked. I hope this helps debug and helps others that may experience this issue. Thanks!

Attachments (2)

6234.patch (7.9 KB) - added by imath 3 years ago.
6234.02.patch (22.8 KB) - added by imath 3 years ago.

Download all attachments as: .zip

Change History (17)

#1 @boonebgorges
3 years ago

  • Keywords reporter-feedback removed

Thanks for the report, bphelp.

Are we sure that this bug has anything to do with the passing of multiple scopes? I have a feeling it might be related to the fact that you've hardcoded the scope parameters in your template. @imath, can you chime in?

#2 @bphelp
3 years ago

Hi @boonebgorges
I overloaded the template according to:
https://codex.buddypress.org/themes/theme-compatibility-1-7/a-quick-look-at-1-7-theme-compatibility/#overloading-template-compatibility-theme-files
Is there another way to implement multiple scopes without overloading the activity-loop.php template and hard-coding the scope parameters that I am unaware of? As I stated before it works the way I did it but if activity auto-refresh is checked in the dashboard/settings/buddypress/settings then a user you are not friends with still gets the load newest notice on the activity page and if clicked on then that user can see the activity briefly until they refresh the activity page then it goes away. @danbp also confirmed this phenomenon on this support forum thread:
https://buddypress.org/support/topic/multiple-scopes-in-activity-loop-php-not-entirely-working-in-bp-2-2-1/
Also please note I have tested this using the twenty-fifteen theme all the way down to twenty-thirteen with the same results. Thanks!

#3 @imath
3 years ago

Thanks for your feedback bphelp.

Will do some tests but i'm almost sure hardcoding the parameters into the template will not let the auto refresh know about them.

Have you tried using a filter on bp_ajax_querystring, for instance ?

#4 @r-a-y
3 years ago

bphelp, to help you debug, try this plugin:
https://github.com/r-a-y/bp-activity-home

It combines the just-me, friends and mentions scopes together into a "Home" tab on the Sitewide Activity page.

See if auto-refresh works there.

The plugin uses the bp_parse_args() approach, which alters the activity loop by filtering the activity arguments. Read this codex article for more info:
https://codex.buddypress.org/developer/using-bp_parse_args-to-filter-buddypress-template-loops/

Last edited 3 years ago by r-a-y (previous) (diff)

#5 @bphelp
3 years ago

Hi @r-a-y
I tested your plugin and it works great whether activity auto-refresh is checked or unchecked in dashboard/settings/buddypress. That would make a nice little plugin on the repo if you chose to put it there. I appreciate the codex link as well as it gives me a better understanding of bp_parse_args().
@imath is there anymore documentation on filtering bp_ajax_querystring other than the codex: https://codex.buddypress.org/developer/function-examples/bp_ajax_querystring/ because it doesn't go into very much detail.
Thank you both.

#6 @imath
3 years ago

You're welcome @bphelp

I've took the time to explore the reason why hardcoding the activity arguments into the activity-loop.php template was not working with "HeartBeat" activities despite the fact it's working with the "Load more" activities.

When clicking on the load more link, Ajax action is interpreted by bp_legacy_theme_activity_template_loader() in which the activity-loop.php template is used and as a result it gets the hardcoded arguments.

On the other hand "HeartBeat" activities in bp_activity_heartbeat_last_recorded() are using a specific loop to easily intercept the time of the last activity recorded to include it in its response to buddypress.js. If not being able to hard cod arguments into the template is really annoying for this feature, we'd have to change this in favor of using the activity-loop.php template and find a new way to get the time of the last activity recorded. Imho, using hardcoded arguments is not a good idea, but i don't mind working on this.

About the codex page, it might be interesting to upgrade it, i agree, i'll see what i can do about it. In the meantime, i've quickly built this "documented" gist : https://gist.github.com/imath/6017aec29dfcbde5e2ee

I hope it will help you :)

#7 @bphelp
3 years ago

Hi again @imath
If hard-coding arguments is not a good idea or the best practice since BP has had many changes and improvements in this last year alone then it would probably be a best to get some decent documentation on the codex so that users will know how to apply the best practices instead of the old ways of getting around things like I had done by hard-coding the parameters and arguments in an overloaded template file. https://codex.buddypress.org/developer/loops-reference/the-activity-stream-loop/ needs to be updated as well because it leads one to believe the only way one can alter the behavior of the activity stream is by hard-coding the parameters and arguments into the template file. It doesn't even mention template overload which shows how out of date this is. I am all for change if it improves BP but I would like to see the improvements/changes a little better documented on the codex so other users as well as myself know how to apply the best practices. Thanks for the gist link and thanks again for your time, I really appreciate it.

#8 @imath
3 years ago

@bphelp i understand your point. But, so far that's only my humble opinion :)

Possible scenarios are :

  • my opinion is shared by everyone, and we'll update the codex page.
  • i'm all alone, and we'll need to make sure hardcoding the arguments will also work in HeartBeat activities.

#9 @boonebgorges
3 years ago

Thanks for working through this, imath and bphelp.

If not being able to hard cod arguments into the template is really annoying for this feature, we'd have to change this in favor of using the activity-loop.php template and find a new way to get the time of the last activity recorded. Imho, using hardcoded arguments is not a good idea, but i don't mind working on this.

Hardcoding the params into the template is not ideal for a number of reasons - for one, it doesn't play nicely with our cookie implementation. But, regardless, bphelp is correct that this is what's been "officially" recommended for a long time. So I think we should do what we can to support it.

imath - Switching to activity-loop.php in bp_activity_heartbeat_last_recorded() doesn't seem at a glance like it'll be overly difficult. But if there are problems, here's another idea (I have no idea how hard it will be to implement): Maybe bp_has_activities() can include in its return value an array of parameters that it used to retrieve items; then we could print these as a JS variable using wp_localize_script(), and heartbeat could use them to retrieve the next items. The more I think about it, the worse this idea seems, but I thought I'd throw it out there :)

#10 @imath
3 years ago

Thanks boonebgorges for ideas, i'll explore the first one, then the second one if problems :)

#11 @imath
3 years ago

  • Keywords has-patch reporter-feedback added

6234.patch is a first approach to reply to @bphelp need.

But i can imagine potential issues, as :

  • i'm playing with the $_POST variable to be sure the activity-loop.php template will not output the ul & form elements
  • i rely on a template hook bp_after_activity_entry to get the activity last recorded time displayed in the activity-loop.php template. If a specific theme is removing this action, it will break the process :(

@bphelp could you test the patch to confirm it's solving your issue ?

@boonebgorges : i'm going to explore

Maybe bp_has_activities() can include in its return value an array of parameters that it used to retrieve items; then we could print these as a JS variable using wp_localize_script(), and heartbeat could use them to retrieve the next items. The more I think about it, the worse this idea seems, but I thought I'd throw it out there :)

It seems to be a better plan imho ;)

@imath
3 years ago

#12 @boonebgorges
3 years ago

The $_POST thing is such an awful hack. Not your fault - it's embarrassing that we do this check in the template. I assume this is for AJAX pagination or something? In a future version - maybe with a new template pack - we should stop doing this, maybe by having separate template parts for AJAX vs non-AJAX. (Or by fetching activity items via an API and building the page in JS - a man can dream!)

The bp_after_activity_entry part I'm not as worried about.

Looking forward to seeing the results of your exploration into the other option. I suspect that it's going to be pretty ugly...

#13 @imath
3 years ago

@boonebgorges i'm also dreaming about this :

(Or by fetching activity items via an API and building the page in JS - a man can dream!)

I think, we'll need to wait a bit for what i'm suggesting. I think, if we are to use the data that are prepended to a script using wp_localize_script(), then we need to first work on this one #5931 : it seems more natural to me to intercept the script data while we can, and as the activity loop happens after wp_head(), we need an extra time we can get by moving the javascript into the footer.

6234.02.patch is a working example of how this could look like.

Last edited 3 years ago by imath (previous) (diff)

@imath
3 years ago

#14 @DJPaul
3 years ago

  • Milestone changed from Awaiting Review to Future Release

If a ticket has a patch, then it ought to not be in the Awaiting Review milestone :)

#15 @DJPaul
3 years ago

  • Keywords needs-refresh needs-testing added; has-patch removed
Note: See TracTickets for help on using tickets.