Skip to:
Content

bbPress.org

Opened 14 months ago

Last modified 5 months ago

#3399 accepted defect

BuddyPress: Toggling group forum creates a duplicate forum

Reported by: r-a-y Owned by: johnjamesjacoby
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.0
Component: Extend - BuddyPress Keywords: has-patch dev-feedback
Cc:

Description

In BuddyPress, if you create a group with a forum attached to it, then navigate to the group's "Manage > Forum" page to disable the forum and save. Next, if you re-enable the forum and save again, another forum with the same name as the group will be created and will be used as the new group forum.

The previous forum still exists, but is now no longer attached to the group.

The issue is when removing and re-enabling the forum, bbPress doesn't record the previous forum ID and doesn't look for the previous forum.

I have attached a patch, which fixes this issue.

Attachments (1)

3399.01.patch (1.2 KB) - added by r-a-y 14 months ago.

Download all attachments as: .zip

Change History (5)

@r-a-y
14 months ago

#1 @r-a-y
14 months ago

I should note that if #3014 is implemented, this patch will not work.

#2 @johnjamesjacoby
10 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 2.6.7
  • Owner set to johnjamesjacoby
  • Status changed from new to accepted

#3 @johnjamesjacoby
10 months ago

  • Keywords reporter-feedback 2nd-opinion added
  • Milestone changed from 2.6.7 to 2.7

Hey @r-a-y! Thanks for the patch.

The approach you've taken works as intended for me, but...

I think that BBP_Forums_Group_Extension might make more sense, since it has methods for editing groups where a "trash/untrash" type functionality could be introduced, specifically disconnect_forum_from_group(), 'remove_forum()', and toggle_group_forum().

Then, bbPress can have new & separate functions similar to the others but specifically for interacting with the "previous" meta-data, and it can better target the edit action specifically instead of always assuming that bbp_remove_forum_id_from_group() handles it.

Bonus points for coding it in such a way that the hypothetical "multiple forums per group" functionality is not inhibited. I know it isn't fully implemented in the UI, but the current code is written with that in mind.

Going to bump this into the 2.7 milestone so there is a bit more time to iterate on the changes here.

Version 0, edited 10 months ago by johnjamesjacoby (next)

#4 @r-a-y
5 months ago

  • Keywords dev-feedback added; reporter-feedback 2nd-opinion removed

Just getting to this.

Regarding:

I think that BBP_Forums_Group_Extension might make more sense, since it has methods for editing groups where a "trash/untrash" type functionality could be introduced, specifically disconnect_forum_from_group(), remove_forum(), and toggle_group_forum().

Then, bbPress can have new & separate functions similar to the others but specifically for interacting with the "previous" meta-data, and it can better target the edit action specifically instead of always assuming that bbp_remove_forum_id_from_group() handles it.

I think bbp_remove_forum_id_from_group() is the proper place to introduce the saving of the previous forum ID because it has all the validation logic.

If you want to move the previous forum ID saving logic back to the remove_forum() method, then that'd require duplicating the functionality from bbp_remove_forum_id_from_group().

Bonus points for coding it in such a way that the hypothetical "multiple forums per group" functionality is not inhibited

This would mean allowing multiple forum IDs to be passed to bbp_remove_forum_id_from_group() and also reworking this portion of the code for the edit_screen_save() method:
https://bbpress.trac.wordpress.org/browser/trunk/src/includes/extend/buddypress/groups.php?marks=551-562#L543

So it will pass multiple forum IDs to the remove_forum() method. Likewise for the disconnect_forum_from_group() method. toggle_group_forum() doesn't look like it requires any changes.

If that's what you're thinking, let me know and I'll create a new patch.

Last edited 5 months ago by r-a-y (previous) (diff)
Note: See TracTickets for help on using tickets.