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-2630: Implement the new GPII data model #591

Merged
merged 122 commits into from May 16, 2018

Conversation

Projects
None yet
6 participants
@cindyli
Copy link
Contributor

commented Feb 23, 2018

The new GPII data model can be found here.

This pull request includes:

  1. Removed the raw preferences server module;
  2. The preferences server and the flow manager now share the same database called "gpii";
  3. The preferences server in the dev config no longer reads/writes individual preferences files from the file system for preferences. These files are converted then loaded into PouchDB or CouchDB. Both the preferences server and flow manager reads/writes from pouchDB when running in the dev config, and CouchDB when running in the production config.
  4. Removed "vagrant-configs" directory. This directory has the VM that starts GPII in the production mode. This VM has been replaced by the VM running in the universal root directory in which GPII in the production config can be started using the universal docker image. The details can be found in the Production setup instruction documentation.

These ops repository pull requests need to be merged once this pull request gets in the master:

These ops repos for setting up the preferences server are no longer needed:

The repo docker-preferences-dataloader should be renamed to gpii-dataloader.

Note:

  1. ansible-flow-manager repo can be removed too if it's only used by the VM in the "vagrant-configs" directory, which has been removed in this branch.
  2. The docker image cindyqili/gpii-dataloader used at vagrantCloudBasedContainers.sh should point to gpii/gpii-dataloader when the latter is ready.

cindyli added some commits Nov 22, 2017

GPII-2630: Converted testData/preferences/*.json into gpiiKeys.json a…
…nd prefsSafes.json in the new data structure.
GPII-2630: Renamed "authDBServerPort" to "dbServerPort" since the dat…
…abase port will be shared by not only the auth server, but also the preferences server.
GPII-2630: In progress of refactoring the preferences server to repla…
…ce http endpoints with internal invokers for getting, creating and updating preferences.
GPII-2630: Replaced the preferences server API for creating a new pre…
…ferences set (POST to /preferences) with an invoker createPrefsSet().
GPII-2630: Completed refactoring the preferences server. In progress …
…of fixing node tests that use the prefs server.
@amb26

This comment has been minimized.

Copy link
Member

commented May 11, 2018

@cindyli - Further renaming point through from Gregg - throughout the code, "preferences set" should be globally replaced by "preference set"

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2018

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

@amb26, @simonbates, all review comments have been addressed. Also tested this branch with gpii/windows repo using its windows VM in these aspects:

  • "npm install" the published gpii-universal based off this branch
  • Running gpii/windows tests
  • login and logout with keys "carla" and "vladimir" in these configs:
    ** "all-in-local" config that uses the default gpii.config.development.all.local
    ** Running the cloud and the untrusted LFM as separate node instances on different ports: the cloud uses gpii.config.cloudBased.development.all.local and the untrusted LFM uses gpii.config.untrusted.development.

All above works well. Let me know if I missed anything.

This pull request is ready for more reviews.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2018

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

ok to test

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2018


var fluid = require("infusion");

fluid.module.register("gpii-db-operation", __dirname, require);

This comment has been minimized.

Copy link
@amb26

amb26 May 15, 2018

Member

I meant by the rename for the entire module to be renamed - the main purpose of the rename is for its name in package.json to be a valid npm module name. This directory, and the name in package.json should be renamed as well. We can retain the camel-casing in the individual file names.

isError: true
},
missingDoc: {
message: "The record of %docName is not found",

This comment has been minimized.

Copy link
@amb26

amb26 May 15, 2018

Member

Why is this named "docName" when at all call sites it is actually sourced from a "docType"?
Also, the wording of the error is not terribly clear. Perhaps it should be something like "A record of type %docType was not found"?

This comment has been minimized.

Copy link
@amb26

amb26 May 16, 2018

Member

Bump this comment

This comment has been minimized.

Copy link
@cindyli
isError: true
},
mismatchedDocType: {
message: "The document type must be \"%docType\"",

This comment has been minimized.

Copy link
@amb26

amb26 May 15, 2018

Member

It would probably be helpful to specify what type was actually chosen by the user

* @param {Component} ontologyHandler - An instance of gpii.ontologyHandler component.
* @param {Object} preferences - A preferences object.
* @param {String} view - An view that the provided preferences is based upon.
* @param {String} gpiiKey - (Optional) The GPII key to be created.

This comment has been minimized.

Copy link
@amb26

amb26 May 15, 2018

Member

Optional params are signalled in JSDocs by putting the parameter name in square brackets - http://usejsdoc.org/tags-param.html#optional-parameters-and-default-values - although we tend to also retain the text [optional] for clarity in the description


if (data && data.doc && data.value) {
result = {};
fluid.set(result, "oauth2ClientId", data.key);

This comment has been minimized.

Copy link
@amb26

amb26 May 15, 2018

Member

Another clutch of unnecessary fluid.sets


if (data && data.doc && data.value) {
result = {};
fluid.set(result, "accessToken", data.key);

This comment has been minimized.

Copy link
@amb26

amb26 May 15, 2018

Member

Some more

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2018

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

@amb26, all comments have been addressed. Ready for more reviews. Thanks.

@@ -84,7 +84,7 @@ var gpii = fluid.registerNamespace("gpii");

/**
* Decode the URL to find out the view/map function name and parameters for this function.
* @param url {String} The URL to be decoded
* @param {String} url - The URL to be decoded
* @return {Object} The returned object is in the structure of:
* {
* viewName: {String},

This comment has been minimized.

Copy link
@amb26

amb26 May 16, 2018

Member

Note that the "else" branch in this function is untested - is it necessary?

This comment has been minimized.

Copy link
@cindyli

cindyli May 16, 2018

Author Contributor

I added a test to cover the "else" block.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2018

@cindyli

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

@amb26, comments so far have been addressed again.

@amb26 amb26 merged commit cdf652f into GPII:master May 16, 2018

1 check passed

default Build finished.
Details

@cindyli cindyli deleted the cindyli:GPII-2630 branch Nov 28, 2018

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.