Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 21 months ago

#5834 closed defect (bug) (fixed)

Notice while activating a member (with no role on any blogs) on a multisite config

Reported by: imath Owned by: imath
Milestone: 2.1 Priority: normal
Severity: normal Version:
Component: Blogs Keywords:
Cc:

Description

Multisite config with users & blogs registration opened

Steps to reproduce :

  • register a dummy account by making sure you don't request a blog
  • with a super admin account, go to the Manage Signups screen and activate the member.

Notice error in bp_blogs_add_user_to_blog() using array_keys() at line 862. I think as the member has no role on any blogs the check for the user meta $wpdb->get_blog_prefix( $blog_id ). 'capabilities' simply returns an empty string.

Casting this to an array is fixing the issue, see attached patch.

Attachments (4)

5834.patch (610 bytes) - added by imath 5 years ago.
5834.unit_tests.patch (3.9 KB) - added by imath 5 years ago.
5834.solo_tests.patch (3.3 KB) - added by imath 5 years ago.
only the tests, previous was included the fix
5834.cant-trust-first-signup.patch (2.7 KB) - added by imath 5 years ago.
wow cannot trust the first signup !

Download all attachments as: .zip

Change History (26)

@imath
5 years ago

#1 @imath
5 years ago

It's also happening if a user registered a blog.

#2 follow-up: @boonebgorges
5 years ago

This seems like a fine fix to me. A unit test would be nice.

#3 in reply to: ↑ 2 @imath
5 years ago

Replying to boonebgorges:

This seems like a fine fix to me. A unit test would be nice.

Thanks for your feedback 5834.unit_tests.patch is 5834.patch + 2 tests :

  • one to test user accounts activation on regular/multisite configs
  • one other to test user accounts + blogs activation on multisite config.

#4 follow-up: @boonebgorges
5 years ago

Thanks, imath. Those tests look good and are nice to have, but they pass even without your patch to bp_blogs_add_user_to_blog(). Can you write a test just for this function that fails before your fix?

#5 in reply to: ↑ 4 @imath
5 years ago

Replying to boonebgorges:

but they pass even without your patch to bp_blogs_add_user_to_blog().

Oops my bad the last patch contains the previous one, i should have not include the fix in the patch sorry :(

If you checkout the changes to blogs functions file you should see tests failing

#6 @imath
5 years ago

Suddenly i have a doubt, checking !

@imath
5 years ago

only the tests, previous was included the fix

#7 @imath
5 years ago

*including*

Just checked and added 5834.solo_tests.patch so it's easier to only apply the tests.

The problem only occurs on multisite configs actually so make sure to use the -c tests/phpunit/multisite.xml option to see the tests failing.

On regular configs each member has a role, so there's no problem.

#8 follow-up: @boonebgorges
5 years ago

  • Keywords commit added

Thumbs up. Thanks, imath!

#9 in reply to: ↑ 8 @imath
5 years ago

Replying to boonebgorges:

Thumbs up. Thanks, imath!

Thanks, committing :)

#10 @imath
5 years ago

  • Owner set to imath
  • Resolution set to fixed
  • Status changed from new to closed

In 8965:

In {{{bp_blogs_add_user_to_blog
()}}} function make sure getting user roles returns an array

On multisite configs, when a member registers, he is not assigned to any blog and has no role.
When activating the user from the Manage signups screen, getting his role in bp_blogs_add_user_to_blog() was returning an empty string causing a notice error when trying to get the keys of the "capabilities" usermeta.
To avoid this, we now make sure this usermeta is an array.

Fixes #5834

#11 @boonebgorges
5 years ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Looks like this broke another signup-related test https://travis-ci.org/buddypress/BuddyPress/builds/33634198 My bad, I should have run the whole suite before adding the 'commit' tag. imath, can you have another look?

#12 @imath
5 years ago

Checking right away

#13 @imath
5 years ago

Weird it's one of the tests i've added BP_Tests_BP_Signup::test_activate_user_accounts_with_blogs
but it's working fine for me ?

#14 @boonebgorges
5 years ago

Are you running all the tests together? There must be some global stuff bleeding through, or some other test not cleaning up properly.

#15 @imath
5 years ago

Replying to boonebgorges:

Are you running all the tests together? There must be some global stuff bleeding through, or some other test not cleaning up properly.

Just did, and saw there was a trouble :( Searching what is the trouble..

#16 @imath
5 years ago

The problem is i use path blogone, blogthree

and in WP_UnitTest_Factory_For_Blog it sets testpath1 for the first blog without taking in account the signup 'path' attribute. But for the 3rd blog it's taking in account the path attribute of the signup ?!?

i've tried many things like setting the domain, path, title to nothing for blogone, it keeps on creating a blog having a path set to testpath1

The only way i got it to work was to set blogone's path to 'testpath' knowing at the end the blogone's path would be 'testpath1'. Very strange :(

But then if only testing --group BP_Signup it dramatically fails :(

#17 @imath
5 years ago

  • Keywords 2nd-opinion added

Ok i think we cannot trust the first signup, so in 5834.cant-trust-first-signup.patch, i suggest to avoid taking it in account.

I've double checked by running all tests and --group BP_Signup twice and it's working.

Boone, can you check it ?

@imath
5 years ago

wow cannot trust the first signup !

#18 @boonebgorges
5 years ago

  • Keywords 2nd-opinion removed

and in WP_UnitTest_Factory_For_Blog it sets testpath1 for the first blog without taking in account the signup 'path' attribute. But for the 3rd blog it's taking in account the path attribute of the signup ?!?

I don't really follow this. Where in your BP_Signup tests are you actually creating blogs? And why would that blog creation happen with WP_UnitTest_Factory_For_Blog?

Worst case scenario, you should go back to the drawing board and make your test smaller. You're running a big long test with signups. The bug here, however, is in bp_blogs_add_user_to_blog() Write the smallest possible test that demonstrates the error:

$u = $this->factory->user->create();
$b = $this->factory->blog->create();

$result = bp_blogs_add_user_to_blog( $u, $b );

$this->assertSame( null, $result );

This test kinda stinks because bp_blogs_add_user_to_blog() doesn't return any value on success, but that's an item for a separate ticket. The point is that this tests something much closer to the problem, and skirts your signup issue.

#19 follow-up: @boonebgorges
5 years ago

Tests pass for me. Go with it.

#20 in reply to: ↑ 19 @imath
5 years ago

Replying to boonebgorges:

Tests pass for me. Go with it.

Thanks doing it :)

#21 @imath
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 8966:

Edit the unit test test_activate_user_accounts_with_blogs()

Make sure the test is working while running all tests and only BP_Signup tests

Fixes #5834

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


21 months ago

Note: See TracTickets for help on using tickets.