Groups single item home : improve the way we deal with custom front and activity component

[imath]

Some screenshots will help me to describe the situation :

1. If the activity component is not active, the group home tab is almost the same than the members tab (the difference is that the group admins are not listed).

2. If there's a custom front in use in the group, members cannot post activities within the group but need to post them from the activity directory.

I think this should be improved, but we'll need to edit the templates...

In the attach patch i'm suggesting to have this instead :

1. Do not duplicate the members listing if the activity is not active
2. Make sure an activity tab is generated if the group's has a custom front page.

The patch also includes some kind of template hierarchy for the groups/single/front.php template

[imath]

As i'm using a similar approach for blog single items home (#6026), let's work on this for 2.4

[DJPaul]

This ticket is somehow confusing, I've started writing a reply two times before I realised I mis-understood the intent. :)

Moving the custom front logic out of the template into bp_groups_load_group_front_template etc seems to make sense as a general improvement. I haven't looked at it closely, but I would like us to check that the function name is consistent (do we do anything similar anywhere else?).

Pinging @mercime or @hnla to look at the proposed template hierarchy changes for this template (look at the bp_groups_get_group_front_template function in the patch) -- again, does this make sense / is the proposed file name/conventions consistent/make sense, etc. And also a reminder we'd need to update our documentation when this goes in.

:)

[imath]

@DJPaul i understand your concerns. And i'm ok to change the function name to whatever suits best :)

I really hope @mercime and @hnla will have a look to the proposed template hierarchy improvements suggested etc..

This ticket is really a stone in my shoe. And it hurts me bad, each time i think about it :)

So i'd really like if we could solve what i consider now as a several bugs:

• when a group has a custom template, it's losing the activity feature.
• when the activity component is disable and the group has no custom front, it has 2 members tab (home & members)
• we are using bp_is_group_home() to check if we are in the activity area of the group which is in my opinion wrong as the group can have a custom template. the right check needs to be bp_is_group_activity()

So the 6388.02.patch is updated to trunk and now includes some improvements to bp_is_group_activity() & bp_is_group_member()

[r-a-y]

Nice work, imath!

03.patch makes some minor changes:

• Removes function parameters in bp_groups_load_group_front_template(). I removed these parameters because I'm not sure when this function would need to be used outside a single group page.
• Renames bp_groups_load_group_front_template() to bp_groups_front_template_part() so it is similar to bp_groups_members_template_part().
• Renames bp_groups_get_group_front_template() to bp_groups_get_front_template().
• Registers the activity subnav at position 11 so the 'Activity' tab displays right after the custom 'Home' tab. Before, the 'Activity' tab would display near the very end of the group subnav, which looks a little awkward.

[imath]

Thanks a lot for your feedback and improvements @r-a-y

I will test them asap.

My first feedback without any testing is:
About your first point, i wasn't sure also, reason why i've left a possibility to plugin dev to know if a group was using a custom template by passing the group object. If you think it's not a good idea, then i'm ok with your recommandation, but then, i think maybe bp_groups_get_front_template() should also be edited so that it doesn't accept a group object but directly check the groups_get_current_group() :)

[imath]

@r-a-y Just tested and now i understand better your first point :)

Forget about my last comment, the fact that you leave the $group argument into bp_groups_get_front_template() is allowing plugins to test if the group has a front template :)

I love your version of the patch and i seriously think you should commit it!!

[imath]

Ah! Just tested using an unchanged groups/single/home.php and an unchanged activity/post-form.php and i'm afraid we might have to expose users who changed these templates or themes to issues. We first need to find a way to deal with backcompat.

[imath]

If the theme hasn't updated the templates, this is what's happening :
1- if no front.php template: everything works like before.
2- if front.php template : there's now an activity group page, but posting an activity is not posting it into the group but into the user's profile.
3- if front-id-{current_group_id}.php template, then we have 2 activity pages: home + activity. Posting from the first is ok, from the second it's case 2-

I'll try to see if we can avoid 2 & 3.

[imath]

6388.04.patch is my suggestion to deal with backcompat issue 2 & 3 of my previous comment. BTW 3 shouldn't happen if we have a codex page explaining the new template hierarchy for groups single item requires the template to be up to date :)

But anyway.. The trick is to add a new argument to an existing hook in this case : bp_before_activity_post_form. This argument is the BuddyPress version when the template has evolved.

For an outdated template, this version won't be there, so in this case i'm using a global to make sure it will still be possible to post activities. So this solves case 2.

