Page MenuHomePhabricator

Create a way to save preference options wherever they are set by GlobalPreferences without disruption
Closed, ResolvedPublic5 Estimate Story Points

Description

This came up as a result of working on T218265: Mute: Add links to disable email and mute specific user to emails sent via Special:EmailUser but a generalized solution can help other products that need to update preferences in similar ways.

The problem

GlobalPreferences can only be saved from the Special:GlobalPreferences page. While the extension intercepts all option saves from the onUserSaveOptions hook, it only updates the global preferences specifically, from the Special:GlobalPreferences page. This was an intentional product decision when the extension was created.

The main idea is the GlobalPreferences allows for setting/updating the global preferences specifically through an API, and the decision on whether a preference should be updated locally or globally should be done by the product, and not as an overarching decision by the GlobalPreferences extension.

This works well for extensions and gadgets, both of which can directly call the GlobalPreferences-specific API, but it has some flaws:

  • It does not work for any product in core, since core does not know about GlobalPreferences extension. Any code-driven preference change in core, therefore, is only updating local preferences, even if that local preference is overridden by core, or has a local override by GlobalPreferences itself.
  • Products that want to update a preference regardless of where it is saved (whether it is global, local, or locally overridden by the user) must do these checks themselves if they want to. Those checks are doable, but add complexity to the code that is repetitive and since each product needs to do that for themselves, it can quickly get non standard.

Proposed solution

There should be a way to tell GlobalPreferences that certain preference options should be saved based on its own logic, wherever they are enabled.

The code that tells core to save preference should not have to deal with whether GlobalPreferences exists, is enabled, and then whether the specific option's state is globalized, local, or a local override. Code in products should "just" save the option and GlobalPreferences should handle this logic. However, products should decide whether this is wanted or not, since there are multiple cases (especially in core) where options that are saved in the code are meant to be local (for example, RecentChangesFilters' saved queries).

Technical implementation

To accomplish this, we can implement the following:

  • Create a wg variable that stores an array of local option names that should be handled by GlobalPreferences. If a product wants GlobalPreferences to decide where the best to save it, the product should add their preference names to this global variables in config.
  • In the onUserSaveOptions code, the GlobalPreferences extension will evaluate the options given and compare each to the wg variable.
  • If any of the given options is in the wg variable, GlobalPreferences will handle it itself (and take it out of the options array when giving it back through the hook)
  • For each option that GlobalPreferences needs to handle, it will do the following:
    • If this option is local for the user (the user does not have that preference enabled in Special:GlobalPreferences) then the value will be saved locally.
    • If this option is globalized (the user enabled the preference in Special:GlobalPreferences) then the value will be stored in the global preference.
    • If this option is globalized, but has a local override (the user has the option enabled in Special:GlobalPreferences, but in the specific wiki also enabled a local override) then the value will be saved in the override.

This will mean, essentially, that for all preference values given to GlobalPreferences to handle, it will save the given value where it is actually set at the moment of saving, and will result in the least overriding of other values as possible.

This also means that if a preference was changed through some code through the method above, and the user, after that, enabled/disabled or changed the state of what preference is "on" (enabled or disabled global preference or override, etc) then the value will change from the value that the code has set. That, however ,will be specifically done by the user and in either Special:Preferences or Special:GlobalPreferences where they can see what is active and what is overridden.

Details

Related Gerrit Patches:
mediawiki/extensions/GlobalPreferences : masterSupport for autoglobal preferences

Event Timeline

Mooeypoo created this task.May 23 2019, 7:55 PM
Restricted Application added a project: Community-Tech. · View Herald TranscriptMay 23 2019, 7:55 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Pinging @Niharika and @MaxSem.

We discussed this for the other ticket (linked) but it was fairly generally. I summed up the idea here. Please see if it makes sense, and @Niharika please approve (or comment on changing?) if you're approving the change to the product behavior.

With this, the behavior will only be different to specific options that are picked, while for everything else status quo is the same.

