Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 9 years ago

#5441 closed defect (bug) (fixed)

Profile Settings show "Email" sub tab even if content is empty

Reported by: landwire's profile landwire Owned by: boonebgorges's profile boonebgorges
Milestone: 2.2 Priority: normal
Severity: normal Version: 1.9.2
Component: Settings Keywords: good-first-bug has-patch needs-testing
Cc: brad@…, georgemamadashvili@…

Description

On the public profile the sub tab "Email" shows up, even if there are no options to choose. It still shows:
"Send an email notice when:" and then there are no options just the save button.

Surely the default behaviour should be to check if there are any email settings to set and then show the tab and the form.

Maybe the tab could also be changed to: "Email Notifications" or "Notification Settings". "Email" to me suggested that I change my email address under that tab.
Sascha

Attachments (2)

email_settings_tab_conditional_check.patch (1.8 KB) - added by pro120 9 years ago.
First attempt at conditional check for the email settings tab
5441.diff (1.8 KB) - added by Mamaduka 9 years ago.
Remove 'Email' sub nav, if there're no components using it.

Download all attachments as: .zip

Change History (13)

#1 @boonebgorges
10 years ago

  • Milestone changed from Awaiting Review to 2.0

Yes, this seems like a reasonable fix.

Can you please share more info about your configuration? It's unusual that there would be no options listed here. I assume that you've only activated the Members and Settings component, and all other components are inactive?

#2 @boonebgorges
10 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.0 to 2.1

Sorry we didn't get around to fixing this for 2.1. It should be a straightforward fix for 2.1.

#3 @williamsba1
10 years ago

  • Cc brad@… added

I was able to reproduce this having only the Members and Settings components active: https://www.dropbox.com/s/b34y3blmryb3s9t/Screenshot%202014-04-18%2011.54.01.png

Should we hide the Email tab all together or change the message displayed or?

I'll can work on a patch to fix.

#4 @boonebgorges
10 years ago

williamsba1 - Thanks for verifying. I'd say hide the Email tab altogether, assuming it's easy to do. (Hint: I'm guessing you can do it with has_filter().)

#5 @DJPaul
10 years ago

  • Keywords good-first-bug added

I've marked this as good_first_bug, though it's a little more complicated to understand than some of the others as it involves the black box that is BuddyPress' navigation/route building code. :) Maybe good if you've submitted a few patches to BuddyPress before, or are an experienced WordPress developer. Adventurous explorers should grab a machete and take the following to heart:

  • Recreate the issue by going to [your profile] > settings, when you ONLY have the Members and Settings component active. You should see what williamsba1 included in his screenshot.
  • The email navigation menu/item is added here: https://buddypress.trac.wordpress.org/browser/trunk/src/bp-settings/bp-settings-loader.php?rev=8568#L103
    • You should be able to comment that block out and see the subnav disappear; in fact, if you were on the email settings screen when you make this change, the request should 404 as the nav item doesn't exist.
    • Another way to remove the nav item is something like this (untested): bp_core_remove_subnav_item( buddypress()->settings->slug, 'notifications' ).
    • The component class' setup_nav methods are invoked by the bp_setup_nav action.
  • I think the tricky part is deciding when/where to remove the code. Adding a conditional block around the segment in bp-settings-loader.php is tempting, but due to possible load order problems, and to make it easy for developers to un-hook, it should probably be done in a separate function. I'm not sure where to suggest this should be done; perhaps late (> 10) to bp_setup_nav.

#6 @DJPaul
10 years ago

  • Milestone changed from 2.1 to 2.2

@pro120
9 years ago

First attempt at conditional check for the email settings tab

#7 @pro120
9 years ago

So here is the first set of components we could used to check for. I also added a filter to allow custom components to turn on the email tab as well. Could use some input on what the $defaults should be if we go with this approach.

$defaults = array( 'friends', 'forums', 'messages' );
$components_with_email = apply_filters( 'bp_components_using_email_tab', $defaults );

#8 @boonebgorges
9 years ago

pro120 - Thanks for the patch. I like how your approach is fairly straightforward, but I am nervous that there are edge cases where it will cause backward compatibility problems. In particular, if you have disabled Friends, Forums, and Messages, but you have enabled a third-party plugin that adds settings to this screen, it'll disappear unless that plugin author updates the plugin to filter 'bp_components_using_email_tab'. I think a safer technique is probably something like this:

  • Register the Email tab in the normal way
  • At some point after core is initialized, and after plugins have had a chance to load themselves, but *before* we render the screen, do something like this:
if ( ! has_filter( 'bp_notification_settings' ) ) {
    bp_core_remove_subnav_item( ... );
}

The trick here will be to find the proper place in the load order to make this happen. I'd suggest: as late as possible before loading the screen, maybe on 'bp_actions'.

@Mamaduka
9 years ago

Remove 'Email' sub nav, if there're no components using it.

#9 @Mamaduka
9 years ago

  • Cc georgemamadashvili@… added
  • Keywords has-patch needs-testing added; needs-patch removed

@boonebgorges followed your suggestions.

  • Patch introduces new function bp_settings_remove_email_subnav(), hooked in bp_actions which is fired before screens are loaded.
  • Also removes 'Email' from admin bar.

Tested patch with default components.

#10 @boonebgorges
9 years ago

Perfect. I've also tested with the case I mention above (no default components with notification settings, but a plugin does) and this works like a charm. Only change I'm going to make: use has_action() rather than has_filter(), since this is technically an action.

#11 @boonebgorges
9 years ago

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

In 9128:

Don't show Settings > Email tab when there's nothing to display there.

Props Mamaduka.
Fixes #5441.

Note: See TracTickets for help on using tickets.