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-3117: Remove the EnvironmentReporter and adjust ContextManager accordingly #823

Merged
merged 35 commits into from Nov 22, 2019

Conversation

@klown
Copy link
Contributor

klown commented Oct 28, 2019

JIRA: https://issues.gpii.net/browse/GPII-3117

  • Removed periodic invocation of EnvironmentReporter from ContextManager,
  • Removed PUT /environmentChanged endpoint,
  • Removed support for conditions: blocks in prefsSafes, and removed the blocks themselves from preferences data and test code,
  • Moved the remaining functionality to the LifeCycleManager, and removed the ContextManager entirely,
  • Continued support for manual context (now "prefsSets") changes by the user via a preferences editor, via PSPChannel integration.

NOTE: Before merging with master, tag master with 'EnvironmentReporter' to easily find the removed functionality if we decide to go back to it.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 28, 2019

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Oct 28, 2019

There's a PSPChannel integration test that tests the PSPChannel context change API with a /environmentChanged request - https://github.com/GPII/universal/blob/master/tests/shared/PSPIntegrationTestDefs.js#L25-L29

This test can be removed too.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Oct 28, 2019

Thanks, @cindyli. I'll remove it.

What's puzzling is there is no /environmentChanged endpoint anymore, so why does the test pass? Specifically, why is there no 404 or other error code for that request? The log shows:

15:59:00.955: Sending a PUT request to: /environmentChanged on port 8081
15:59:00.982: Kettle server getDispatcher found no matching handlers, continuing

Hopefully there aren't any more of these lurking around.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 28, 2019

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 28, 2019

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 29, 2019

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Oct 29, 2019

Ready for review @cindyli although the README needs work.

Also, I found it tricky to make one of the integration tests work. I came up with a solution that involves storing context information in the ContextManager. I'm not sure that it's the correct way to go. The test case is possibly invalid now. The test case is where the context changes before a user logs in. Previously, the ContextManager stored information about the environment such that when a user logged in, the context appropriate to the stored environment was available. Removing all the code/data for the environment means that context information is gone, and the test fails. Storing the context name (e.g., "bright"), in the ContextManager model is sufficient to make the test work again. In addition, making that change has not caused regressions elsewhere.

Still, I don't know enough to say whether storing a context in the ContextManager is useful or legitimate. Do you have any insight?

