Bulk Notifications Management

[colabsadmin]

I've been working on plugin to bulk manage the notifications list. Maybe you'd consider making part of BuddyPress.

The code adds a dropdown to filter the notifications by type and will add checkboxes next to each notification so you can do bulk actions (read/unread or delete).

For the dropdown filter, I’m using bp_notifications_get_registered_components() to get a list of all possible notification components. The issue is that some of the names aren’t very pretty. Is there another function I can tap into for this?

All of the code is here: ​https://github.com/colabsadmin/BuddyPress-Manage-Notifications

[boonebgorges]

Wow, this is really excellent. If you don't mind rewriting this as a BP patch, I think we can definitely look at including it for 2.1. A couple comments:

• For sort, please use a different parameter from 's'. This is used by WP and many other plugins for 'search'. Maybe something like 'notification_type'?
• For the "not very pretty" component names, you could do something like: buddypress()->{$component}->name. We could build a wrapper for this if you think it'd be better. Or if you think that notification types should register their own names, it might be an even more elegant system.
• Please use our API-level functions for processing bulk actions, rather than direct SQL queries. (Especially because you aren't sanitizing the input correctly :) ). Check out ​https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-notifications/bp-notifications-functions.php#L254, and if you don't see the functions you need, we can either write them, or you can use our DB class ​https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-notifications/bp-notifications-classes.php#L628

Thanks for your work on this so far!

[colabsadmin]

Sweet. I'll take a look at all of your suggestions. I'm extremely new to wordpress/buddypress, so not clear on the best way to handle things. Thank you for the input.

[DJPaul]

Hey, colabsadmin, are you still out there? It looks like we've missed the boat for 2.1 for this, but we'd still love to get it in for 2.2 if you're interested in helping us update the patch.

[jgwolfensberger]

I would support plug-in development with a donation if anyone can take colabsadmin's concept to functional plugin. I'm at jgw [at] newsavvyproductions [dot] com

[lakrisgubben]

Hi y'all.

Since there hasn't been any action on this ticket (or the github repo mentioned above) for quite a while I took the liberty to try and whip up a patch. It's based on @colabsadmin plugin, but I changed some parts to use BP's API for deleting/marking as read etc. I also made some adjustments to the code to more closely try and follow how other parts of bp-core are written, mainly inspired by the "delete all selected messages" part. I also didn't include the option to sort notifications by type, even though that might be useful, since I wanted to limit the scope of this patch to what I think is most important, i.e. bulk deletion/marking as read.

This is my first BP patch (and also, svn, only used git before) so please bear with me if some things aren't perfect. And I'd be happy to change things around if you'd like. :)

[boonebgorges]

lakrisgubben - Thanks so much for working up this patch! Looking pretty good. I have a few suggestions:

• Move the checkbox column to the left, rather than the right. This is more consistent with other bulk action stuff in WP/BP.
• When marking all items read/unread, the AJAX request removes them from the list, leaving an empty list. When refreshing the page, you see a "You have no (un)read notifications" message instead of the table. This is a bit jarring - I wonder if we can dynamically remove the table and show the "You have no..." message when all items are removed. Could you take a look to see how hard this is, given the way our templates currently work?
• Similarly, removing items from the list dynamically messes with pagination. (Try appending ?num=2 to the URL to see what I mean.) The solution here might be the same as above: namely, the AJAX request should return an entire template. Have a look at how this is done in group membership requests.
• If the two previous points are too hard to solve, maybe we can think about scrapping AJAX support for now, and just doing this all with a page refresh, in which case the problems go away.
• Related to the previous point: it looks like this won't work with JS enabled. With this kind of form, enabling no-JS support is very easy, so let's do it. When JS is disabled, let's make sure the Select All checkbox doesn't show, since it won't do anything.
• Small things:
  • Function names should be more specific. bp_notifications_bulk_management should say something about dropdown etc (see if there's another function in BP whose function name you can copy). bp_legacy_theme_ajax_notifications_bulk_management() should be a verb rather than a noun; see the other AJAX handlers.
  • Let's use foreach() rather than for() in bp_legacy_theme_ajax_notifications_bulk_managaement().
  • The 'do_action' param in the AJAX request is confusingly named. do_action has a particular connotation in WP-land :)

Thanks again for your work on this! BTW, feel free to do your work and patching in Git if it's more comfortable - we have a mirror at ​https://buddypress.git.wordpress.org, and we can handle git-formatted patches without a problem (or run git diff --no-prefix).

[lakrisgubben]

Replying to boonebgorges:

Thanks for the quick feedback Boone! :) Most of the stuff are no-brainers and I'll fix them in the next patch. Just wanted to ask some stuff about the things I'm not totally confident about...