This is also solving case 3, even if we have 2 activity page, because both are working :) And having 2 activity page will alert the user of the theme something is wrong with the theme, as soon as he activate WP_DEBUG as i'm using a doing it wrong.

The risk of this trick is if someone is hooking bp_before_activity_post_form with a function eg: 'my_hook' and uses this function directly passing to it an argument to act differently, then there might be a problem.. But this is really an edge case!

Another option is to add a private action at the top of each template looking like this
do_action( '_bp_template_activity_post_form_2_4_0' );

Then when in an older hook we could check if the template is up to date like this :
$is_uptodate = did_action( '_bp_template_activity_post_form_2_4_0' );

Another option, it we think people will update their template is to postpone this ticket to 2.5 and post on bpdevel and then do nothing for backcompat.

[imath]

Quickly made this plugin ​https://github.com/imath/bp-template-checker yesterday night to illustrate the do_action( '_bp_template_activity_post_form_2_4_0' ); option.

[imath]

Forget about my funky 6388.04.patch and this ​comment (i was very blind!).

I just opened my eyes and find a much better solution to the potentiel backcompat issue!

In this patch we're editing the activity post-form template and the group's home template. So as i've said earlier (i wasn't completely blind), if these templates were overridden and not updated once this patch gets in (I pray for 2.4!!!), then we meet points 2 & 3 described ​here

Imho if point 3 happens it's the site admin's fault! Because if he wants to use the template hierarchy we're introducing, it's his responsability to update his group's home templates. We just need to put in CAPITALS STRONG: "before using the front template hierarchy update your templates!!!" into the codex article :)

So point 2 is the one we need to fix and 6388.05.patch is fixing it in a very simple way!

i'm just using a fallback on bp_is_group() to set the object and the item_id of the activity if the corresponding $_POST vars didn't make it (because the administrator/theme designer forgot to update the activity post form..)

So i say, let's commit 6388.05.patch and have great groups front page in 2.4 :)

[r-a-y]

Love the new approach, imath!

Commit anytime!

[boonebgorges]

Yeah, the approach seems much better. I'm not worried about what happens if an admin has implemented a custom front.php or front-id-123.php template but *not* updated the other templates - as you say, that's their responsibility. I just want to make sure that sites without any front.php (that is, all existing BP sites!) continue to work as before.

As for your question from Slack:

