#4802 closed defect (bug) (fixed)
Site notices to all users and bp theme compat
Reported by: | imath | Owned by: | |
---|---|---|---|
Milestone: | 1.7 | Priority: | normal |
Severity: | normal | Version: | 1.7 |
Component: | Core | Keywords: | has-patch |
Cc: |
Description
2 things :
1/ i think that in user notification settings, the settings for site notices shouldn't be printed as whatever the user choose, he will never receive an email when the admin sends a notice to all users. That's why in the attached diff, i've delete some lines in bp-messages-screens.php. Leaving it can give the admin a wrong idea about this setting as the notification settings is titled : "Email Notification".
2/ These site notices are not showing using bp theme compat, so i suggest to add some code in :
- bp-templates/bp-legacy/buddypress-functions.php
- bp-templates/bp-legacy/css/buddypress.css
- bp-messages/bp-messages-template.php
in order to try to display it. The modifications are in the diff.
Another option could be to build a sidebar widget...
Attachments (4)
Change History (20)
#2
@
12 years ago
he will never receive an email when the admin sends a notice to all users
Is this a bug? Are they *supposed* to receive an email? If so, we should fix the bug rather than removing the setting (assuming it's a regression).
Well i've just checked in 1.6.4, and i don't think not sending emails is a bug : in the bp-message-screens.php the function messages_screen_notification_settings() checks for 'notification_messages_new_notice' usermeta and this usermeta is stored in bp-settings-action.php with the function bp_settings_action_notifications(). But it's never used to eventually send an email to the user. I guess, if the host limits the number of emails that can be sent at the same time, and the community has a lot of users, it can be problematic to send a lot of emails at the same time..
Instead, when an admin adds a new notice, it is inserted in $bp->messages->table_name_notices and another usermeta seems to store an array containing the ids of the notices he has closed thanks to bp_dtheme_ajax_close_notice() in order to stop displaying it. The usermeta key is 'closed_notices'.
I think that using a sidebar widget is more elegant
I agree, but, i think it's then necessary to check if this widget is activated before displaying the checkbox to the admin in the compose screen of the bp message component.
On the other hand, i realized that the diff i posted won't display the notice at 100%. For instance if for some reason the admin is not using at least 1 widget, then the notice is not displayed..
#3
follow-up:
↓ 6
@
12 years ago
Well i've just checked in 1.6.4, and i don't think not sending emails is a bug
Thanks for looking. It sounds like the bug is, indeed, that we're showing a settings option for an action doesn't exist. So the question remains: should we fix this by removing the unused setting option, or by instating the new feature of emailing these notices out? The whole idea behind sitewide notices seems to be that the messages are *important*, so email seems like a pretty natural thing to do. But, as you note, it might be problematic to send out so many emails at once.
In either case, I think the bug has been in place long enough that it's not urgent to fix it for 1.7, especially if we're going to start adding new functionality.
it's then necessary to check if this widget is activated before displaying the checkbox to the admin in the compose screen of the bp message component.
Yeah, that's an interesting idea. Alternatively, on the compose screen, we could check to see whether there's an active Sitewide Notices widget, and if not, we show a notice: "Site Admin: Enable a Sitewide Notices widget to display your message to all users" - and then we would disable the checkbox. Of course, this would only apply to theme-compat.
is this a crazy idea? Also, is it feasible for 1.7? It seems like we should do *something*, as the Notices feature is currently pretty much broken for theme compat.
#4
@
12 years ago
Do other devs have a thought about this?
On some BP sites, I've been adding the markup for the sitewide notices in the footer and using CSS to position the notice so it is fixed and floating near the top of the page.
Might be too big of a UI change to do for 1.7, but just throwing it out there.
So the question remains: should we fix this by removing the unused setting option, or by instating the new feature of emailing these notices out?
I don't mind how we address this. Sending email in-sequence is a problem though, so if we're going to add this feature, we should make sure we have the proper filters in place to disable this feature and so devs can grab the email output and do their own thing.
#5
@
12 years ago
is this a crazy idea? Also, is it feasible for 1.7?
I think it's a very interesting idea, as the admin will be able to add the widget in the sidebar widget area of his choice.
I made some tests this morning. You'll find a new diff (4802-widget.diff) and the widget (bp-messages-widget.php) i put in "buddypress/bp-messages" folder.
It loads the widget only if the theme is not bp-default (or is not a child theme of bp-default). If the widget is not active, in the compose screen it replaces the checkbox with an invite to activate the (BuddyPress) Site Wide Notices widget.
#6
in reply to:
↑ 3
@
12 years ago
In either case, I think the bug has been in place long enough that it's not urgent to fix it for 1.7
Agree. But needs its own ticket; this ticket is about the notices not appearing. I'd prefer to remove the option. Sending an email to every user doesn't scale.
On some BP sites, I've been adding the markup for the sitewide notices in the footer
We have planned to do similar for Turtleshell.
Widget...
...is a valid idea, but needs to be in its own ticket for a future release.
If this ticket is about notices not appearing in theme compat., that's what we need to fix. The other good enhancements can come in a later release.
#7
@
12 years ago
If this ticket is about notices not appearing in theme compat., that's what we need to fix. The other good enhancements can come in a later release.
I agree, but (sorry to insist) i think the widget is really interesting to maximize the appearance of notices in theme compat. In previous versions and in this version with bp-default activated, the notices are displayed in the sidebar. Now, if for some reason, the active theme use a "no-sidebar" template to display BuddyPress content, the notices won't appear in the sidebar, and you'll need to display it elsewhere (for example footer, header).
Now if you use the Sitewide notices widget, by checking if it's active before eventually allowing administrator to activate the checkbox in the compose screen or displaying a message inviting him to activate the widget to be able to send notices, then it's easy for the administrator to change his template and add the widget to his sidebar..
#8
@
12 years ago
I'd like other dev feedback, but 1.7 has had plenty of months already for new widgets to be added. I don't think this issue is important enough to address in 1.7.
#9
@
12 years ago
I understand. And you, Boone, John, Ray and the other core contributors / developers achieved a fantastic work.
I'm sure you'll find the best answer about this. Maybe i'm part a the few using this feature, that's why i insisted about it.
#10
@
12 years ago
If this ticket is about notices not appearing in theme compat., that's what we need to fix. The other good enhancements can come in a later release.
If that is the case, then we need to hardcode the notices somewhere. The most logical place to put this is in the footer. But if we decide to do this, it should be in theme compat only.
However, what Boone and imath state about a widget and using a checkbox on the "Notices" screen is an interesting idea.
The problems with the widget option are:
- As Boone states above, a site admin would have to manually add this sitewide notice widget before it is shown. (Related: #1474) This can be looked at as both a pro and a con.
- As imath states above, this option wouldn't work if a theme does not have any registered widget areas.
- A widget would be redundant in bp-default since bp-default hardcodes the notices in the sidebar template.
#11
@
12 years ago
I don't think this issue is important enough to address in 1.7.
I disagree. While, as imath points out, there may be relatively few people using sitewide notices, it's still the case that they are completely broken for theme compat. We should do *something* for 1.7, even if it's suboptimal.
I think a widget would be neat, but there are lots of problems that have been amply spelled out above. Maybe for 1.8.
For this release, I think going with r-a-y's suggestion (hooking into the footer only when theme compat is running, and outputting the messages there - so a form of hardcoding) is probably the lightest-weight, easiest solution at this point. Before I write a patch for it, are there ane objections to this route in principle?
#12
@
12 years ago
- Keywords has-patch added; 2nd-opinion removed
(Updated) Attached patch:
- Adds the notice markup in the footer in bp-legacy
- Wraps the markup in a container div and adds a class depending if the WP Toolbar is active or not.
- For the CSS, I piggybacked off bp-legacy's default message color scheme. CSS is trimmed down from my previous patch. Feel free to tweak!
Tested in TwentyTen, TwentyEleven and TwentyTwelve.
We still need to make a decision on the sitewide notice notification setting; I think the consensus was to remove that option?
#13
@
12 years ago
I think that the patch looks good, r-a-y. I'm going to go ahead and commit it.
We still need to make a decision on the sitewide notice notification setting; I think the consensus was to remove that option?
Yes, I think that sounds right. I'll take care of that too.
Is this a bug? Are they *supposed* to receive an email? If so, we should fix the bug rather than removing the setting (assuming it's a regression).
I think that using a sidebar widget is more elegant. But, on the other hand, moving it to a widget makes it *optional* to display sitewide notices, which is not how it works in bp-default. The change could be confusing for new BP installations. Do other devs have a thought about this?