I agree with you about the problems with the current js-only approach, and after thinking about it a bit more I think that a page reload is the better approach, since it stays in line with how marking a single notification as read/unread works. It also gets rid of the problems with pagination etc (even though they of course could be solved in other ways..).

You mention that "With this kind of form, enabling no-JS support is very easy, so let's do it." But I'm not totally sure (and maybe I'm blind, but can't find any good examples in bp-core) of how you usually do this. How you handle this kind of stuff when marking a single notification looks good, but how would I be able to pass the checked notifications to the form/get-params without js? The first solution that spring to mind is of course putting all of the checkboxes into the same form as where the submit button is. But that would mean wrapping the whole table in a form, and seems kind of not like how you usually do it.

I think I just need a pointer in the right direction here. :) My initial thought was to use the messages list as a starting point, but the bulk actions there only work with js enabled... :/

[boonebgorges]

The first solution that spring to mind is of course putting all of the checkboxes into the same form as where the submit button is. But that would mean wrapping the whole table in a form, and seems kind of not like how you usually do it.

This is the way to do it :) It looks like we *don't* do this in the case of messages (which is the other obvious place where we do bulk management), but we should. I'd build it this way first, and hold off on the AJAX for the time being.

[lakrisgubben]

Replying to boonebgorges:

This is the way to do it :) It looks like we *don't* do this in the case of messages (which is the other obvious place where we do bulk management), but we should. I'd build it this way first, and hold off on the AJAX for the time being.

Cool, just wanted to be sure that I didn't break bp-protocoll. :) If this patch gets accepted sometime maybe the next mission would be to fix this for messages as well.

The new patch puts the checkboxes to the left, (hopefully) has better function names, gets rid of most of the js (only saving the part for selecting all), hides the select all if body.no-js and only uses page refresh to post notification ids to bulk manage.

If you're wondering about

#buddypress #notifications-bulk-management {
	clear: left;
}

it's because firefox flipped out if the form wasn't cleared and put the table to the right of the whole content area. :/

btw. thanks for a great talk at WCSF! :)

[boonebgorges]

btw. thanks for a great talk at WCSF! :)

Thanks :)

If this patch gets accepted sometime maybe the next mission would be to fix this for messages as well.

+1. It should look just like this (both visually and in terms of code). It's a bit more complex there because of backward compatibility. Please feel free to open another ticket.

I'm going to make a few minor cleanups and commit this one. If you or someone else feels like doing the necessary work to make this all work with AJAX, let's handle it in another ticket. Thanks very much for this contribution!

[boonebgorges]

In 9132:

Add bulk management for notifications.

Props colabsadmin, lakrisgubben.
Fixes #5513.

[hnla]

@lakrisgubben great patch

If you're wondering about #buddypress #notifications-bulk-management {clear: left;} it's because firefox flipped out if the form wasn't cleared and put the table to the right of the whole content area. :/

The issue is actually that the BP pagination element sitting in the flow before the table, and now the form is floated as are it's children, not really sure why it was floated as it probably ought not to be really, suspect it was to contain those floated children but overflow or a clearfix would have been more appropriate, however your 'clear' should be fine.

As refinement it might be nice to have a little js snippet to trap the 'bulk action' option from being passed - shame really that there still is no definitive means to adding an option as a label that works cleanly.

Alternatively we could opt(semi pun intended) for using the optgroup element to carry the string 'Bulk action'

We should though add a label element for the select, it's mandatory really and we can hide using one of those assistive text classes or add one to hide the label text.

Ticket #6005 is related, and looking at the messages screens in a similar vein to notifications.

[boonebgorges]

We should though add a label element for the select, it's mandatory really and we can hide using one of those assistive text classes or add one to hide the label text.

Yes. I did this for "select-all-notifications" but forgot it for the select box. Reopening in order to fix it.

[hnla]

Lets add a bp class for hidding label text that marks it as 'Assistive' in same manner found in WP default themes for our labels and where a theme might not provide for:

