Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6197 closed defect (bug) (fixed)

Register/Activate pages and bp_core_get_directory_page_ids() since 2.2

Reported by: imath Owned by: imath
Milestone: 2.2.1 Priority: normal
Severity: normal Version: 2.2
Component: Registration Keywords: has-patch commit
Cc:

Description (last modified by imath)

#6176 is a relative ticket.

Context :

  • Multisite config
  • User registration is set to 'blog' in other words 'Logged in users may register new sites'

If we go to site.url/wp-signup.php we get :
https://cldup.com/yQyvkBDnzF.png
WordPress displays a message to inform registration are closed.

BuddyPress 2.1.1

Without creating the pages 'register' and 'activate' nor touching the page setting association settings, if we go to site.url/wp-signup.php, we are redirected to site.url/register and we get a 404.

If we create the 'register' and 'activate' pages and set the page association settings, we get :
https://cldup.com/JJiIskSjTR.png

After Upgrading to BuddyPress 2.2.0

if we go to site.url/wp-signup.php, we are redirected to site.url/register and we get a blank page if the slug of the 'register' page is 'register' if it's something else we get a 404.

Applying the attached patch bring us back to 2.1.0 situation for the activate and register page. I still need to work on unit tests to cover the different possible situations.

Attachments (3)

6197.patch (551 bytes) - added by imath 5 years ago.
6197.unittests.patch (7.9 KB) - added by imath 5 years ago.
Only unit tests
6197.unittests.02.patch (6.3 KB) - added by imath 5 years ago.

Download all attachments as: .zip

Change History (21)

@imath
5 years ago

#1 @imath
5 years ago

  • Description modified (diff)

@imath
5 years ago

Only unit tests

#2 @imath
5 years ago

  • Keywords needs-unit-tests removed

Just added some unit tests.

#3 @boonebgorges
5 years ago

Awesome - thanks, imath. Regarding the changed test (test_bp_core_get_directory_page_ids_should_not_contain_register_and_activet_pages_when_registration_is_closed()). You've modified it to show what happens when registration is closed but the pages do not exist. Presumably, part of the problem described in this ticket is that sometimes registration is closed but the pages *do* exist. What should be the behavior in these cases? We should probably have a separate test for it.

#4 @imath
5 years ago

You're welcome.

Actually I thought this test was covering the case :
test_bp_core_get_directory_page_ids_register_activate_pages_created_signup_not_allowed()

In this case going to the registration page should display this :
https://cldup.com/JJiIskSjTR.png

#5 @imath
5 years ago

and bp_core_add_page_mappings() will not create the activate/register page if signups are not allowed. So it must be the admin to set it from the Administration and then we are using bp_core_admin_slugs_setup_handler() so i think that's what i'm doing in test_bp_core_get_directory_page_ids_register_activate_pages_created_signup_not_allowed()

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

#6 follow-up: @boonebgorges
5 years ago

I guess I trust you here, but I'm still confused about what's happening and what you intend to be testing. At a glance, it seems like we need tests for the following:

  • Ensure that bp_core_get_directory_pages() returns 'register' and 'activate' when registration is active and the pages exist
  • Ensure that bp_core_get_directory_pages() returns 'register' and 'activate' when registration is *inactive* and the pages exist (right?)
  • Ensure that bp_core_get_directory_pages() does not 'register' and 'activate' when registration is active and the pages don't exist
  • Ensure that a notice is thrown when registration is active and the pages don't exist
  • Ensure that the notice is not thrown when registration is active and the pages do exist

Exactly *how* the pages are created during the tests is not terribly important.

Again, though, I don't 100% understand the issue here. For me, the systematic tests are a way to make the problem easier to understand, and in this case I feel like the tests don't cover all the cases. That being said, if you do understand it and are confident with your solution, then go with it :)

#7 @DJPaul
5 years ago

I think we need to take a bit of time to make sure we're all clear on the issue, and what the unit tests to be covered, especially if we want to ship this in a minor release.

Last edited 5 years ago by DJPaul (previous) (diff)

#8 @imath
5 years ago