One question is also where this wg variable "lives"; GlobalPreferences shouldn't really have a list of options in it, so we can put this in Config, but we will have to make sure that all given options are global (not per wiki). Could use your input on this, @MaxSem

dbarratt added a subscriber: dbarratt.EditedMay 24 2019, 2:16 AM

@Mooeypoo

What if, instead of adding a global config, we create a new hook for GlobalPreferences (or anything else) to listen to? Something like UserSettingsPreSaveAlter? That way GlobalPreferences can do it's thing without core being aware of it. We could add this to a convenience method in User, something like this:

public function saveModifiedSettings() {
    $this->loadOptions();
    Hooks::run( 'UserSettingsPreSaveAlter', [ &$this->mOptions ] );
    $this->saveSettings();
}

This way our code can call this method, rather than saveSettings(). This lets the code decide if it would like to opt-into the new behavior (handling the global prefs) or continue to use the old behavior.

What do you think?

Restricted Application added a subscriber: MGChecker. · View Herald TranscriptMay 24 2019, 2:18 AM

GlobalPreferences is listening to the UserSaveOptions hook, which hands over the $options array that can then be handled. See https://github.com/wikimedia/mediawiki-extensions-GlobalPreferences/blob/master/includes/Hooks.php#L91

Right now, it's just not doing anything with any preference unless the save came from Special:GlobalPreferences. The idea above is to expand the same hook and give it a way to know which options to make the process work for.

dbarratt added a comment.EditedMay 24 2019, 2:22 AM

GlobalPreferences is listening to the UserSaveOptions hook, which hands over the $options array that can then be handled. See https://github.com/wikimedia/mediawiki-extensions-GlobalPreferences/blob/master/includes/Hooks.php#L91
Right now, it's just not doing anything with any preference unless the save came from Special:GlobalPreferences. The idea above is to expand the same hook and give it a way to know which options to make the process work for.

I understand the solution, I'm saying the simpler solution might be to add another hook / method. The new hook would do something with all the options (just like if the form was being saved). This way, code can save the settings locally (unmodified) or globally (modified).

Mooeypoo added a comment.EditedMay 24 2019, 2:26 AM

I understand the solution, I'm saying the simpler solution might be to just add another hook / method. The new hook would do something with *all* the options (just like if the form was being saved). This way, code can save the settings locally (unmodified) or globally (modified).

We could do that, but there are a few problems with this since core isn't supposed to know about other extensions. If we create this in core, it's redundant (saveOptions does the same thing) and the uses of it in other places will be inconsistent. Why would something use this instead of the regular call? What if GlobalPreferences isn't installed or enabled? etc.

On top of that, I think it makes more sense to bring the logic "upwards" to GlobalPreferences. If GlobalPreferences exists, it does what it needs to do. If it doesn't, the system needs to be agnostic to it. The only place where we want to connect core to GlobalPreferences is our production environment (or, for that matter, a 3rd party production environment). Any other case, core -- and any other extension -- should "just" be doing their saving regardless of whether GlobalPreferences exists or not.

Am I misunderstanding your proposal? What does the extra call give us as on top of the regular existing call that evokes the UserSaveOptions hook?

dbarratt added a comment.EditedMay 24 2019, 2:36 AM

The problem is, is if you make a whitelist and put the email-blacklist preference on it, that specific preference is going to have a different behavior in the Action API than all of the other ones. You've changed the behavior of anything using User::saveSettings() with that preference (which includes things like the Action API).

In other words, where the preference is being saved from is more important than the preferences themselves.

Eh, yes, good point about the API. We can find a solution for that, but to be fair, that might well be acceptable behavior. The API module is split between local and global preferences, since the users of the API don't have core's problem of not having to know about the existence of GlobalPreferences. The explicit product choice (until now at least) was that whoever wants to change the preferences through the API needs to explicitly choose either local API or global.

That said, I don't think it *should* be acceptable behavior. It should be consistently doing the same thing between the API and local saveSettings.