The webaim recommendation

.bp-screen-reader-text {
 position:absolute;
 left: -10000px;
 top: auto;
 width: 1px;
 height: 1px;
 overflow: hidden;
}

or the 'clip' approach used by 2014

.bp-screen-reader-text {
 clip: rect(1px, 1px, 1px, 1px);
 position: absolute;
}

[boonebgorges]

+1 to whatever you recommend. Make it so.

[hnla]

I'll have a test of best methods and open a separate ticket for the addition.

[lakrisgubben]

The new patch adds a label for the bulk actions as well a class of .bp-screen-reader-text to both the labels (for the dropdown and the select all). It doesn't add any css so these will be visible pending a patch from @hnla. It removes the css that hid the select all label, and introduces a basic js-check so that a bulk action is chosen before submitting the form. Maybe you'd like some kind of error message here as well? If so, do we have these kind of error messages somewhere else so I can reuse the html?

[boonebgorges]

introduces a basic js-check so that a bulk action is chosen before submitting the form. Maybe you'd like some kind of error message here as well?

What happens in Dashboard > Posts when you submit Bulk Actions without selecting anything? I think we can probably do the same thing.

[lakrisgubben]

Replying to boonebgorges:

What happens in Dashboard > Posts when you submit Bulk Actions without selecting anything? I think we can probably do the same thing.

Nothing happens. But there's a page refresh going on there, the thing here is that nothing even happens when you click the "apply" button since we hijack the submit with js. Which might lead the user to think that something's wrong... So either we skip the hijacking and just do a page refresh without anything changing (the current no-js approach), or we should probably tell the user that something needs to be changed in order to submit.

[boonebgorges]

Ah right, AJAX. I think just disabling the 'Apply' button is probably enough.

[lakrisgubben]

that's more or less what this patch do I'd say. :) But do you want me to also set the disabled attribute on it. Can do that, even though it requires some more js since then we'll also have to watch for changes on the bulk action select to remove the disabled attribute once it's changed..

[boonebgorges]

Yeah, I think it's worth the extra code to set 'disabled'. That will give some visual indication that you have to select something to make it work. With your patch, I can imagine someone being confused about why the click does nothing.

[lakrisgubben]

Replying to boonebgorges:

Yeah, I think it's worth the extra code to set 'disabled'. That will give some visual indication that you have to select something to make it work. With your patch, I can imagine someone being confused about why the click does nothing.

Latest patch implements this.

[hnla]

It was my thinking too to use JS to disable the submit if option value equalled 'Bulk Action', only enabling the submit when an option selection did not equal 'Bulk action'. As for users awareness could we maybe not change 'Bulk Action' for 'Select Bulk Action'?

In fact think it might make sense to be literal with label human readable text and state 'Select Bulk Action' after all this is what is being asked of the user where bulk Action is simply a statement with no intent?

[lakrisgubben]

Replying to hnla:

Sure, I'll fix the copy to say Select, agree that the more literal approach is better. :)

Regarding the js, not totally sure if I get you. Do you mean to disable the submit button already on document.ready or do you just mean that the <option>Select Bulk Action</option> actually should contain a value of 'Select Bulk Action' that I should check against instead of how it's in the latest patch where that options value is just empty and I check for that?

[hnla]

@lakrisgubben

looking at that JS was wondering whether we shouldn't just default the submit to disabled then remove on change of select value and back again if selection reverted then style on the attr 'disabled to remove background, cursor, opacity etc; so something like:

	/* Make sure a 'Bulk Action' is choosed before submiting the form */
	jq('#notification-bulk-manage').attr('disabled', 'disabled');
			

	/* Remove the disabled attribute from the form submit button when bulk action has a value */
	jq('#notification-select').on('change', function(){
	 if ( jq(this).val() !== '' ) {
	  jq('#notification-bulk-manage').removeAttr('disabled');
	 }else {
	  jq('#notification-bulk-manage').attr('disabled', 'disabled');
	 }
  });

Something like that with perhaps a more elegant or stronger approach using ? .toggle

[hnla]

