Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPII-3958: Implement the PSPChannel read API #826

Open
wants to merge 21 commits into
base: master
from

Conversation

@cindyli
Copy link
Contributor

cindyli commented Nov 13, 2019

No description provided.

cindyli added 17 commits Sep 6, 2019
… use of "gpii.pspChannel.adjustPreferenceMessage".
…g settings and context change to use typed messages.
…settings for a solution will read all settings from the system.
@cindyli cindyli requested a review from amb26 Nov 13, 2019

The value at the path `value.settingControls.{preference}.value` can be any valid value constrained by this setting's
schema defined in its solution registry entry. This value is required for sending the requested preference through the
matchmaking process but it doesn't take any effect to the system.

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

take any effect -> "have any effect"

This comment has been minimized.

Copy link
@klown

klown Nov 13, 2019

Contributor

"to the system" -> "on the system"
After the changes, the phrase is "have any effect on the system".

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 13, 2019

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 13, 2019

* @param {String} contextName - A context name.
* @return {Object} The actual preferences for the active context.
*/
gpii.lifecycleManager.getContextedPreferences = function (contextualPrefs, contextName) {

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

Given @klown 's #823 is in progress, I think we should avoid introduce the name "context" in any new material in the repository just so we have less work when it comes time to reconcile all of this. Anticipating this work I think we should name this function something like "getPreferencesForSet" and change the name of argument 2 to "prefsSet". Argument 1 is also oddly named, it is not really the "contextual prefs" but actually the full prefs document.

This comment has been minimized.

Copy link
@klown

klown Nov 13, 2019

Contributor

If the second argument is a name only, and not a set of preferences, then I suggest "prefsSetName".

var matchedPrefs = gpii.lifecycleManager.filterMatchedPrefs(transformed, contextualPrefs);

if (fluid.jQueryStandalone.isEmptyObject(matchedPrefs)) {
preferenceReadFailEvent.fire();

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

Is this really a failure? My impression is that the only real cause of a failure would be a promise rejection from the settings handler. It's unclear in what cases we would return an empty object here and what kind of failure this might be.

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

As a general note I always seem to find any use of isEmptyObject is a possibly suspicious smell which is why it has been left hosted at the jQuery level : P

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 14, 2019

Author Contributor

Good catch on settings handler failures. I've improved the code to cover this failure.

It's unclear in what cases we would return an empty object here and what kind of failure this might be.
Use cases discovered so far that will return an empty object here:

  1. Settings handlers that have get() defined but not implemented - https://issues.gpii.net/browse/GPII-4217
  2. "inverseCapabilitiesTransformations" blocks for one or more settings are not defined. In this case, although setting values are read successfully, when these values are transformed into preference values via gpii.lifecycleManager.transformSettingsToPrefs(), an empty object will return - https://issues.gpii.net/browse/GPII-4216

Overall, an empty object means a matched preference is not found and the read fails.

var session = that.getSession();

// Mock values for running preference thru matchMaker
var contextualPrefs = {};

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

As below, supply better names for this overall prefs document, and avoid any fresh reference to "context" in new code being contributed. activePrefsSetName is better

// Mock values for running preference thru matchMaker
var contextualPrefs = {};
var activeContextName = "gpii-default";
var prefSet = {};

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

I think this name also isn't accurate?

* Read corresponding setting values of the requested preferences. The read result is applied as
* a model change to userSession.model.currentPreferences.
* @param {Component} that - An instance of gpii.lifecycleManager.
* @param {Object} preference - The preference to read settings for. An example is:

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

I imagine more than one pref can be read here? Rename and adjust comment

}
handlerSpec.settings = handlerSpec.supportedSettings;

// TODO GPII-228 This is temporary debugging code while we work through

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

Is this still temporary debugging code? This JIRA appears to be "Implement snapshotter", has some material leaked into this pull by mistake somehow?

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 14, 2019

Author Contributor

This read() function is copied from @sgithens's snapshotter pull request. :) This comment is inherited from the copy. I've adjusted the wording to fit it in this context.

};

/*
* Inverse transform settings to common or application terms

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

Inverse -> invert? We could do with a lot more comments here explaining the context in which case this is helpful and preferably some example payloads accepted by this function. We are usually trying to supply @typedef s rather than plain {Object} for JSdoc arguments these days but I appreciate this might be difficult for our rather freely-structured prefs documents - but do try to do this wherever it is feasible.

if (prefsKey.startsWith(prefApplicationPrefix)) {
// application terms
fluid.each(prefsVal, function (innerPrefsVal, innerPrefsKey) {
var existed = fluid.get(preferences, [prefsKey, innerPrefsKey]);

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

existed -> exists both times


"use strict";

var fluid = require("infusion"),

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

I think this can be a browser test. Keeping the LM browser-clean has somewhat become relevant again with the concept that we may try to embed it in a browser extension, e.g. for UIO+, or as a ChromeOS extension

// Return lifecycleManager.userSession by default. If a GPII key is provided, return the session of the given key.
getSession: {
funcName: "gpii.lifecycleManager.getSession",
args: ["{that}.userSession", "{that}.restoreSession", "{arguments}.0"]
// GPII key
},
// Return the preferences of a given context
getContextedPreferences: {

This comment has been minimized.

Copy link
@amb26

amb26 Nov 13, 2019

Member

Doesn't seem a purpose in making this an invoker since it is a pure function

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 14, 2019

@cindyli

This comment has been minimized.

Copy link
Contributor Author

cindyli commented Nov 14, 2019

Thanks for the review comments, @amb26 @klown. All addressed and ready for another look.

}
handlerSpec.settings = handlerSpec.supportedSettings;

// TODO: The code below is to work around an issue with "gpii.windows.spiSettingsHandler" settings

This comment has been minimized.

Copy link
@amb26

amb26 Nov 19, 2019

Member

Can we add a JIRA reference for what the issue is, which hopefully has an explanation of how we plan to fix the settings handler

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 19, 2019

Author Contributor

@sgithens, is there a JIRA describing what the issue is? This workaround is copied from your snapshotter implementation. Thanks.


(function () {
/*
* Remove settings that are not in the matchMaker settings results.

This comment has been minimized.

Copy link
@amb26

amb26 Nov 19, 2019

Member

Explain when this method is useful

};

/*
* Invert application settings to common or application terms

This comment has been minimized.

Copy link
@amb26

amb26 Nov 19, 2019

Member

Explain when this method is useful

var liveness;
var applications = fluid.get(model, "activeConfiguration.inferredConfiguration.applications");
// "activeConfiguration" only tracks applied preferences. Preferences requested via the read API may or
// may not have entries here depending on if this read preference has ever being requested as applied

This comment has been minimized.

Copy link
@amb26

amb26 Nov 19, 2019

Member

typo "has ever being request as applied" -> "has ever been requested as an applied"


### Send Data to PSPChannel Server

the PSPChannle client can request various actions by

This comment has been minimized.

Copy link
@amb26

amb26 Nov 19, 2019

Member

Typo Channel

…reading multiple preferences via one read reqeust.
@cindyli cindyli changed the title GPII-3958: Implement the PSSChannel read API GPII-3958: Implement the PSPChannel read API Nov 20, 2019
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 20, 2019

@cindyli

This comment has been minimized.

Copy link
Contributor Author

cindyli commented Nov 20, 2019

Ready for another review.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.