I'm really not enamored with the idea of creating another method. The saveOptions method was intended to allow extensions and anything else to manipulate the options and even stop the saving process (see the comment and code at https://github.com/wikimedia/mediawiki-extensions-GlobalPreferences/blob/master/includes/Hooks.php#L91). The purpose of that method is exactly what we want -- we just end up not doing anything with what it gives us...

Perhaps we are over complicating this. What if we added a SpecialMuteSave hook and allowed GlobalPreferences to modify the preferences used on that page?

dmaza added a subscriber: dmaza.May 24 2019, 5:03 PM

I think I already asked this but it escapes me if I did. Why was it that we can't check if a given option is already a global preference or a local exception?
In other words, when saving "whatever-option" on onUserSaveOptions we could determine if it is a global preference and/or if it has a local override and make the changes wherever they need to be done.
Considering that the choice to make something a global or a local exception is made by the user I would say that it will be expected that any changes to those options will be stored based on the user's choice.

I think I already asked this but it escapes me if I did. Why was it that we can't check if a given option is already a global preference or a local exception?
In other words, when saving "whatever-option" on onUserSaveOptions we could determine if it is a global preference and/or if it has a local override and make the changes wherever they need to be done.

Because that would make core aware of the extension, which we can't do.

dmaza added a comment.May 24 2019, 5:18 PM

@dbarratt I meant to check on GlobalPreferences not on core. Same thing @Mooeypoo is suggesting but checking against what we already have on the db instead of a config

@dbarratt I meant to check on GlobalPreferences not on core. Same thing @Mooeypoo is suggesting but checking against what we already have on the db instead of a config

Because when GlobalPreferences was made, they made the decision (for whatever reason) not to do that. Perhaps it would be good to understand those reasons?

aezell added a subscriber: aezell.May 24 2019, 6:06 PM

I wonder if @MaxSem might be able to give us some of the context for why it was built that way?

MaxSem added a comment.EditedMay 30 2019, 10:17 PM

We've discussed it in the office and decided the following (mostly @Mooeypoo's plan):

  • Certain settings are whitelisted as transparently settable. Instead of having a list, I personally would rather have a flag in the preferences array, even if that means that we'll have to put it there with a hook for core settings.
  • When such a setting is getting modified, GlobalPreferences intercepts this and, if the option is global and not overridden locally, a global value is set instead. Modifying a locally overridden value transparently should already work as expected (but we need to verify this regardless).
Mooeypoo added a comment.EditedMay 30 2019, 10:32 PM
  • When such a setting is getting modified, GlobalPreferences intercepts this and, if the option is global and not overridden locally, a local value is set instead. Modifying a locally overridden value transparently should already work as expected (but we need to verify this regardless).

I think I'm misunderstanding it here, what do you mean "if the option is global and not overridden locally, a local value is set instead" ? think I'm not completely understanding that part.

Here's what I thought we agreed to (correct me if I'm wrong) --

If the setting is whitelisted (in however manner we do that, based on your first bulletpoint) GlobalPreferences will check and act according to:

  • If, for the user, the setting is not global -> save in local
  • If, for the user, the setting is global -> save in global
  • If, for the user, the setting is global *but* uses local override -> save in local override.

Basically, GlobalPreferences knows where the setting is actually active for the user, and wherever it is active, it will save it to.

Anything asking for the current value of the preference gets the current value of whatever preference is active anyways (global/local/local-override) so doing a merge for the list in Mute, for example, will be trivial:

  • It will get the value that already exists wherever the setting is actually active
  • Then it will merge into that list whatever user you muted
  • Then it will save that new list wherever the setting is active (Which is where the data came from)

Does that make sense?

Yeah, I've corrected my text.

Yeah, I've corrected my text.

Awesome, thanks! I added a small bit just verifying regarding getting the value of a preference. I'm fairly sure that's already happening, right? (just verifying) -- whatever code is asking for the value of the preference, they get the value of the preference that is active (global/local or local-override). Right?