so just to be even more annoying I might do this:

	/* Make sure a 'Bulk Action' is choosed before submiting the form */
	jq('#notification-bulk-manage').attr('disabled', 'disabled').attr('title', 'Please select a bulk action');
			
	/* Remove the disabled attribute from the form submit button when bulk action has a value */
	jq('#notification-select').on('change', function(){
		if ( jq(this).val() !== '' ) {
			jq('#notification-bulk-manage').removeAttr('disabled');
			jq('#notification-bulk-manage').attr('title', 'Submit bulk action');
		}else {
			jq('#notification-bulk-manage').attr('disabled', 'disabled');
			jq('#notification-bulk-manage').attr('title', 'Please select a bulk action');
		}
	});

and have added a ruleset to style buddypress form elements that have attr 'disabled' for cursor 'not-allowed', and opacity: .5; just to make it clear the submit is disabled.

[lakrisgubben]

Replying to hnla:

Oups, uploaded a patch before I saw this. :/ But anyways, new patch reuses the same css used for the .disabled class for input type="submit" for attr=disabled. And is a bit cleaner js-wise...

If we start tampering with strings in the js don't we have to make sure they're output in the BP_DTheme js-object so that they're translatable as well?

[boonebgorges]

28 seems like overkill. There aren't other places in BP (or WP?) where we change button text like this. I think we can come up with something more subtle. (And yes, we would need to localize the strings for JS.)

I would vote against 'Submit Bulk Action' (definitely not plural) and in favor of 'Bulk Action'. Better parity with WP, shorter, and clear enough. If it's *not* clear enough, let's make the text of that option gray, or set it off like '- Bulk Action -' to make it clear it's not a valid selection. But really, think about it: do you think anyone will really be confused by this in the first place? Let's not overdesign.

[hnla]

I'm uploading css that styles attr 'disabled' and adding the screen reader class plus grouping some of the display none rulesets to save a few bytes.

Not too fussed on the text for bulk action just thought it better that it was clear as to the intended user action required.

[hnla]

Add screen reader class & style form elements with attr 'disabled'

[hnla]

If we start tampering with strings in the js don't we have to make sure they're output in the BP_DTheme js-object so that they're translatable as well?

Yep guess so, being lazy, but it's kind of neat to change the title text after all it's what the title attr is for and lends accessibility points! :) tbh I wasn't sure how we do make those strings translatable.

[lakrisgubben]

Replying to hnla:

I'm uploading css that styles attr 'disabled' and adding the screen reader class plus grouping some of the display none rulesets to save a few bytes.

+1 for the grouping. But I still think that using the styles on rows 865-893 for #buddypress form input[type=submit][disabled="disabled"] would be more consistent, since those styles are already applied to disabled buttons, just that they're not targeting buttons that are disabled, only those with the class disabled. We could of course still use the generic #buddypress form *[disabled="disabled"] styling from your patch. Or what do you think?

And, btw, I'll get rid of the 's' at the end of 'Select Bulk Actions' in the next patch as well .:)

[hnla]

Not in disagreement - the generic ruleset was intended to pick up on any disabled attr so that elements didn't necessarily need a class or if a class was overlooked we would pick up on and style, so if we could keep that approach it wouldn't be a bad thing - the styles on that were simply to provide a visual clue to an element being disabled so adding opacity to 'grey' the element out and adjust it's cursor ( I accept that the cursor may not be to everyone's taste)

We need one review just to establish whether the grouped selectors on all those various possible button/element classes are indeed required (problem with them is it's easy to lose track of whether they are needed or just double up)

As for the string 'Select Bulk Action' did Boone approve of that or feel it was unnecessary we'll be led by him on that one and for consistency with other labels/strings.

I think we're good to go if we just review the css changes and ensure those disableds are covering things correctly.

I liked my title atr approach though ;)

[lakrisgubben]

Replying to hnla:

Not in disagreement - the generic ruleset was intended to pick up on any disabled attr so that elements didn't necessarily need a class or if a class was overlooked we would pick up on and style, so if we could keep that approach it wouldn't be a bad thing - the styles on that were simply to provide a visual clue to an element being disabled so adding opacity to 'grey' the element out and adjust it's cursor ( I accept that the cursor may not be to everyone's taste)

I think we should keep the generic disabled=disabled styling, only question is if inputs that look like buttons should also get the same styling as .disabled buttons. Though I must say I think the cursor: not-allowed is a bit much for my taste. I'd vote for cursor: default.

