GPII-2966 APIs provided by Preferences Server to support PPT #634
Conversation
…ndled by gpii-express-user going forward.
…icial one in master.
sgithens
changed the title
|
CI job failed: https://ci.gpii.net/job/universal-tests/973/ |
testData/dbData/views.json
Outdated
| @@ -10,6 +10,15 @@ | |||
| }, | |||
| "findAuthorizationByAccessToken": { | |||
| "map": "function(doc) {if (doc.type === 'gpiiAppInstallationAuthorization' && doc.revoked === false) {emit(doc.accessToken, {'_id': doc.clientId, 'authorization': doc})}}" | |||
| }, | |||
| "listKeysForPrefsSafe": { | |||
There was a problem hiding this comment.
Most view names starts with "find" and these 2 use "list". Is there a reason in using a different word? If not, can you sync up the view names?
There was a problem hiding this comment.
I think this makes sense @cindyli . I will probably leave listPrefsSafes as it is, since there is not search argument given to it, it really is just a listing.
testData/dbData/views.json
Outdated
| "map": "function(doc) {if (doc.type === 'gpiiKey') emit(doc.prefsSafeId, null) }" | ||
| }, | ||
| "listPrefsSafes": { | ||
| "map": "function(doc) { if (doc.type === 'prefsSafe') emit(doc._id, { 'name': doc.name, 'email': doc.email, 'created': doc.timestampCreated, 'updated': doc.timestampUpdated })}" |
There was a problem hiding this comment.
Remove extra spaces before "created" and "updated".
| }); | ||
|
|
||
| gpii.preferencesServer.listKeysForPrefsSafe.handler.getKeysForPrefsSafe = function (prefsService, preferencesServer, request, prefsSafeId, view) { | ||
| var finalDirectModel = fluid.extend(true, {}, prefsService.dataStore.listKeysForPrefsSafeDataSource.options.directModel, { |
There was a problem hiding this comment.
I don't see a value for prefsService.dataStore.listKeysForPrefsSafeDataSource.options.directModel? Would it be more straightforward to change this line to:
var directModel = {
prefsSafeId: prefsSafeId
};
| prefsSafeId: prefsSafeId}); | ||
| var prefsPromise = prefsService.dataStore.listKeysForPrefsSafeDataSource.get(finalDirectModel); | ||
| prefsPromise.then(function (data) { | ||
| var togo = []; |
There was a problem hiding this comment.
line 76-81 can be simplified using fluid.transform.
|
|
||
| gpii.preferencesServer.prefsSafeWithKeysGet.handler.get = function (prefsService, preferencesServer, request, prefsSafeId, view) { | ||
| // TODO Refactor from above, copied/pasted | ||
| var finalDirectModel = fluid.extend(true, {}, prefsService.dataStore.listKeysForPrefsSafeDataSource.options.directModel, { |
There was a problem hiding this comment.
Same comment as above on the use of prefsService.dataStore.listKeysForPrefsSafeDataSource.options.directModel. This type of comment applies across the pull request.
| } | ||
| }); | ||
|
|
||
| gpii.preferencesServer.prefsSafeWithKeysGet.handler.get = function (prefsService, preferencesServer, request, prefsSafeId, view) { |
There was a problem hiding this comment.
This function does 2 things:
- Find keys associated with a prefs safe;
- Find the data of this prefs safe and add keys retrieved at step 1 to the data.
A couple of suggestions:
- The step 1 is shared by this handler and
gpii.preferencesServer.listKeysForPrefsSafe.handler, so it can be extracted into a function to get rid of the duplication; - Use
fluid.promise.fireTransformEvent()to chain these 2 promises that the output of the first promise can be passed to the second promise. Usingfluid.promise.fireTransformEvent()also helps to reuse the function in the first suggestion.
| // TODO Refactor from above, copied/pasted | ||
| var finalDirectModel = fluid.extend(true, {}, prefsService.dataStore.listKeysForPrefsSafeDataSource.options.directModel, { | ||
| prefsSafeId: prefsSafeId}); | ||
| var prefsPromise = prefsService.dataStore.listKeysForPrefsSafeDataSource.get(finalDirectModel); |
There was a problem hiding this comment.
All handlers in this script are accessing prefsService.dataStore directly, which is not recommended. The prefsService acts as a bridge that helps to decouple the database operation from other GPII modules including preferencesServer. prefsService should provide functions that make sense to request handlers instead of having request handlers directly deal with data that are in raw document structure. I would suggest to move the 2 steps handling in this function to prefsService.
This comment applies to other handlers in this script.
|
Nice work, @sgithens. I added "needs work" label as you will continue to add tests and docs. Once those are out of way, please remove this label. Thanks. |
|
ok to test |
|
CI job failed: https://ci.gpii.net/job/universal-tests/980/ |
|
CI job failed: https://ci.gpii.net/job/universal-tests/1044/ |
- Adding in unit tests - Adding in usage of gpii-express-user - Adding endpoints for creating and unlocking cloud safe credentials.
…ch is caused by the wrong merge handling on arrays.
…to pass unit tests.
- Rename listKeysForPrefsSafe to findKeysForPrefsSafe per review
|
CI job passed: https://ci.gpii.net/job/universal-tests/2064/ |
|
@cindyli @amb26 @klown Hi everyone. This should be ready for another review now. To recap, this branch contains the initial work necessary for username/password account credentials. Previously Cindy had reviewed the entire branch and ok'd it, but the remaining item was not having a migration to remove the For the migration, I've largely cloned the previous GPII-4011 migration Joseph did. Since we're going to be migrating from couchdb shortly I didn't spend a lot of time trying to refactor the common code from the 2 migrations since it seems like it won't be necessary in the future. |
This comment doesn't seem to be addressed. |
|
@cindyli Ok, here is what I've done, with a little explanation. I removed the custom userLookup.json views from the branch. I had on previous occasions thought also that it would be better to use the actual file from gpii-express-user, except that gpii-express-user uses a slightly different method of loading it's data. It's json views are in a single top level JSON object in the file, where-as ours are contained within a top level JSON array. So our dataloader won't load it. I had looked at possibly just loading and transforming the data, expect that the dataloader many layers down expects a file name rather than the JSON, and it would be quite a bit of refactoring to change this I think. Given that we are planning on shortly moving off of couchdb, it didn't seem as if this was worth my time, so I have just included the necessary views along ours in the views.json. I also think this makes the provisioning code simpler, as it works as it, and I don't need to fix the dataloader to load more view files from other node_modules. If we weren't in our last days with couch I would say maybe we should work more on loading views from 3rd party modules but it doesn't seem worth it right now. Also... based on our in flux requirements for user accounts and authentication, I can see us actually wanting to customize these couchdb views slightly for universal. For instance we might want to change the byUsernameOrEmail view a bit to tighten up how the credentialLogin documents are found. I'm not sure if we'll do that, but I wouldn't be surprised if we did. Anyways, this is not perfect, but it's very simple, doesn't change any of our deployment scripts, and I think would work best for our current database trajectory. |
|
Thanks for the explanation, @sgithens. I'm fine with this approach. As you said, we're moving off of CouchDB soon. |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2091/ |
| // TODO This will have json schema validation when the scaffolding is all in. | ||
| dataStore.updatePrefsSafe(prefsSafeData.id, prefsSafeData).then( | ||
| function (/* updatedReturn */) { | ||
| that.getPrefsSafe(prefsSafeData.id).then( |
There was a problem hiding this comment.
This is a too deeply nested promise chain and needs to be broken up
There was a problem hiding this comment.
This has been converted to a transforming promise chain.
|
|
||
| fluid.defaults("gpii.preferencesServer.preferencesService", { | ||
| gradeNames: ["fluid.component"], | ||
| components: { | ||
| dataStore: { | ||
| type: "gpii.dbOperation.dataStore" | ||
| }, | ||
| expressUserUtils: { | ||
| type: "gpii.express.user.utils", |
There was a problem hiding this comment.
I am very suspicious of a component named "utils" ...... what does it do?
There was a problem hiding this comment.
This is a component in the gpii-express-user module that contains the routines for creating and unlocking users.
|
Please could you try out merging this against #833 to flush out whether it happens to perform any modification of component options which will be prohibited under the new framework |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2239/ |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2247/ |
|
@amb26 I've address the couple comments above. I'm investigating a test error when merging in your FLUID-6145 branch, which may or may not be related to gpii-express-user. But in general I believe this is ready for another review. |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2248/ |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2249/ |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2271/ |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2340/ |
|
CI job failed: https://ci.gpii.net/job/universal-tests/2341/ |
|
CI job failed: https://ci.gpii.net/job/universal-tests/2370/ |
Hi @cindyli . There is some cleanup, plus docs and tests needed for this still, but I wanted to at least run the endpoints by you before the weekend and get any comments on them in general. Also there are 3 new couch views for this.