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

[msal-common][msal-node][msal-browser] Merging configuration in common, name changes #1575

Merged
merged 12 commits into from May 5, 2020

Conversation

pkanher617
Copy link
Contributor

This PR merges the configuration object in the common package and removes SPA specific configuration from the common package. We have also renamed some classes to prevent namespaces conflicts.

@pkanher617 pkanher617 changed the base branch from msal-node-common-beta-updates to msal-node-base April 30, 2020 20:02
@@ -16,9 +16,9 @@ import {BaseClient} from "../../src/client/BaseClient";
import {AADServerParamKeys, GrantType} from "../../src/utils/Constants";
import {ClientTestUtils} from "./ClientTestUtils";

describe("DeviceCodeClient unit tests", () => {
describe.skip("DeviceCodeClient unit tests", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we skipping these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are broken currently, need to open a PR to fix them.

Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question about the unit tests. But otherwise looks fine

@pkanher617 pkanher617 changed the base branch from msal-node-base to dev May 1, 2020 23:27
@coveralls
Copy link

coveralls commented May 2, 2020

Coverage Status

Coverage increased (+0.2%) to 79.263% when pulling be8b3ef on merge-config into 693e7bc on dev.

Copy link
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:


expect(createTokenRequestBodySpy.returnValues[0]).to.contain(`${AADServerParamKeys.SCOPE}=${TEST_CONFIG.DEFAULT_GRAPH_SCOPE}%20${Constants.OPENID_SCOPE}%20${Constants.PROFILE_SCOPE}%20${Constants.OFFLINE_ACCESS_SCOPE}`);
expect(createTokenRequestBodySpy.returnValues[0]).to.contain(`${AADServerParamKeys.SCOPE}=${TEST_CONFIG.DEFAULT_GRAPH_SCOPE}%20${Constants.OFFLINE_ACCESS_SCOPE}`);
Copy link
Contributor

@sangonzal sangonzal May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RefreshToken client should return token with all three default scopes. We should see why that's not the case when we update tests in next PR

@pkanher617 pkanher617 merged commit 5d29ac2 into dev May 5, 2020
@pkanher617 pkanher617 deleted the merge-config branch August 1, 2020 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants