Page MenuHomePhabricator

Implement Action API for equivalent of checkboxes for notification types
Closed, ResolvedPublic

Description

Ensure that Echo notification preferences can be configured via the Action API so that app users need not do so via the web interface. If this isn't currently the case, create a module for it.

See https://en.wikipedia.org/wiki/Special:Preferences#mw-prefsection-echo for the prefs in question.

Event Timeline

@JMinor would you please confirm if for v1 it's only the checkboxes under "Notify me about these events" that ought to be configurable by apps clients?

Are the other user configurable settings (email related dropdowns, cross-wiki notifications checkbox, muted users text input box) shown on the web intended to become user configurable in the apps in v1 or some subsequent release or is that out of scope until further notice?

DB574C2A-AE14-4362-924B-3AAC967236AF.png (2×1 px, 795 KB)

Yeah, the intention was just to show the check boxes, so I think the existing description/request is accurate ("equivalent of checkboxes for notification types").

I'd say the other notifications preferences are not required for v1. We'd likely want cross-wiki toggle and mute list eventually, so I'd call those nice to haves. Email preferences are a definite "not needed". Lets confirm this a the next working group meeting, but I think that's where we'll land.

Users' notification preferences are stored along with all other user preferences in the user_properties table. The preference keys follow the form echo-subscriptions-<notifierType>-<eventCategory> and the possible values are 1 (true) and 0 (false). It appears that a user preference is only created when a user overrides the wiki default settings for the event category and notifier type.

So, for example, if an English Wikipedia user elects to enable notifications by email when thanked by another user for an edit (notification category edit-thank), a row in the user_properties table will be created for the user with the property echo-subscriptions-email-edit-thank and value 1.

Like all user preferences, notification preferences can be managed via the MediaWiki API using action=options.

Here's the tricky part. These preferences are site-specific in MediaWiki, but I assume that we want the push notification preferences set by users in the apps to be global across sites. Is that correct?

Hmm, maybe not so tricky: there's a newish extension, GlobalPreferences, that's intended to (just like the name says) manage this kind of user preference globally across wikis.

Hi @Catrope, I'm wondering if you can help me out with some difficulty I'm having with global preferences. The apps would like to set one global preference per notification type (e.g,, edit-thank) for all wikis, and it seems like that should be something they can do via the GlobalPreferences API. But I'm testing on the Beta Cluster, and it doesn't seem like my global preferences are being honored; I set echo-subscriptions-push-edit-user-talk="1" via the GlobalPreferences API for my user Mdholloway, for example, but I am not receving push notifications for user talk page edits. Does EchoAttributeManager::getUserEnabledEvents only consider local user options and not global preferences, or am I confused or doing something wrong?

Okay, I haven't verified in the debugger but I think I see what's going on here. Global echo-subscriptions-* preferences are not honored unless the echo-subscriptions global preference flag (corresponding to the "Select options below to be global" checkbox immediately under "Notify me about these events" in the global preferences web UI) is also true.

In Hooks::onUserLoadOptions():

// FIXME: temporary plug for T201340: DB might have rows for deglobalized
// Echo notifications. Don't allow these through if the main checkbox is not checked.
if ( !( $globalPrefs['echo-subscriptions'] ?? false )
	&& strpos( $optName, 'echo-subscriptions-' ) === 0
) {
	continue;
}

https://github.com/wikimedia/mediawiki-extensions-GlobalPreferences/blob/master/includes/Hooks.php#L65-L71

This makes sense from the perspective of the web UI. It makes life more difficult for us, because I don't think we'll want to silently enable global notifications preferences for app users given that the setting also covers web and email notification preferences.

Hi @Samwilson, Moriel suggested that you'd be the person to come to with questions about GlobalPreferences. We'd like to use the GlobalPreferences API to allow the mobile apps to set global preferences for pushed Echo notifications by notification type. I'm wondering if you can help me understand precisely what's going on in the lines I mentioned above, in T251462#6225459. Reading a little bit between the lines, it looks like there's a tacit assumption here connected to the web UI, namely that having global preferences set without the setting corresponding to their web UI checkbox to make the setting global is a thing that Shouldn't Happen (despite that the extension API will permit it to happen). Is that a correct reading of what's going on here? Would it cause trouble if I were to add a rule to the conditional checks to allow global preferences beginning with echo-subscriptions-push- to pass through even if echo-subscriptions isn't true? Thanks!

I think this problem came about because we didn't want to add a separate 'globalize this?' checkbox for every notification checkbox (i.e. every row and column of the notification grid), and so GlobalPreferences has to rely on the presence of the main checkbox (and saves the echo-subscriptions global pref even though there's no matching local pref by that name). Then, it's only when that's there that we set global notification preferences.

Would it cause trouble if I were to add a rule to the conditional checks to allow global preferences beginning with echo-subscriptions-push- to pass through even if echo-subscriptions isn't true?

I don't think it makes sense to do any override just for echo-subscriptions-push-*, because it doesn't look like those will be particularly any different from other notification preferences. If a user doesn't have global notification preferences, it seems wrong to return a global value. Possibly the correct fix here is to make sure echo-subscriptions is set everywhere that sets global notification preferences (or is this already done, and I'm misreading this?).

Thanks, Sam. I think I understand the extension's internal logic now. Having all notification settings be either global or not as a group does pretty much rule out what I was thinking of doing.

That being the case, I checked in with Joe Walsh, the apps engineering manager, about it, and it sounds tentatively like it's fine to have the apps set these preferences on a per-wiki basis via the Action API (action=options), rather than setting global preferences. That being the case, I think this task can be closed as resolved, pending final confirmation with @JMinor when he's back from vacation next week.

What I ultimately had in mind here if we did need global notifications preferences for the apps would be a kind of shadow notifier type in Echo specifically for apps, the preferences for which would be suppressed from the web UI and always be global. But that would involve updating (or at least hacking around) some assumptions in both Echo and GlobalPreferences to accommodate such a thing, which I'd rather avoid doing if possible.

@JMinor I propose for v1 that we launch with a single "Apps" preference column controlling whether a push notification is sent to any and all apps for which the user has registered a push subscription. As mentioned above, these preferences can also be set/unset in the apps via the Action API action=options module. Sound good?

Change 607622 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/Echo@master] Change push notification preference column label to "App"

https://gerrit.wikimedia.org/r/607622

Change 607622 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Change push notification preference column label to "Apps"

https://gerrit.wikimedia.org/r/607622

@Mholloway that sounds good. Apps as a single column/label makes sense for what we have now.