Niharika triaged this task as Medium priority.May 30 2019, 10:44 PM
Niharika moved this task from Untriaged to Tracking work by other teams on the Anti-Harassment board.

That sounds like a good plan to me. We should start off with a whitelist and in future we can consider making that the default for all settings. Also @MaxSem volunteered to work on this ticket so putting this up for estimation by Community-Tech.

I'm not sure how this resolves the problem(s) I mentioned above. You're allowing an extension to change the behavior of a preference regardless of where it is saved.

This means, if I whitelist the email-blacklist preference, then everywhere that is saved, it will go through this process.

This is a problem, because if I save that preference with the API, it may or may not be global depending on the configuration of the wiki, and it will only be that preference, not any of the others.

Yes, I don't think that's a problem, that's the intent.

The whole issue here is that when the extension was written, it was deemed important to make sure that it's not having too much of unintended consequences of globalizing preferences, so the decision was made to split the behavior and make sure that global preferences are only stored if the user intentionally chose them to be that in Special:GlobalPreferences or if the product intentionally chose to override/do something by using the specific API.

Whether that's a good decision or not is a separate discussion, though there's a lot to say about testing the implications of making something "automatically global" before deciding to do that to all preferences. Right now, this behavior of Mute is a single case, and is not yet shown to be the norm, and our capacity is not big enough to risk falling into a trap of running around production fixing every product that may be intended to work differently -- so the tendency here is to try and be careful.

So by saying "here's a whitelist of preference names" we are basically taking a step in that direction: We don't change the behavior for *everything* where we risk missing edge cases that we can't account for (and will need to run around fixing.) Instead, we start with "only these preferences that we know of are always saved where they are active" and they will be saved like that no matter where you save them from -- intentionally. Because they're whitelisted in GlobalPreferences.

In a few months or whenever else, if we see that the whitelist is growing, and/or that the need is bigger than we though, then changing the code from "only do this to whitelisted preferences" into "do this to all preferences" is trivial.

