Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#5681 closed defect (bug) (fixed)

page directory ids don't update when component is deactivated

Reported by: modemlooper Owned by: boonebgorges
Milestone: 2.2 Priority: normal
Severity: normal Version: 2.0
Component: Core Keywords: good-first-bug has-patch
Cc: modemlooper@…

Description

I noticed this when working on API. If you deactivate a component or delete it's page it's still listed in the directory page id array. If you go into Settings > BuddyPress > Pages and hit save on any page it updates and removes the page from array.

Should a component disconnect itself from the page id when deactivated? Is it a safety thing incase you deactivate then reactivate to keep the page in sync?

Attachments (2)

5681.diff (1.4 KB) - added by Mamaduka 5 years ago.
5681-tests.diff (2.6 KB) - added by Mamaduka 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @modemlooper
5 years ago

  • Cc modemlooper@… added

#2 @boonebgorges
5 years ago

  • Milestone changed from Awaiting Review to 2.1

When the page is deleted, it should be removed from the array. That does seem like a straightforward bug.

Should a component disconnect itself from the page id when deactivated? Is it a safety thing incase you deactivate then reactivate to keep the page in sync?

My thought is that it should probably stay in the bp-pages meta value, but it should be filtered out at some point before being accessible to plugins, etc. Can you say more about how you're accessing this array? Are you reaching into $bp->pages?

#3 @modemlooper
5 years ago

The API returns basic info about BP install so an app can know what components are active and the page ids for each component. Using bp_core_get_directory_page_ids()

Yup, deleting page should remove it from array

#4 @DJPaul
5 years ago

  • Keywords needs-patch added; dev-feedback removed

#5 @DJPaul
5 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 2.1 to 2.2

Moving to 2.2 as I'm not sure the core team will have time to work on a patch. If anyone out there wants to submit a patch within the next 7 days or so, we can get it into the 2.1 release. There also ought to be a unit test demonstrating the fix.

#6 @DJPaul
5 years ago

  • Keywords good-first-bug added
  • Owner set to DJPaul
  • Status changed from new to reviewing

For anyone looking to work on this, the summary of the changes required are:

  • When a page mapped to a BP Component is deleted (or trashed), it should be removed from the "bp-pages" option.
  • When a Component is deactivated, the mapped page ID should remain in the "bp-pages" array, but not be returned in the results from the bp_core_get_directory_page_ids function.

We'll need unit tests for this, but if you're not sure how to add tests to BuddyPress, an initial patch can just contain the above changes, and we/another contributor can then work on the tests.

#7 @DJPaul
5 years ago

  • Status changed from reviewing to accepted

#8 @DJPaul
5 years ago

  • Owner DJPaul deleted
  • Status changed from accepted to assigned

#9 @modemlooper
5 years ago

Here's another weirdness we had at WDS. When the db was transferred over to prod the group page id was 6 on dev but the page id for 6 on prod was the homepage so the body classes for homepage got added to groups page. Even deleting and re-adding the groups page to get another page id did not fix. Required changing homepage page id.

#10 @boonebgorges
5 years ago

I'd assume that changing the value on BuddyPress > Pages and resaving would have fixed that specific issue? But yes, perhaps all these issues could be subsumed under a single re-save routine.

@Mamaduka
5 years ago

#11 follow-up: @Mamaduka
5 years ago

My patch includes only change required for core. Not sure how to handle tests for this one.

#12 in reply to: ↑ 11 @boonebgorges
5 years ago

Replying to Mamaduka:

My patch includes only change required for core. Not sure how to handle tests for this one.

Thanks, Mamaduka. This looks like a good change, but note that it's sorta the opposite of the bug originally being reported in this ticket.

As for tests, they will look just like the manual tests you were doing:

  • one test will delete a directory post, and then pull up bp_core_get_directory_page_ids() to make sure the post has been removed
  • another will delete a post that is *not* a directory post, then pull up bp_core_get_directory_page_ids() to make sure it hasn't been touched.

In each case, make sure that the test cleans up after itself, by ensuring that there is a full set of directory pages.

This ticket was mentioned in Slack in #buddypress by mamaduka. View the logs.


5 years ago

@Mamaduka
5 years ago

#14 @Mamaduka
5 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests removed

#15 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from assigned to closed

In 9177:

Improve matching between active component status and directory page status.

  • When a directory page is deleted, delete the corresponding entry from bp_pages. This ensures that the admin will see the admin_notice that a page mapping is missing.
  • When pulling up page mappings with bp_core_get_directory_page_ids(), remove entries corresponding to deleted pages and deactivated components.

Props Mamaduka.
Fixes #5681.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.