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-2965: Save PSP updated preferences to GPII Cloud #617
Conversation
…e untrusted version of the local flow manager.
* antranig/GPII-2831: GPII-2831: Dependency update to released kettle and other outdated GPII-2831: Updated path to in-browser JSON5 implementation for drift in dependency update GPII-2831: Improvements following review and dependency update GPII-2831: Removed unused kettle variable GPII-2831: Moved over to configs and driver function which apply test case pouch harness and corrected typo GPII-2831: Updated new preference files and .gitignore GPII-2831: Applied mock platform reporter for Windows to avoid duplicating exploding solution into Linux registry GPII-2831: Implementation and test cases for "User Errors" bus.
…odel.preferences between PSP and initial user key-ins/context manager. The latter should not trigger the save of preferences to the cloud.
…erences to the cloud.
CI job passed: https://ci.gpii.net/job/universal-tests/895/ |
CI job failed: https://ci.gpii.net/job/universal-tests/900/ |
CI job failed: https://ci.gpii.net/job/universal-tests/901/ |
CI job passed: https://ci.gpii.net/job/universal-tests/902/ |
@@ -203,6 +211,35 @@ var gpii = fluid.registerNamespace("gpii"); | |||
return fullPayload; | |||
}; | |||
|
|||
// Transform preferences to final payload and apply the final payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proper comment block for this method would be great :)
@@ -65,6 +71,15 @@ var gpii = fluid.registerNamespace("gpii"); | |||
} | |||
}); | |||
|
|||
// Only save prefs to the cloud when the preferences are updated. The change requests on session.model.preferences | |||
// are also fired by the lifecycleManager session start and the contextManager, in which cases preferences should | |||
// not be written to the cloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proper comment block here also - we should consider ourselves in a "modern era" where every significant function should be documented
fluid.set(contextualPrefs, fluid.pathUtil.parseEL(pref), spec.value); | ||
}); | ||
|
||
// The updated preferences meant to be saved to cloud should be have the applier change fired with source === "PrefsUpdated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty strange choice of namespace and will not really help anyone who reads it to understand the purpose - since clearly the preferences are always being updated if there is change to "preferences". Could you pick a namespace which more clearly defines what the route of update is. Perhaps something like "userUpdate" or "directUserUpdate"
}, { | ||
event: "{rawPrefsAtEnd}.events.onComplete", | ||
listener: "gpii.test.untrusted.pspIntegration.verifyRawPrefsAtEnd" | ||
// "i" needs to be dynamically provided at constructing testDefs with the current sequence number to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a shame we never had time to start working with the FLUID-5903 grade sequence system
@@ -84,37 +84,16 @@ fluid.defaults("gpii.pspChannel", { | |||
} | |||
}, | |||
events: { | |||
preferencesApplied: null | |||
preferencesAppliedLocal: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that there are any listeners to this event, or tests for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This event is fired once the updated preferences are applied to the local computer. At the moment it's only used to compose the aggregate event preferencesApplied
that indicate the completion of the handling of the updated preferences. This aggregate event has been tested in the PSP integration test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - could you add a comment indicating that its main current purpose is to compose the aggregate event - thanks
@@ -44,16 +44,54 @@ fluid.defaults("gpii.flowManager.untrusted", { | |||
proximityTriggered: { | |||
gradeNames: "gpii.flowManager.untrusted.stateChangeHandler" | |||
} | |||
}, | |||
invokers: { | |||
setSettings: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably partner this with a "notImplemented" invoker in FlowManager.local, just to remind ourselves to implement this at the point we ever try using a trusted FlowManager again
|
||
gpii.flowManager.untrusted.getSettings = function (untrustedSettingsDataSource, gpiiKey, deviceReporterData, onMatchDone, onError) { | ||
var settings = untrustedSettingsDataSource.get(gpiiKey, deviceReporterData); | ||
settings.then(onMatchDone.fire, onError.fire); | ||
}; | ||
|
||
gpii.flowManager.untrusted.setSettings = function (untrustedSettingsDataSource, gpiiKey, preferences, preferencesSavedSuccess, preferencesSavedError) { | ||
var saveResult = untrustedSettingsDataSource.set(gpiiKey, preferences); | ||
saveResult.then(preferencesSavedSuccess.fire, preferencesSavedError.fire); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More useful than having the Success/Error events would be to return this promise, and also document this method
@@ -71,6 +71,7 @@ var gpii = fluid.registerNamespace("gpii"); | |||
onSessionStart: null, // fired with [gradeName, gpiiKey] | |||
onSessionSnapshotUpdate: null, // fired with [{lifecycleManager}, {session}, originalSettings] | |||
onSessionStop: null, // fired with [{lifecycleManager}, {session} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing square bracket here
"gpii_userErrors_StopApplicationFail-details": "You can stop the program yourself. If this problem continues, try restarting your computer.", | ||
"gpii-userErrors_SaveToCloudFail-title": "Adjusted settings are not saved", | ||
"gpii_userErrors_SaveToCloudFail-subhead": "The adjusted settings was failed at saving to the Cloud so they can not be applied on other computers.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update this with the new working which is now listed in the UX spreadsheet at https://docs.google.com/spreadsheets/d/1ybyf9xoYoArjswWBhhBWECrrJdTPspI6FAuzjtcNmsM/edit#gid=0 ?
Also, this is a good opportunity to rename "GPII" to "Morphic" in this entire bundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. will do.
CI job passed: https://ci.gpii.net/job/universal-tests/941/ |
@amb26, thanks for the review. All comments have been addressed and this pull request is ready for another look. |
@@ -138,12 +138,37 @@ fluid.defaults("gpii.flowManager.local", { | |||
type: "gpii.flowManager.getGpiiKey.handler" | |||
} | |||
}, | |||
invokers: { | |||
// Needs to be implemented if the handling is required when users update their preferences. | |||
setSettings: "fluid.identity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fluid.notImplemented is safer, plus an explicit implementation in the other derived class - which can probably just always return a promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I encountered with using fluid.notImplemented
is, this function reports a failure when the associated invoker is not implemented, which makes sense when this function presents as a placeholder/reminder. However when GPII runs in all-in-local config where the other derived "untrusted" class is not included, fluid.notImplemented
will be executed and cease the run with a failure.
Let me know if I didn't use function in a proper way. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've improved this placeholder function to return an empty promise as you suggested in the comment below.
} | ||
}); | ||
|
||
gpii.flowManager.local.savePreferences = function (that, gpiiKey, preferences) { | ||
var savePromise = that.setSettings(gpiiKey, preferences); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "internal polymorphism" is fragile and unnecessary. It's ok to implement setSettings so it always returns a promise - or alternatively just go over to the "transforming promise chain" approach and make setSettings into a pseudoevent as we are now doing in many places.
https://github.com/fluid-project/infusion/blob/master/src/framework/core/js/DataSource.js#L178
CI job passed: https://ci.gpii.net/job/universal-tests/946/ |
@amb26, new comments have been addressed. This pull request is ready for more review. |
No description provided.