We need one review just to establish whether the grouped selectors on all those various possible button/element classes are indeed required (problem with them is it's easy to lose track of whether they are needed or just double up)

After a quick search through trunk I can't actually find any place where we use either the .pending or the .disabled class except for in the css as well as one check in the js to disable stuff with .pending. But this might be good to have some second eyes on. :)

As for the string 'Select Bulk Action' did Boone approve of that or feel it was unnecessary we'll be led by him on that one and for consistency with other labels/strings.

That was how I interpreted it, that he just didn't like the plural...

[hnla]

Though I must say I think the cursor: not-allowed is a bit much for my taste. I'd vote for cursor: default.

:) Thought this would be case, yes I was in two minds about it. Lets use 'default' as value.

I can't actually find any place where we use either the .pending or the .disabled class except for in the css as well as one check in the js to disable stuff with .pending.

This was somewhat my puzzlement here, wasn't sure but had a feeling those .disabled & .pending were unused, in my time I couldn't recall why we had these supposed classes in the sheet and sadly when this occurs it does mean having to spend a bit of time tracking things down. To be honest I think the .disabled/.pending rulesets are spurious ones and that those properties are never being applied thus are actually redundant and ought to be removed, but don't want to guess at that we need to determine properly, however as they exist and as determining does increase the time factor in completion somewhat lets leave those added selectors in place we can always delete the whole ruleset/s if necessary at some later juncture.

As for the string 'Select Bulk Action' did Boone approve of that or feel it was unnecessary we'll be led by him on that one and for consistency with other labels/strings.

That was how I interpreted it, that he just didn't like the plural...

Reading the comment back Boone was in favour of not over designing on this and favoured 'Bulk Action'

I stand by the point that the form control is asking for a user action and it should be saying what action is required 'select blah blah' so as a compromise lets keep the option string as 'Bulk Action' but the label as 'Select Bulk Action' thus we read the instruction in the label as select your option anf the option reads as a heading although to be perfectly honest I don't like options used this way optgroup would be more suitable or nothing at all and disable the submit based on no checkboxes having been selected or if multiple checkboxes not selected.

@Boone could we have you take ( and final judgement ) on the label point: True form label as 'Select Bulk Action', option string as 'Bulk Action' or both as 'Bulk Action'?

[lakrisgubben]

lets keep the option string as 'Bulk Action' but the label as 'Select Bulk Action' thus we read the instruction in the label as select your option anf the option reads as a heading although to be perfectly honest I don't like options used this way optgroup would be more suitable or nothing at all

Agreed that it's not semantically beautiful to use an option with an empty value as a 'placeholder'. Problem with optgroup is that it's not the one showed by default, so even if we add that it would still be 'mark read' that was shown.

[hnla]

optgroup is that it's not the one showed by default,

No I know, it's not really intended as a 'placeholder' but like the fieldset to group options for clarity, I'm afraid it's just a habit that's sprung up to use an option element as placeholder text when really selects are/were never intended to have or need that -but this aspect is a digression and I mustn't derail things harping on about it :)

[boonebgorges]

lets keep the option string as 'Bulk Action' but the label as 'Select Bulk Action'

This sounds good to me.

Agreed that it's not semantically beautiful to use an option with an empty value as a 'placeholder'.

I also agree, but this is something we do in many places throughout BP and WP. I'm all for finding a better solution, but it's beyond the scope of this ticket.

[lakrisgubben]

latest patch changes option string to 'Bulk Action' and merges the css-patch from @hnla (​https://buddypress.trac.wordpress.org/attachment/ticket/5513/5513-css-classes.patch). Those where the two things that needed to be done as far as I can tell. Or is there anything else I should fix? :)

[hnla]

I think that's everything covered looking through the patch.

[boonebgorges]

This looks purty. Thanks, gang.

[boonebgorges]

In 9143:

Improve labeling and validation behavior of notifications Bulk Action dropdown.

Props lakrisgubben, hnla.
Fixes #5513.

[boonebgorges]

Side note: the screen-reader-text treatment here is very nice. If anyone wanted to do an audit of our existing selectors and their labels, to make sure that we're doing something equally nice in cases where we don't display a visible label, that would be outstanding (and a subject for another ticket). Thanks again to all.

[hnla]

If anyone wanted to do an audit of our existing selectors and their labels

I'll open a ticket as a reminder todo.

[DJPaul]

Updated RTL CSS in r9147. grunt build-commit ought to be run before committing. :)