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.

Sign up for GitHub

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

Jump to bottom

Jp okta 417473 #1145

Closed
wants to merge 1 commit into from
Closed

Jp okta 417473 #1145

wants to merge 1 commit into from

Conversation

jaredperreault-okta
Copy link
Contributor

No description provided.

@aarongranick-okta aarongranick-okta force-pushed the ag-OKTA-466786-fix-remediate branch from 0a8d702 to eaa7ee0 Compare March 22, 2022 18:09
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2022 •
edited
Loading

Codecov Report

Merging #1145 (6fd207f) into master (3e14603) will decrease coverage by 1.19%.
The diff coverage is 90.10%.

@@            Coverage Diff             @@
##           master    #1145      +/-   ##
==========================================
- Coverage   93.52%   92.33%   -1.20%     
==========================================
  Files         157      153       -4     
  Lines        4280     4434     +154     
  Branches      938      990      +52     
==========================================
+ Hits         4003     4094      +91     
- Misses        261      319      +58     
- Partials       16       21       +5
Impacted Files Coverage Δ
lib/OktaAuth.ts 89.83% <ø> (-0.07%) ⬇️
lib/idx/authenticator/Authenticator.ts 100.00% <ø> (ø)
lib/idx/idxState/v1/actionParser.ts 100.00% <ø> (ø)
lib/idx/idxState/v1/parsers.ts 100.00% <ø> (ø)
lib/idx/remediators/EnrollmentChannelData.ts 100.00% <ø> (ø)
lib/idx/remediators/ReEnrollAuthenticator.ts 25.00% <0.00%> (-5.00%) ⬇️
...dx/remediators/SelectAuthenticatorUnlockAccount.ts 88.23% <ø> (ø)
lib/idx/remediators/util.ts 66.66% <ø> (ø)
lib/idx/types/idx-js.ts 100.00% <ø> (ø)
lib/util/object.ts 87.50% <ø> (ø)
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e14603...6fd207f. Read the comment docs.

@aarongranick-okta aarongranick-okta force-pushed the ag-OKTA-466786-fix-remediate branch from c3a0109 to f416467 Compare March 23, 2022 17:56
@jaredperreault-okta jaredperreault-okta force-pushed the jp-OKTA-417473 branch 2 times, most recently from 7ed17eb to b292b32 Compare March 23, 2022 22:36
@jaredperreault-okta jaredperreault-okta changed the base branch from ag-OKTA-466786-fix-remediate to master March 23, 2022 22:37
jaredperreault-okta
jaredperreault-okta commented Mar 23, 2022
View reviewed changes
lib/idx/remediators/Base/Remediator.ts
@@ -182,6 +182,10 @@ export class Remediator {
if (Array.isArray(input)) {
input.forEach(i => inputs.push(i));
} else {
// guarantees field-level messages are passed back
if (messages) {
input.messages = messages;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

field-level messages fix

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember we also have a jira for this issue, we can close that one together.

@jaredperreault-okta jaredperreault-okta marked this pull request as ready for review March 23, 2022 22:39
shuowu
shuowu approved these changes Mar 24, 2022
View reviewed changes
Copy link
Contributor

@shuowu shuowu left a comment

Choose a reason for hiding this comment

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

Looks great! Please also add changelogs

lib/OktaAuth.ts
@@ -336,8 +335,6 @@ class OktaAuth implements OktaAuthInterface, SigninAPI, SignoutAPI {
unlockAccount: unlockAccount.bind(null, this),
};

setGlobalRequestInterceptor(createGlobalRequestInterceptor(this)); // to pass custom headers to IDX endpoints
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! this is what we need for the tree shaking next step

lib/idx/interact.ts
@@ -35,6 +35,20 @@ export interface InteractResponse {
meta: IdxTransactionMeta;
}

/* eslint-disable camelcase */
export interface InteractParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove export to keep this type private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this type is used in a few other places: https://github.com/okta/okta-auth-js/blob/jp-OKTA-417473/lib/types/api.ts#L298

lib/idx/remediators/Base/Remediator.ts
@@ -182,6 +182,10 @@ export class Remediator {
if (Array.isArray(input)) {
input.forEach(i => inputs.push(i));
} else {
// guarantees field-level messages are passed back
if (messages) {
input.messages = messages;
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember we also have a jira for this issue, we can close that one together.

test/spec/OktaAuth/constructor.ts Outdated
features: require('../../../lib/features'),
idxHeaders: require('../../../lib/idx/headers')
};
// const mocked = {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

test/spec/idx/idxState/unit/v1/generateIdxAction.test.js
/*
Doing a jest.mock('../../src/makeIdxState') has problems with jest.mock causing the test to hang
and spikes up the CPU usage for the current node process.
Alternative mocking approach: https://jestjs.io/docs/en/es6-class-mocks
*/
const mockMakeIdxState = jest.fn();
jest.mock('../../../../../../lib/idx/idx-js/v1/makeIdxState', () => ({
jest.mock('../../../../../../lib/idx/idxState/v1/makeIdxState', () => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe in a future refactor we can map ../../../../../../lib/ to an alias in jest config with moduleNameMapper

@jaredperreault-okta jaredperreault-okta force-pushed the jp-OKTA-417473 branch 3 times, most recently from 6fd207f to a1e8d9d Compare March 25, 2022 15:34
eng-prod-CI-bot-okta pushed a commit that referenced this pull request Mar 25, 2022
@jaredperreault-okta @eng-prod-CI-bot-okta
refactor: idx methods now use auth-js http client
513646f 
OKTA-417473
<<<Jenkins Check-In of Tested SHA: 4baca3d for eng_productivity_ci_bot_okta@okta.com>>>
Artifact: okta-auth-js
Files changed count: 76
PR Link: "#1145"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongranick-okta aarongranick-okta aarongranick-okta approved these changes

@shuowu shuowu shuowu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jaredperreault-okta @codecov-commenter @shuowu @aarongranick-okta @eng-prod-CI-bot-okta