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-3721: Enhance CBFM to support the creation of new GPII keys and associated preferences #744

Merged
merged 7 commits into from Mar 5, 2019

Conversation

Projects
None yet
3 participants
@cindyli
Copy link
Contributor

commented Feb 22, 2019

No description provided.

@cindyli cindyli requested a review from amb26 Feb 22, 2019

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

message: "Error when retrieving preferences: " + err.message,
statusCode: 404
});
var nonExistentGPiiKeyMsg = "GPII key \"" + gpiiKey + "\" does not exist";

This comment has been minimized.

Copy link
@amb26

amb26 Mar 1, 2019

Member

Funny capitalisation of this string

});
var nonExistentGPiiKeyMsg = "GPII key \"" + gpiiKey + "\" does not exist";
// GPII-3721: Return an empty payload for nonexistent GPII keys
if (err.message.includes(nonExistentGPiiKeyMsg)) {

This comment has been minimized.

Copy link
@amb26

amb26 Mar 1, 2019

Member

Looking for a substring within a user message isn't a reliable way of implementing core infrastructure. We need to improve the preferences server so that it supplies in the rejection payload an error field with a statically determinable error code, of the kind that we get from HTTP or FS errors, e.g. something like "GPII_ERR_NO_SAFE"

This comment has been minimized.

Copy link
@amb26

amb26 Mar 1, 2019

Member

Please add a comment referencing GPII-3721 to this site so we can find it easily again when we come to implement the proper grant-sensitive policy, so that we can easily search for the code sites that need to be fixed

@@ -24,10 +24,11 @@ fluid.defaults("gpii.flowManager.cloudBased.settings.put.handler", {
funcName: "gpii.flowManager.cloudBased.settings.put.handleRequest",
args: [
"{flowManager}.prefsServerDataSource",
"{gpii.flowManager.cloudBased}.authGrantFinder",

This comment has been minimized.

Copy link
@amb26

amb26 Mar 1, 2019

Member

This argument payload is now over the critical mass - please encode it as a struct with named members rather than a sequence of 6 arguments

@@ -44,10 +44,15 @@ fluid.defaults("gpii.preferencesServer", {
type: "gpii.preferencesServer.get.handler"
},
preferencesPost: {
route: "/preferences",
route: "/preferences/:gpiiKey",

This comment has been minimized.

Copy link
@amb26

amb26 Mar 1, 2019

Member

Why do we have preferencesPost in addition to preferencesPut?

This comment has been minimized.

Copy link
@cindyli

cindyli Mar 1, 2019

Author Contributor

POST is to create a new gpiiKey and its vault.

PUT is to update the vault of an exiting gpiiKey. If the key doesn't exist, an error will be returned.

@amb26

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Please merge up with trunk for conflicts

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@amb26, all comments have been addressed. This pull request is ready for another look.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

errorCode: "GPII_ERR_NO_GPIIKEY",
message: "GPII key does not exist"
},
gpiiKeyExisted: {

This comment has been minimized.

Copy link
@amb26

amb26 Mar 4, 2019

Member

Should read "Exists" rather than "Existed"

errorCode: "GPII_ERR_GPIIKEY_ALREADY_EXIST",
message: "GPII key \"%gpiiKey\" already exists"
},
noUpdateOnSnapset: {

This comment has been minimized.

Copy link
@amb26

amb26 Mar 4, 2019

Member

Rather than encode "snapset" in this key and message name, I think it's wiser to just refer to the prefs set being "readOnly" (which it might be for a variety of reasons). GPII_ERR_PREFS_READONLY and similarly in the key name. You could refer to "snapset" in the message detail - e.g. "Can't update preferences for readOnly GPII key %gpiiKey (perhaps it is a snapset)"

gpiiKeyPromise.then(function (data) {
// GPII-3721: If the GPII key already exists, update its prefs safe. Otherwise, create the GPII key and its prefs safe.
if (data) {
handlerPromise = that.prefsServerDataSource.update(gpiiKey, "true", preferences);

This comment has been minimized.

Copy link
@amb26

amb26 Mar 4, 2019

Member

boolean true seems more appropriate than string "true"

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@amb26, thanks for suggestions. All addressed and ready for another look again.

message: "GPII key \"%gpiiKey\" already exists"
},
readOnly: {
errorCode: "GPII_ERR_NO_UPDATE_TO_SNAPSET",

This comment has been minimized.

Copy link
@amb26

amb26 Mar 5, 2019

Member

Let's get rid of "SNAPSET" from this errorCode's value

message: "GPII key does not exist"
},
gpiiKeyExists: {
errorCode: "GPII_ERR_GPIIKEY_ALREADY_EXIST",

This comment has been minimized.

Copy link
@amb26

amb26 Mar 5, 2019

Member

Should be GPII_ERR_GPIIKEY_EXISTS

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Good catch, @amb26. Ready for another review. Thanks.

@amb26 amb26 merged commit 9624433 into GPII:master Mar 5, 2019

1 check passed

default Build finished.
Details
@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Merged at 167c851

@cindyli cindyli deleted the cindyli:GPII-3721 branch Mar 5, 2019

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