Thanks boonebgorges & DJPaul for your feedbacks. I'll review the cases boonebgorges listed and i agree with taking the time on this one. Also, i think it could be interested to ask people who reported problems on the forum if they could test the patch to see if it's solving their problem (as it's just removing a part of an if statement).

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

Replying to boonebgorges:

In 6197.unittests.02.patch, i'm easing the unit tests the way you suggested

  • Ensure that bp_core_get_directory_pages() returns 'register' and 'activate' when registration is active and the pages exist

This test should cover it : test_bp_core_get_directory_pages_register_activate_page_created_signups_allowed()

  • Ensure that bp_core_get_directory_pages() returns 'register' and 'activate' when registration is *inactive* and the pages exist (right?)

Yes it's right and it should fix some issues posted in the support forum.
This test should cover it : test_bp_core_get_directory_pages_register_activate_page_created_signups_notallowed()

  • Ensure that bp_core_get_directory_pages() does not 'register' and 'activate' when registration is active and the pages don't exist

This test should cover it : test_bp_core_get_directory_pages_register_activate_page_notcreated_signups_allowed()

  • Ensure that a notice is thrown when registration is active and the pages don't exist

This test should cover it : test_bp_core_activation_notice_register_activate_pages_notcreated_signup_allowed()

  • Ensure that the notice is not thrown when registration is active and the pages do exist

This test should cover it : test_bp_core_activation_notice_register_activate_pages_created_signup_allowed()

#10 @boonebgorges
5 years ago

Thanks for bearing with me, imath - it's starting to make more sense to me.

If I understand correctly, test_bp_core_get_directory_page_ids_should_not_contain_register_and_activet_pages_when_registration_is_closed() - which you have modified - is covering the case where signups are *closed* and where 'register' and 'activate' *do not exist*, right? So it completes the matrix, so to speak?

It's looking to me like https://buddypress.trac.wordpress.org/attachment/ticket/6197/6197.patch is probably the right fix, but it would be great (as imath noted) to get some feedback from people who are experiencing the problem on the forums.

#11 @imath
5 years ago

I must admit my first tests was over complicated! Sorry for that boonebgorges

I actually added another test in 6197.unittests.02.patch :
test_bp_core_get_directory_pages_register_activate_page_notcreated_signups_notallowed() it directly covers the case

where signups are *closed* and where 'register' and 'activate' *do not exist*

test_bp_core_get_directory_page_ids_should_not_contain_register_and_activet_pages_when_registration_is_closed()
is creating pages then deleting them then checking register and activate are not in page_ids. So it might be a good idea to remove it. I actually adapted it because he was here before.

I've created a small script to add in bp-custom.php file to retain the activate/register page ids, i think it's safer to ask people testing it instead of asking them to edit a core file.

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


5 years ago

#13 @imath
5 years ago

:( the user in support forum cannot (or is afraid to) help. So i'll run some tests with invite-anyone today.

#14 @imath
5 years ago

Just tested the patch with the invite anyone plugin activated. I confirm it's fixing the issue.

#15 @boonebgorges
5 years ago

  • Keywords commit added

Thanks for looking into it more, imath. I think your tests and the fix look fine. Let's go with them for 2.2.1.

#16 @imath
5 years ago

Ok thanks for your feedback boonebgorges, committing it asap.

#17 @imath
5 years ago

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

In 9478:

Make sure Site admins can set the "register" and "activate" pages in the pages association settings screen even if signups are not allowed

In 2.2, when signups are not allowed, it is not possible to associate a WordPress page to the register or the activate specific components. It introduced some confusion for some site admins and had an impact on plugins allowing users to invite others.

We will keep on working on the best way to deal with these directory pages in the next release. In the meantime, we are bringing back the possibility to always set the association for the register and the activate specific components.

props boonebgorges

See #6176
Fixes #6197 (trunk)

#18 @imath
5 years ago

In 9479:

Make sure Site admins can set the "register" and "activate" pages in the pages association settings screen even if signups are not allowed

In 2.2, when signups are not allowed, it is not possible to associate a WordPress page to the register or the activate specific components. It introduced some confusion for some site admins and had an impact on plugins allowing users to invite others.

We will keep on working on the best way to deal with these directory pages in the next release. In the meantime, we are bringing back the possibility to always set the association for the register and the activate specific components.

props boonebgorges

See #6176
Fixes #6197 (branch 2.2)

Note: See TracTickets for help on using tickets.