The same technique is used in one of the PSPIntegrationTests that previously depended on a /environmentChanged request. But in this case, the change in context should not change the settings (and it doesn't). I could see removing this test entirely, but, again, I'm not completely sure.

@amb26

This comment has been minimized.

Copy link
Member

amb26 commented Oct 29, 2019

Doesn't make sense - if the EnvironmentReporter is removed, we should remove the ContextManager implementation and its tests also since they serve no user function.

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Oct 29, 2019

@amb26, the code left for ContextManager in @klown's ContextManager serves as a bridge to support the PSPChannel context change API that allows the switch from for example "gpii-default" to "bright". The remaining code sets lifecycleManager.session.model.activeContextName to the new context name then triggers lifecycleManager.update(). If it's OK to add a new contextChange() function into lifecycleManager and get PSPChannel trigger it directly, ContextManager can be removed completely.

Is this what you meant by removing the ContextManager implementation?

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Oct 29, 2019

@klown, the PSPIntegrationTests you mentioned can be removed completely.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Oct 29, 2019

@amb26 , as @cindyli noted, the point was to keep the functionality that handled context changes coming through the PSPChannel. The choice is either keep that contained inside a ContextManager component, or disperse the functionality elsewhere.

FWIW, I've found it easier to concentrate on a component called "ContextManager" and not think about where the functionality and tests would migrate to if there was no such component.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Oct 29, 2019

@cindyli

@klown, the PSPIntegrationTests you mentioned can be removed completely.

Yes, it doesn't look like it was doing anything real anymore, but I wasn't sure.

What about the "Context changed before login" test in ContextIntegrationTests? Towards an answer to the question:

  • Can a context change come through the PSPChannel before a user logs in?
  • Is that context asserted (stored, noted) such that it has an effect after the user logs in?
@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Oct 29, 2019

@klown, I think whether or not removing ContextIntegrationTests depends on the decision of whether ContextManager should be removed.

* Can a context change come through the PSPChannel before a user logs in?

A special GPII key "noUser" will log in when there's no real user logs in. It's nice to test a context change doesn't break the system when "noUser" is logged in or even before any user including "noUser" logs in.

* Is that context asserted (stored, noted) such that it has an effect after the user logs in?

It may not be a good idea to have ContextManager save the current context even if we decide to keep it since the active context is tracked by lifecycleManager.userSession.model.activeContextName. Saving the active context in ContextManager will have to ensure the sync-up of this value in 2 components.

@amb26

This comment has been minimized.

Copy link
Member

amb26 commented Oct 29, 2019

"Contexts" have long since been a cursed term in the project, since they have ceased to have the meaning of what we now call "preference sets" for over 3 years. @cindyli 's comment of a few minutes ago is spot on, there is no need for separate tracking anywhere else of the piece of state held at lifecycleManager.userSession.model.activeContextName , which we should take the opportunity to rename as activePrefsSetName . As a component the ContextManager itself can be removed wholesale together with its dedicated tests, although as noted we still need tests which verify the overall functionality of changing prefs set name, e.g. via the PCP API. There is no meaning of a "context change before a user logs in" because the only current meaning corresponding to a "context" is a selection from the set of prefs set names which are found to be available in the user's preference safe at the point they log in.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Oct 29, 2019

It may not be a good idea to have ContextManager save the current context even if we decide to keep it since the active context is tracked by lifecycleManager.userSession.model.activeContextName. Saving the active context in ContextManager will have to ensure the sync-up of this value in 2 components.

The way it's written keeps them in sync, with one exception. Glossing over details, if ContextManager.updateCurrentContext() is called with new context:

  • it updates the ContextManager's model, causing a model change
  • that triggers the ContextManager's contextChanged() model listener
  • that updates the lifecycleManager.userSession.model.activeContextName

Note that this works the same way it did when the ContextManager model was a currentEnvironment. The difference is storing a context vs. an environment.

The one exception is when no one is logged in (more accurately when a reserved GPII key is active). That relates to the test in question -- changing the context when no one is logged in. Based on the logic of the code, I'm guessing it's undesirable to store a pre-login context name in the lifecycleManager.userSession.model.activeContextName, but I don't know why.

What is problematic about storing a change in context in the LifecycleManager when no one is logged in? Perhaps it isn't. If so, that would be another reason to get rid of the ContextManager.

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Nov 11, 2019

Removed ContextManager files should be removed from .nycrc.

Searching the code base with activeContext shows 12 matches in a few files. Please consider to change them to activePrefsSet.

*/
gpii.lifecycleManager.updateActivePrefsSetName = function (that, fullPayload, newPrefsSetName) {
var activeSessionPrefsSetName = that.getSession().model.activePrefsSetName;
if (!newPrefsSetName) {

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 11, 2019

Contributor

line 1337-1344 can be simplified to:

fullPayload.activePrefsSetName = newPrefsSetName ? newPrefsSetName : activeSessionPrefsSetName ? activeSessionPrefsSetName : "gpii-default";
path: "/environmentChanged",
method: "PUT"
});

// NOTE: This inherits from from "gpii.test.testCaseHolder" via "gpii.test.integration.testCaseHolder.linux"
// from which it gets loginRequest, logoutRequest and other standard events
fluid.defaults("gpii.tests.contextIntegration.testCaseHolder", {

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 11, 2019

Contributor

With the removal of ContextManager, this file can be removed too.

This comment has been minimized.

Copy link
@klown

klown Nov 11, 2019

Author Contributor

Removed ContextManager files should be removed from .nycrc.

Okay, done.

Searching the code base with activeContext shows 12 matches in a few files. Please consider to change them to activePrefsSet.

I could only find three matches for "activeContext" (singular), but maybe that's because I had already removed files or sections of files with "conditions" or "ContextManger", etc. The three remaining matches were all relative to the MatchMaker. Here are the links, where activeContexts has been replaced with activePrefsSets:

I'm still pondering that last one because, so far, I can't see where it's used.
Update: removing it caused two test failures:

  • jq: FAIL: Module "gpii.tests.acceptance.windows.builtIn.config tests" Test name "Testing os_win using default matchmaker" - Message: Checking that settings are set - at sequence position 18 of 30
  • jq: FAIL: Module "Journal Restoration Tests" Test name "Journal state and restoration" - Message: Checking that settings are properly reset - at sequence position 21 of 34

This comment has been minimized.

Copy link
@klown

klown Nov 12, 2019

Author Contributor

@cindyli I think I've found a bug, and am looking to confirm. Here's an summary of the "problem". Note that the links below are to the current master branch.

The MatchMaker utility gpii.matchMakerFramework.utils.preProcess() allocates an activeContexts[] array.

Later the cloud-based FlowManager's SettingsGetHandler performs the match in gpii.flowManager.cloudBased.matchToSettings(). The first operation there is to filter the finalPayload in terms of keys such as gpiiKey, preferences, and others. In particular, it looks for the activeContextName key. Note the difference between activeContexts and activeContextName. I don't think the SettingsGetHandler ever retrieves the activeContexts, and I don't know if there ever is an activeContextName.

However, I've put in some log messages, and am running all of the tests to see what matchToSettings() actually does. I'll let you know the results.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 11, 2019

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 13, 2019

@cindyli
I wrote:

I've put in some log messages, and am running all of the tests to see what matchToSettings() actually does. I'll let you know the results.

The results of running all the node tests with this extra logging information is listed below. The input to the match is finalPayload which is filtered in creating settings. After the filtering, settings.preferences is further processed by gpii.matchMakerFramework.utils.filterPreferencesFromInferredConfig().

  • finalPayload.activeContexts exists, but is always empty
  • finalPayload.activeContextName does not exist
  • neither settings.activeContexts nor settings.activeContextName ever exist.

I've also removed the activeContexts array where it is first added, and re-run the tests. Nothing fails. So far, it looks like activeContexts is superfluous in this context (pardon the pun).

The only thing left to check are the browser tests to see if activeContexts shows up there.

Note: "activePrefsSets" was formerly "activeContexts".
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 13, 2019

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 14, 2019

@cindyli @amb26 I've removed the use of activePrefsSets (formerly activeContexts) local to MatchMakerUtilities.js. I cannot find its use anywhere. For the same reason, I've deleted the test data file matchmaker.out.payload.json.

If you know that these are useful somehow, let me know and I'll put them back. Otherwise, this is ready for another review, thanks.

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Nov 14, 2019

In "documentation/README.md", line 16 can be removed. It points to "context manager" doc that no longer exists.

"context" appears in "documentation/MatchMakerFramework.md" at line 5 and 53 should be changed to "preferences set".

In "documentation/PreferencesServer.md" line 12, "according to a specific set of contexts" -> "according to a specific preferences set".

In "documentation/SolutionRegistryFormat.md" line 311, "context changes" -> "preferences set changes".

In "documentation/Transformer.md" line 13, remove "/context manager".

In "examples/pspChannelClient/README.md", modify various appearance of "activeContextName" and "context".

In "examples/pspChannelClient/pspChannelClient.js" line 40, "activeContextName" -> "activePrefsSetName".

In "gpii/node_modules/eventLog/src/metrics.js", "gpii.metrics.trackContextChange" function name should be renamed to "gpii.metrics.trackPrefsSetChange". Also at line 174, 175, "active context" -> "active preferences set".

In "gpii/node_modules/flowManager/src/PSPChannel.js" and ,"gpii/node_modules/flowManager/test/PSPChannelTests.js", except the use of "contexts", most appearance of "context" should be changed to "preferences set".

Please also check the use of "context" in "LifecycleManagerSession.js", "MatchMakerUtilities.js", "MatchMakerFrameworkTests.js", "Transformer.js" etc. In short, if you search the keyword "context" in the code base, many uses of it should be reconsidered.

fluid.log("lifecycleManager: Same preferences set as before (" + newPrefsSetName + "): no prefs set change");
return;
}
// Update the prefs set name immediately so we don't produce a model update from the lifecycleManager with a

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 14, 2019

Contributor

This comment is for line 1379. Please move it one line below.

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Nov 14, 2019

With the removal of "resetWithEnvReportTests", these config files used by the tests can be removed too:

  • tests/configs/gpii.tests.acceptance.resetWithEnvReport.config.json5
  • tests/configs/gpii.tests.acceptance.untrusted.resetWithEnvReport.config.json5
klown added 3 commits Nov 14, 2019
…porter

These configurations are defunct given the prior removal of the environment
reporter.
Removed references to the ContextManager and replaced some occurrences
of "context" with "preferences set" (or something similar).
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 15, 2019

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 15, 2019

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 15, 2019

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 18, 2019

@cindyli
I think I've replaced or removed all occurrences of "context manager" and variants thereof (ContextManager, contextManager, contextmanager, context manager, context_manager, etc) from universal, as well as many cases of "context".

In short, if you search the keyword "context" in the code base, many uses of it should be reconsidered.

I searched for all the instances of the string context in the code base in universal/gpii/node_modules/.... After filtering out:

  • "contexts":,
  • contexts,
  • fluid.contextAware,
  • contextValue (assumed that it's associated with fluid.contextAware),
  • context1

there are still 210 places where "context" is used. I started to evaluate them but it's not obvious what "context" means in each case -- it might be "context" in the sense of the ContextManager, or it might be something else. It's going to take a while to work through all of these. In fact, isn't this one of the tasks for GPII-4228: 'Replace "contexts:" from preferences and codebase' ?

FYI here is the command I used to find potential replacements for "context". If you know of something that would cut the size down even more, or give more accurate results, let me know:
grep -r -i context gpii/* | grep -v '\"contexts\":' | grep -v 'contexts:' | grep -v 'fluid.contextAware' | grep -v contextValue | grep -v 'context1'

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Nov 18, 2019

@klown, your grep command is much smarter than my method of searching "context" in IDE and going through the search result. ;)

I agree the replacement of "contexts: " should be deferred later so we can fix the backend DB data and the code base together. But as we agreed to replace "context -> prefs set" when applicable, I think it's worthwhile to clean up the code base on this part.

You've addressed the majority of the replacement. The last a couple:

  1. gpii/node_modules/matchMakerFramework/src/MatchMakerUtilities.js
  2. gpii/node_modules/ontologyHandler/src/ontologyHandler.js
  3. Let's take the chance to fix a historical typo: Context Integration Tests -> Journal Integration Tests
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 18, 2019

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 18, 2019

@cindyli

You've addressed the majority of the replacement. The last a couple:
...

Okay, I took care of those.

New grep command to ignore "deviceContext":
grep -r -i context gpii/* | grep -v '\"contexts\":' | grep -v 'contexts:' | grep -v 'fluid.contextAware' | grep -v contextValue | grep -i -v 'ContextAwareness' | grep -v 'context1' | grep -i -v 'deviceContext'

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 19, 2019

@cindyli I've double checked universal's tests, testData, and documentation sub-folders using my grep command, and investigated the occurrences of "context" it output. I did not see anything that needed changing to "prefsSet" or similar.

@amb26 amb26 merged commit 5e9c4f7 into GPII:master Nov 22, 2019
1 check passed
1 check passed
default Build finished.
Details
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.