. I can see another potential risk with plugins now :( if some (i think it's many actually) are using bp_is_group_home() to check they are on the group's activity page then we might break some of their features :( What do you think if we deprecate bp_is_group_home() in favor of bp_is_group_front() and make bp_is_group_home() act like bp_is_group_activity() ?

The change you suggest would break plugins that expect bp_is_group_home() to report whether you're looking at example.com/groups/foo/, with no bp_current_action(). That is, after all, what "group home" suggests. So I say no: if we're going to allow custom group front pages, we should keep bp_is_group_home() just as it is. Let's make a blog post about it for bpdevel.

One thing that would make things easier is if example.com/groups/foo/, with 'activity' set to the home page, made it true that bp_is_group_activity(). It doesn't do that, because it'd mean manually setting the current_action during BP_Groups_Component::setup_globals(). Is there any reason we can't do this? Or maybe bp_is_group_activity() should be smarter: it checks for bp_is_current_action( 'activity' ) OR bp_is_group_home() && activity_is_the_front_page_for_this_group() (replace that with a real way to check ;) ). This way, we can tell people that they ought to use bp_is_group_activity() for, well, checking if you're looking at group activity :)

[r-a-y]

The change you suggest would break plugins that expect bp_is_group_home() to report whether you're looking at example.com/groups/foo/, with no bp_current_action().

If you are on example.com/groups/foo/, the current action is 'home', so that wouldn't break any checks to see if bp_current_action() is empty.

One thing that would make things easier is if example.com/groups/foo/, with 'activity' set to the home page, made it true that bp_is_group_activity(). It doesn't do that, because it'd mean manually setting the current_action during BP_Groups_Component::setup_globals(). Is there any reason we can't do this?

This sounds pretty good as well, but this might break those that do explicit checks on the 'home' action. Not sure who would do that, but it's possible.

[boonebgorges]

If you are on example.com/groups/foo/, the current action is 'home', so that wouldn't break any checks to see if bp_current_action() is empty.

Ah right, thanks - but the point is the same. bp_is_group_home() should report whether it's the group home, bp_is_group_activity() should report whether it's the group activity. I say we make them work the way they sound like they ought to work, and we can publicize the change for anyone doing something weird. (Might be worth a search in the plugin repo too.)

but this might break those that do explicit checks on the 'home' action. Not sure who would do that, but it's possible.

Yeah. I think the safer place for the fix is bp_is_group_activity().

These comments aren't meant to block progress on the ticket. It's just that if we're going to announce that bp_is_group_home() now works a bit differently, we should also be able to announce that we have another function you should use it its place :)

[imath]

Thanks a lot to both of you for your great feedbacks, i think i'm now sure about what's best to do :)

As seen on slack, there was some misunderstanding about what the patch is doing. So let's compare what's happening today to what will happen once the 6388.06.patch will be committed (i've edited mentions & heartbeat functions so that they use bp_is_group_activity()).

Today:

1. bp_is_group_activity() has no utility because it's always false. The reason for that is due to the fact that the only place where the group's activity template is loaded is the group's home.
2. when there's a front.php template in /buddypress/groups/single/ the activity template is simply no more showing. If the activity component is deactivated, there are 2 members templates loaded (home + members).

Once the patch will be committed:

1. bp_is_group_activity() will return true each time the activity template will be displayed (in group's home or in the activity page if there's a custom front). So this function now does what it should, and plugins wishing to do things in group's activity should use this check systematically.
2. bp_is_group_members() will return true each time the members template will be displayed (in group's home or in the members page),
3. bp_is_group_custom_front() will return true if the current displayed group is using a custom front.
4. bp_is_group_home() will return true if we're on group's home, but this will not necessarily mean it's the activity template that is loaded. It can be a custom front. And if the activity component is not active it can be the members template.
5. backcompat with themes who might not have updated their templates once 2.4.0 is released will be ok.

Finally, my last hesitation was about plugins using bp_is_group_home() to check if the activity template is displayed. As we've seen in a. It's already a mistake to do that. But we didn't have the right conditional tag so far to help them be sure the group's activity page was displayed. Once this ticket will be fixed, they'll have it, it's bp_is_group_activity().

I thought about deprecating bp_is_group_home(), but if we do that every standalone BuddyPress theme or end user having a config where the group's home template is overridden will be annoyed. So as a plugin developer, i know my "colleagues" will do their best to adapt and edit their plugins if needed, like i'll have to do myself!

The good news are :

• Groups will potentially have great custom front pages, these front pages will be able to be different (according to the displayed group id/slug or status) thanks to the template hierarchy introduced.
• We will be able to have an activity page if the group has a custom front !!

I'll commit asap and post on bpdevel about it :)

[imath]

In 10184:

Improve template loading for Groups single items home pages.

• Make sure an activity sub navigation will be generated if the displayed group has a custom front page.
• Make sure no members sub navigation is generated if the activity component is not active and the Group has no custom front. As, in this case the home page of the group is already displaying the members template.
• Make sure some Groups single item conditional tags are behaving the right way:
  • bp_is_group_home() is true when on the home page of the group (eg: site.url/groups/single-group/).
  • bp_is_group_activity() is true when the activity page of the group is displayed. It can be the home page of the group or its activity page (eg: site.url/groups/single-group/activity).
  • bp_is_group_members() is true when the members page of the group is displayed. It can be the home page of the group or its members page (eg: site.url/groups/single-group/members).
• Introduce a new conditional tag: bp_is_group_custom_front() to check if the home page of the group is using a custom front template.
• Introduce a new template tag bp_groups_front_template_part() used to choose the appropriate template to load for the home page of the group (activity, members, or the custom front).
• Introduce a template hierarchy for the buddypress/groups/single/front.php template so that it is possible to have different front pages according to the ID, slug or status of the Group.
• And finally make sure the introduced improvements are back compatible with themes who forgot to update their buddypress/groups/single/home.php and buddypress/activity/post-form.php templates.

Props r-a-y, boonebgorges, DJPaul, and imath :)

Fixes #6388

[hnla]

I think we ought to add a simple php comment to explain what's happening when we run:`
bp_groups_front_template_part();

A brief explanation that one can add a template named 'front' which will be used as homepage for the group.

We sort of explained that by virtue of the way things were before showing us loading 'front.php' if it were located.

Possibly with a link to the fuller codex guide?

[imath]

@hnla no objections about it. FYI, i've already edited the codex page to include an explanation of the groups front template hierarchy: ​https://codex.buddypress.org/themes/theme-compatibility-1-7/template-hierarchy/#single-groups-front-page