This is the iteration-step, really. And if we think about the way we want to product to behave, having the whitelisted preference names act differently than other preferences (because they're whitelisted) is exactly the intent, not a bug.

So there are three potential behaviors with the API:

  1. The preference is saved locally
  2. The preference is saved locally OR globally depending on the user's preference for that preference
  3. The preference is saved locally OR globally depending on the user's preference for that preference IF the preference is in the config (which is different for each wiki and is not exposed with the API) OTHERWISE it is saved locally.

What you are proposing is #3 which, to me, is super complicated for the consumer to understand and is just a way to avoid doing #2 properly. I think there is a greater risk in being stuck in #3 permanently than there is in creating issues with #2. Also, this behavior is completely hidden to the API consumer. There isn't a way to know that a preference will be saved with the new behavior or the old one unless we create a new API endpoint to expose this config.

Also, the hypothesis assumes that email-blacklist is not one of those preferences that would in fact break if you suddenly change the behavior. And it would be really difficult for a developer to debug the problem because it's not obvious that this preference is stored differently. Likewise, it will work one way on some wikis and a different way on others.

I think the risk in doing #3 is way too high, and I would recommend pursuing an alternative solution.

So there are three potential behaviors with the API:

  1. The preference is saved locally
  2. The preference is saved locally OR globally depending on the user's preference for that preference
  3. The preference is saved locally OR globally depending on the user's preference for that preference IF the preference is in the config (which is different for each wiki and is not exposed with the API) OTHERWISE it is saved locally.

@Mooeypoo can correct me if I'm wrong here but I thought we were defining the config list globally and not per-wiki. I agree that doing it per-wiki would unnecessarily complicate things, like David mentions.

What you are proposing is #3 which, to me, is super complicated for the consumer to understand and is just a way to avoid doing #2 properly. I think there is a greater risk in being stuck in #3 permanently than there is in creating issues with #2. Also, this behavior is completely hidden to the API consumer. There isn't a way to know that a preference will be saved with the new behavior or the old one unless we create a new API endpoint to expose this config.

If we do it globally and not per-wiki, then #3 to me is an intermediate step to doing #2. It gives us some cushion to do testing and QA before enabling that for all preferences.

Also, the hypothesis assumes that email-blacklist is not one of those preferences that would in fact break if you suddenly change the behavior. And it would be really difficult for a developer to debug the problem because it's not obvious that this preference is stored differently. Likewise, it will work one way on some wikis and a different way on others.
I think the risk in doing #3 is way too high, and I would recommend pursuing an alternative solution.

Do you think the risk is still high if the config list is global and not per-wiki?

Do you think the risk is still high if the config list is global and not per-wiki?

I think that slightly mitigates it, but it still needs to be exposed somehow to API consumers which preferences are whitelisted (maybe if we add a flag in the preferences array like @MaxSem this would be the case?)

But also, if we can't commit to moving to #2, then I think the risk of permanently being in the #3 state is really high and I don't think that is a good place to be in for long. I can't imagine having to explain this behavior to someone.

@dbarratt I don't see any reason why we can't move to enabling it for all preferences after testing it on a whitelisted set of preferences. There are some weird preferences out there. I am being a bit cautious so we don't cause a change that catches users by surprise.

@dbarratt I don't see any reason why we can't move to enabling it for all preferences after testing it on a whitelisted set of preferences. There are some weird preferences out there. I am being a bit cautious so we don't cause a change that catches users by surprise.

I mean that's totally fine then. However, then this system is for testing purposes only and it doesn't really matter which ones you test on.

What if we did this the other way around? What if we made a blacklist instead of a whitelist?

Niharika added a comment.EditedMay 31 2019, 10:29 PM

@dbarratt Just so we don't accidentally cause any breakage, I would prefer to stick with a whitelist that we can grow and test with. There are way too many preferences to know which ones to blacklist to start with. For example, ULS does a thing when you change the language, it only sets it locally and doesn't want to make it global. So it is either a local pref or a local override. It is their product decision and us making this change might break the way ULS users expect it to work.
There are probably some other such preferences. We should do an announcement before we make this change for all preferences but meanwhile we can start with a whitelist.

Niharika set the point value for this task to 5.Jun 6 2019, 5:22 PM
MaxSem claimed this task.Jun 10 2019, 9:29 PM
MaxSem moved this task from Ready to In Development on the Community-Tech (Resolved 2018-19 Q4) board.

Change 516983 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/GlobalPreferences@master] WIP: support for autoglobal preferences

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

Change 516983 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Support for autoglobal preferences

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

dom_walden added a subscriber: dom_walden.

Concentrating on Special:Mute and email-blacklist option.

With:

$wgGlobalPreferencesAutoPrefs = ['email-blacklist'];

and muting/unmuting users via Special:Mute:

  • If option is global, then Special:Mute will set the option globally.
  • If option is global but is locally overridden, it will set the option locally.
  • If option is not global, then it will set the option locally.

I tested this on a local farm of wikis. Special:Mute will only set options globally on those wikis in the farm which have $wgGlobalPreferencesAutoPrefs appropriately set. Otherwise, it will continue to try to set it locally (even if it is global).

With:

$wgGlobalPreferencesAutoPrefs = ['language'];

Setting language via the API (api.php?action=options) leads to the same outcomes listed above.

I notice that the API response does not state whether the option was set locally or globally. Nor does it report failure if it tries to locally set a global option. I don't know if this matters.

@dom_walden Does the state (when loading the Special:Mute page or via the API) reflect what it should? (i.e. if it's globally set to true but locally set to false does it display as true?)

@dom_walden Does the state (when loading the Special:Mute page or via the API) reflect what it should? (i.e. if it's globally set to true but locally set to false does it display as true?)

Both the API and Special:Mute reflect the global status in this case.

Niharika closed this task as Resolved.Jul 11 2019, 11:49 PM