Convert MFA to Glimmer Component #1558
Conversation
WalkthroughThis update introduces a comprehensive refactor and modularization of the multi-factor authentication (MFA) management system within account settings. Several new Glimmer components and their templates are added to handle enabling, disabling, and switching MFA methods (email and app-based) via dedicated modals. The main MFA management component is simplified, delegating detailed flows and asynchronous logic to these new components. Testability is enhanced with additional data-test attributes and improved form handling. The update also introduces new SCSS files for modal-specific styling and removes redundant styles from the main MFA SCSS. A new integration test suite is included to thoroughly verify all MFA flows and UI states. Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying irenestaging with
|
| Latest commit: |
0e30001
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://68192957.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1654-convert-mfa-to-glimm.irenestaging.pages.dev |
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1654-Convert-MFA-to-Glimmer
|
| Run status |
|
| Run duration | 06m 21s |
| Commit |
|
| Committer | SmitGala |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
31
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an ipa file: 58061 |
Test Replay
Screenshots
|
|
SmitGala
force-pushed
the
PD-1654-Convert-MFA-to-Glimmer
branch
from
475eb26 to
b2c4c00
Compare
SmitGala
force-pushed
the
PD-1654-Convert-MFA-to-Glimmer
branch
from
b2c4c00 to
25f92ee
Compare
SmitGala
force-pushed
the
PD-1654-Convert-MFA-to-Glimmer
branch
from
25f92ee to
d440685
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (28)
app/components/account-settings/security/mfa-app-enable/index.hbs (1)
1-15: Structure looks good, but add accessibility attributesThe modal structure is well-organized, but consider adding accessibility attributes like
data-test-*selectors for testing andaria-labelfor the modal to improve accessibility.<AkModal @showHeader={{true}} @headerTitle={{t 'modalCard.mfaTitle.enable'}} @onClose={{this.closeAppEnable}} + data-test-mfa-app-enable-modal + aria-label={{t 'modalCard.mfaTitle.enable'}} > <AccountSettings::Security::MfaAppverify @secret={{this.mfaAppSecret}} @email={{this.user.email}} @otp={{this.appOTP}} @onContinue={{perform this.verifyAppOTP}} @onCancel={{this.closeAppEnable}} @waiting={{this.verifyAppOTP.isRunning}} @loading={{this.noMFAEnableApp.isRunning}} + data-test-mfa-app-verify /> </AkModal>app/components/account-settings/security/mfa-appverify/index.hbs (1)
63-84: Add data-test attribute for the cancel buttonThe continue button has a data-test attribute, but the cancel button is missing one. For consistent testability, add a data-test attribute to the cancel button as well.
<AkButton @variant='outlined' @color='neutral' {{on 'click' this.cancel}} + data-test-mfa-appverify-cancel-button > {{t 'cancel'}} </AkButton>app/components/account-settings/security/mfa-switch-to-email/index.hbs (1)
1-106: Consider adding OTP resend functionality for email verificationThe modal handles the OTP verification flow well, but there's no way for users to request a new OTP if they don't receive the email or if the code expires. Consider adding a "Resend Code" button during the email verification step.
{{#if this.showSwitchTOEmailEmailVerify}} <div local-class='info-box' class='px-3 py-2'> <AkTypography data-test-switch-to-email-enable-email> {{t 'modalCard.enableMFAEmail.description'}} <strong>{{this.user.email}}</strong> </AkTypography> + <AkButton + @variant='text' + @color='primary' + @size='small' + {{on 'click' (perform this.resendEmailOTP)}} + class='mt-2' + data-test-switch-to-email-resend-code + > + {{t 'resendCode'}} + </AkButton> </div>app/components/account-settings/security/mfa-email-enable/index.hbs (1)
16-21: Consider adding OTP resend functionalitySimilar to the switch-to-email component, consider adding a "Resend Code" button to allow users to request a new OTP if needed.
<div local-class='info-box' class='px-3 py-2'> <AkTypography data-test-mfa-email-enable-email-desc> {{t 'modalCard.enableMFAEmail.description'}} <strong>{{this.user.email}}</strong> </AkTypography> + <AkButton + @variant='text' + @color='primary' + @size='small' + {{on 'click' (perform this.resendEmailToken)}} + class='mt-2' + data-test-mfa-email-resend-code + > + {{t 'resendCode'}} + </AkButton> </div>app/components/account-settings/security/mfa-switch-to-app/index.hbs (2)
71-79: Consider adding a “Cancel” button to the email‑OTP state for UX parityIn the email‑verification state the footer exposes only a “Confirm” button. Every other state offers both “Confirm/Continue” and “Cancel”, so users lose a quick escape route once they begin entering the OTP.
Small change, but it keeps the modal behaviour consistent.
26-31: Accessibility nit – missing explicit label/id association for OTP field
AkTextFieldwill render a visually hidden<label>only if@labelis supplied.
The label is important for screen‑readers in an OTP form:<AkTextField - @label={{t 'modalCard.enableMFAEmail.enterCode'}} + @label={{t 'modalCard.enableMFAEmail.enterCode'}} @value={{this.emailOTP}} data-test-switch-to-app-disable-email-field />If
AkTextFieldalready links the label internally you can ignore this comment, otherwise please keep accessibility in mind.app/components/account-settings/security/mfa-disable/index.hbs (2)
12-17: Visual semantics – using “success” styling for a denial alert
alert-successis semantically misleading when the alert message is an error (“disableMFADenied”).
Consider changing the modifier to an error/warning variant (e.g.alert-error) or removing the success class.
46-49: Accessibility – provide a label for the OTP text fieldThe OTP
<AkTextField>lacks an explicit@label, which makes the form harder to use with assistive technologies.<AkTextField + @label={{t 'modalCard.disableMFA.enterOTPLabel'}} @value={{this.disableOTP}} data-test-mfa-disable-textfield />This also helps automated a11y tests pass.
tests/integration/components/account-setting/security/multi-factor-auth-test.js (2)
37-40: Minor: unnecessary async call may slow tests
await this.owner.lookup('service:organization').load();is executed for every test although the organization record is not used afterwards. Dropping theawait(or the entire call) can shave a few hundred ms off the suite.Not critical, but worth keeping in mind as the test matrix grows.
45-63: Repeatedserver.get('/v2/mfa' …)stubs may leak across tests
this.stubMfaStatere‑registers the same route on each invocation. Mirage keeps the last handler, so earlier stubs are shadowed but still accumulate. Over many tests this can make debugging difficult.Consider:
this.server.get('/v2/mfa', (schema, request) => { … }, { timing: 0, overwrite: true }); // or this.server.passthrough().get…or use
this.server.get('/v2/mfa').timing = 0to reset between tests.app/components/account-settings/security/multi-factor-auth/index.hbs (1)
111-116: Left‑over test hook looks accidental
data-test-button-aaais not self‑descriptive and does not follow the convention used elsewhere (e.g.data-test-mfa‑*). Consider renaming or removing to avoid brittle tests and future confusion.app/components/account-settings/security/mfa-switch-to-email/index.ts (3)
32-35: Boolean flag naming is inconsistent and hard to scan
showSwitchTOEmailAppVerify/showSwitchTOEmailEmailVerifymix the capital “TO” and read awkwardly. Prefer concise, homogeneous names such asisAppStep,isEmailStep, etc., to improve readability and avoid typo‑prone repetitions.
104-106: Leverage optional chaining for clarity
const otpMsg = errorObj.otp && errorObj.otp[0];can be simplified:-const otpMsg = errorObj.otp && errorObj.otp[0]; +const otpMsg = errorObj.otp?.[0];Produces the same result while reducing cognitive load.
🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
141-147: Duplicate error‑handling block – factor it outThe invalid‑OTP vs generic error logic is repeated in both verification tasks. Consider extracting a small helper to DRY the code and keep both tasks minimal.
🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-app-enable/index.ts (2)
64-66:noMFAEnableApp– misleading method nameThe prefix “no” implies a negative action; the task actually enables MFA. Rename to
enableMfaAppTask(or similar) for clarity and future maintainability.
110-118: Optional chaining + shared error helperSame comments as in the switch‑to‑email component—
errorObj.otp?.[0]and a shared “showOtpError” helper would cut duplication.🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-email-enable/index.ts (3)
78-79: Localise the “failed to send OTP” message
All other user‑facing strings come fromintl, but this one is hard‑coded English. Users on non‑English locales will see a mixed UI.- this.notify.error('Failed to send OTP. Please try again.'); + this.notify.error(this.intl.t('mfaEmailEnable.failedToSendOtp'));
107-109: Use optional chaining to simplify error parsingStatic analysis already flagged this. Optional chaining is shorter and avoids the manual fallback object.
- const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = (error as AjaxError).payload?.otp?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
39-42: Naming consistency
tsomethingWentWrongbreaks camel‑case used bytEnterOTP/tInvalidOTP. Rename totSomethingWentWrongfor readability.app/components/account-settings/security/mfa-switch-to-app/index.ts (2)
91-92: Reuse optional‑chaining pattern for brevity- const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = (error as AjaxError).payload?.otp?.[0];Apply to all three duplicated locations.
Also applies to: 129-130, 165-166
🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
48-57: Minor typo: capitalise translation helper
tsomethingWentWrong→tSomethingWentWrongfor consistency.app/components/account-settings/security/mfa-disable/index.ts (2)
110-114: Adopt optional chaining in error handlingCleaner & matches project‑wide style.
- if (payload.otp && payload.otp.length) { + if (payload?.otp?.length) {and
- const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = (error as AjaxError).payload?.otp?.[0];Also applies to: 145-146
🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
35-38: Consistent camel‑casing for translation vars
Same casing nit as the other components:tsomethingWentWrong→tSomethingWentWrong.app/components/account-settings/security/multi-factor-auth/index.ts (5)
20-24: Service declarations are correct, but consider “private” visibility
Adding theprivatekeyword (orreadonly) to injected services can prevent accidental reassignment.
26-31: Many tracked booleans – consider consolidating modal state
A single enum/union discriminated union will scale better than six individual flags:-@tracked showEmailEnableModal = false; -@tracked showAppEnableModal = false; -@tracked showSwitchToEmailModal = false; -@tracked showSwitchToAppModal = false; -@tracked showMFADisableModal = false; +@tracked activeModal: 'email-enable' | 'app-enable' | + 'switch-email' | 'switch-app' | + 'disable' | null = null;This removes the need to manually reset competing flags.
116-118: Minor naming nit
noMFAEnableEmailDialog()could be shortened toopenEmailEnable()for parity with the other helpers.
125-128: Typo: “Model” vs “Modal”-@action -showAppOTPModel() { +@action +showAppOTPModal() {Rename for clarity; same applies to the template invocation.
155-163: Rename ambiguous helpersstaShow/staClose-@action -staShow() { - this.showSwitchToAppModal = true; -} - -@action -staClose() { - this.showSwitchToAppModal = false; -} +@action +showSwitchToApp() { + this.showSwitchToAppModal = true; +} + +@action +closeSwitchToApp() { + this.showSwitchToAppModal = false; +}Clear names self‑document the intent and ease template usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app/components/account-settings/security/mfa-app-enable/index.hbs(1 hunks)app/components/account-settings/security/mfa-app-enable/index.ts(1 hunks)app/components/account-settings/security/mfa-appverify/index.hbs(1 hunks)app/components/account-settings/security/mfa-appverify/index.ts(2 hunks)app/components/account-settings/security/mfa-disable/index.hbs(1 hunks)app/components/account-settings/security/mfa-disable/index.scss(1 hunks)app/components/account-settings/security/mfa-disable/index.ts(1 hunks)app/components/account-settings/security/mfa-email-enable/index.hbs(1 hunks)app/components/account-settings/security/mfa-email-enable/index.scss(1 hunks)app/components/account-settings/security/mfa-email-enable/index.ts(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.hbs(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.scss(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.ts(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.hbs(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.scss(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.ts(1 hunks)app/components/account-settings/security/multi-factor-auth/index.hbs(6 hunks)app/components/account-settings/security/multi-factor-auth/index.scss(0 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(2 hunks)tests/integration/components/account-setting/security/multi-factor-auth-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/components/account-settings/security/multi-factor-auth/index.scss
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/components/account-settings/security/mfa-switch-to-email/index.ts (2)
app/models/user.ts (1)
UserModel(14-198)app/services/ajax.ts (2)
IreneAjaxService(143-385)AjaxError(33-41)
app/components/account-settings/security/mfa-email-enable/index.ts (2)
app/models/user.ts (1)
UserModel(14-198)app/services/ajax.ts (2)
IreneAjaxService(143-385)AjaxError(33-41)
tests/integration/components/account-setting/security/multi-factor-auth-test.js (6)
app/components/account-settings/security/mfa-app-enable/index.ts (1)
user(55-57)app/components/account-settings/security/mfa-email-enable/index.ts (1)
user(54-56)app/components/account-settings/security/mfa-disable/index.ts (1)
user(50-52)app/components/account-settings/security/multi-factor-auth/index.ts (1)
user(70-72)app/components/account-settings/security/mfa-switch-to-app/index.ts (1)
user(59-61)app/components/account-settings/security/mfa-switch-to-email/index.ts (1)
user(58-60)
🪛 Biome (1.9.4)
app/components/account-settings/security/mfa-switch-to-email/index.ts
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-app-enable/index.ts
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-switch-to-app/index.ts
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 165-165: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-disable/index.ts
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-email-enable/index.ts
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (25)
app/components/account-settings/security/mfa-email-enable/index.scss (1)
1-3: Clean and effective styling for the info box.The style is properly implemented using CSS variables for theming, which follows good practices for maintaining consistent UI appearance across the application.
app/components/account-settings/security/mfa-switch-to-email/index.scss (1)
1-3: Consistent styling with other MFA components.This styling matches the pattern used in other MFA components, ensuring visual consistency across the MFA flow.
app/components/account-settings/security/mfa-disable/index.scss (1)
1-8: Well-structured alert styling with appropriate theming variables.The success alert styling is correctly implemented with separate variables for border, background, and text colors, allowing for consistent theming and potential dark mode support.
app/components/account-settings/security/mfa-switch-to-app/index.scss (1)
1-3: Consistent info box styling across MFA components.The styling maintains visual consistency with other MFA components that use the same info box pattern.
app/components/account-settings/security/mfa-appverify/index.ts (2)
18-18: Good addition of optional loading state.The optional loading property is properly typed with
?modifier, following TypeScript best practices for optional properties.
49-51: Improved form handling with proper event prevention.The change correctly updates the
continuemethod to handle the event object and prevent default form submission behavior. This is an important enhancement for proper form handling and prevents page reloads.app/components/account-settings/security/mfa-appverify/index.hbs (3)
2-31: Great addition of loading state skeleton UIThe skeleton UI implementation is well-structured and provides a good visual representation of the loading state. The use of AkStack for layout and AkSkeleton components matches the actual content structure.
41-42: Good use of data-test attributes for testingAdding data-test attributes improves testability of the component. This makes it easier to write and maintain integration tests.
Also applies to: 48-49, 60-61, 72-73
52-85: Well-structured form with proper event handlingThe form is well-organized with proper event handling. The submit event is bound to the continue action, and buttons are properly grouped in a flex container with appropriate spacing and alignment.
app/components/account-settings/security/mfa-switch-to-email/index.hbs (2)
31-50: Good use of info-box for email verification instructionsThe info-box styling for email verification instructions provides clear visual separation. The form structure with proper aria-label and test attributes is well-implemented.
54-104: Well-structured conditional footer with appropriate button statesThe footer section with conditional rendering of different button sets based on the current step is well-implemented. Loading states are properly bound to the running state of async tasks.
app/components/account-settings/security/mfa-email-enable/index.hbs (4)
1-6: Good use of modal properties and test attributesThe modal setup with header, title, and close action is well-implemented. The data-test attribute on the modal helps with testing.
8-13: Well-structured conditional content based on flow stateThe conditional rendering based on
showEmailSendConfirmandshowEmailOTPEnterstates provides a clear multi-step UX. The use of strong tags for the email helps with visual emphasis.Also applies to: 15-34
23-33: Form handling with proper event binding and accessibilityThe form is well-structured with proper event binding to the
verifyEmailOTPaction. The aria-label and test attributes improve accessibility and testability.
38-72: Well-structured footer with appropriate button statesThe footer implementation with conditional buttons based on the current step is clean. The loading states are properly bound to the running state of async tasks.
app/components/account-settings/security/multi-factor-auth/index.hbs (1)
88-90:showMFADisablehandler: double‑check that the action still existsThe template wires the “Disable” button to
this.showMFADisable, but the backing Glimmer class was massively simplified in this PR. Please verify that a method with that exact name is still exported; otherwise the click will throw at run‑time.app/components/account-settings/security/mfa-email-enable/index.ts (1)
34-36: UnusedsecretinTokenData
emailTokenData.secretis never referenced. If the template doesn’t use it, consider pruning the field to avoid mental overhead.app/components/account-settings/security/mfa-disable/index.ts (1)
62-73:enabledMethodcan resolve toNONEIf both
isEmailMFAEnabledandisAppMFAEnabledare false we sendmethod: NONE, which the API likely rejects. Consider guarding:if (this.enabledMethod === ENUMS.MFA_METHOD.NONE) { this.notify.error(this.intl.t('mfaDisable.noMethodEnabled')); return false; }app/components/account-settings/security/multi-factor-auth/index.ts (7)
1-8: Imports look good and are minimal
The imported symbols are necessary and used; no issues spotted here.
14-18: Well‑typed public signature
Defining an explicit signature object keeps the component API clear. 👍
43-47:findAll('mfa')+slice()converts to a plain array but loses live updates
If other parts of the app push new MFA records into the store, this array won’t react.
Consider:this.mfas = [...mfaResponse.toArray()];or keep the
ManyArrayand access.slice()only in the getter.
70-71: Simple getter – no issues
74-75:isMFAEnabled– concise & readable
78-81: Email‑MFA check is fine
84-87: App‑MFA check is fine
app/components/account-settings/security/mfa-app-enable/index.hbs
Outdated
Show resolved
Hide resolved
app/components/account-settings/security/mfa-switch-to-email/index.hbs
Outdated
Show resolved
Hide resolved
app/components/account-settings/security/mfa-switch-to-app/index.hbs
Outdated
Show resolved
Hide resolved
SmitGala
force-pushed
the
PD-1654-Convert-MFA-to-Glimmer
branch
from
d440685 to
295c7ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
app/components/account-settings/security/mfa-switch-to-email/index.ts (1)
1-11:NotificationServiceimport still missing – TS will fail to compile
The class injects@service('notifications') declare notify: NotificationService;but the symbol has no import. Re-add the import to satisfy the TS compiler.import { service } from '@ember/service'; ... +import NotificationService from 'ember-cli-notifications/services/notifications';app/components/account-settings/security/mfa-app-enable/index.ts (2)
1-11: MissingNotificationServiceimportSame compilation issue as previously highlighted; bring the type into scope:
import { service } from '@ember/service'; ... +import NotificationService from 'ember-cli-notifications/services/notifications';
52-53: Triggering an async task in the constructor may run before DOM is readyFiring
this.noMFAEnableApp.perform()insideconstructorrisks racing against
did-inserttiming, potentially leading to state updates before the template
renders. Move the invocation to an@actionwired to
{{did-insert}}, or call it fromwillDestroysafe lifecycle hook instead.app/components/account-settings/security/mfa-email-enable/index.ts (1)
85-100:⚠️ Potential issueGuard against missing token before verifying OTP
emailTokenDatacan still benullwhen the previous request failed or was cancelled.
Attempting to hit/v2/mfawith an emptytokencauses an unnecessary extra round-trip and a confusing 400 for the user.- const token = this.emailTokenData?.token; + const token = this.emailTokenData?.token; + if (!token) { + this.notify.error(this.tsomethingWentWrong); + return false; + }app/components/account-settings/security/mfa-switch-to-app/index.ts (1)
82-99: Success path leaves the modal in “waiting” state
No change since the last reviewWhen the initial
staInitialEmailrequest succeeds, no UI flag is flipped, so the user sees the same confirm step indefinitely. Forward them tostaShowVerifyApp()(or close the modal) just like the failure-to-OTP branch.🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/multi-factor-auth/index.ts (2)
34-41: Fire-and-forget task started in constructor still lacks error handlingIf
store.findAll('mfa')rejects, the exception is swallowed and the component renders an empty state silently. Catch and surface the error (toast / logger) or expose the task’slastErroredstate to the template.
90-98: Type-safety: acceptENUMS.MFA_METHODinstead of parsing stringsThe switch statement still coerces
methodwithparseInt, re-introducing the enum → string mismatch flagged earlier. Pass the enum directly from the template and drop the coercion.-enableMFA = task(async (method: string) => { - switch (parseInt(method)) { +enableMFA = task(async (method: ENUMS.MFA_METHOD) => { + switch (method) {
🧹 Nitpick comments (12)
tests/integration/components/account-setting/security/multi-factor-auth-test.js (2)
84-84: Remove vestigialdebuggerstatementEven though the line is commented-out, leaving breadcrumb
debugger;statements in committed code makes grepping noisy and may be re-enabled accidentally. Please delete the line.
130-136: Consider lifting magic OTP value into a constant/helperThe literal
'000000'appears many times across the test suite. Definingconst MOCK_OTP = '000000';at the top (or a dedicated helper) will improve readability and allow easier future changes.app/components/account-settings/security/mfa-switch-to-email/index.ts (2)
100-108: Prefer optional chaining to silence lint warning & avoid undefined accessStatic analysis suggests replacing the verbose guard with optional chaining:
- const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = (error as AjaxError).payload?.otp?.[0];Same applies to the analogous block at lines 141-147.
🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
32-35: Naming consistency:showSwitchTOEmailEmailVerifylooks like a typoThe
TOin the middle breaks camel-casing and may confuse template authors.
Recommend renaming toshowSwitchToEmailEmailVerifyfor consistency (update all references).app/components/account-settings/security/mfa-app-enable/index.ts (2)
110-116: Replace verbose null-check with optional chaining- const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = (error as AjaxError).payload?.otp?.[0];This matches the Biome hint and slightly reduces complexity.
🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
38-41: Typo in property nametsomethingWentWrongRename to
tSomethingWentWrongto follow camel-case convention used by the other translation fields.app/components/account-settings/security/mfa-email-enable/index.ts (2)
74-79: Internationalise & surface server-side error detailsThe end-user won’t see a translated message here, while every other toast relies on
intl.
Prefer using an i18n key and, if available, showerror.payload.messageto help support diagnose issues quicker.- this.notify.success(`OTP sent to ${this.user?.email}`); + this.notify.success( + this.intl.t('mfa.email.otpSent', { email: this.user?.email }) +); ... - this.notify.error('Failed to send OTP. Please try again.'); + const msg = + (error as AjaxError).payload?.message ?? + this.tsomethingWentWrong; + this.notify.error(msg);
107-115: DRY-up error branching with optional chainingRepeated
const otpMsg = errorObj.otp && errorObj.otp[0];can be simplified and made safer:- const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj?.otp?.[0];Purely cosmetic but improves readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-switch-to-app/index.ts (1)
128-133: Minor: optional chaining simplifies payload lookup- const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj?.otp?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-disable/index.ts (2)
109-115: Use optional chaining for terser payload checks- if (payload.otp && payload.otp.length) { + if (payload?.otp?.length) {🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
145-150: Simplify OTP extraction- const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj?.otp?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
32-32:mfaEndpointis declared but never usedUnused members add maintenance burden and can confuse future readers—consider removing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app/components/account-settings/security/mfa-app-enable/index.hbs(1 hunks)app/components/account-settings/security/mfa-app-enable/index.ts(1 hunks)app/components/account-settings/security/mfa-appverify/index.hbs(1 hunks)app/components/account-settings/security/mfa-appverify/index.ts(2 hunks)app/components/account-settings/security/mfa-disable/index.hbs(1 hunks)app/components/account-settings/security/mfa-disable/index.scss(1 hunks)app/components/account-settings/security/mfa-disable/index.ts(1 hunks)app/components/account-settings/security/mfa-email-enable/index.hbs(1 hunks)app/components/account-settings/security/mfa-email-enable/index.scss(1 hunks)app/components/account-settings/security/mfa-email-enable/index.ts(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.hbs(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.scss(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.ts(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.hbs(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.scss(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.ts(1 hunks)app/components/account-settings/security/multi-factor-auth/index.hbs(6 hunks)app/components/account-settings/security/multi-factor-auth/index.scss(0 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(2 hunks)tests/integration/components/account-setting/security/multi-factor-auth-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/components/account-settings/security/multi-factor-auth/index.scss
✅ Files skipped from review due to trivial changes (2)
- app/components/account-settings/security/mfa-switch-to-app/index.scss
- app/components/account-settings/security/mfa-app-enable/index.hbs
🚧 Files skipped from review as they are similar to previous changes (10)
- app/components/account-settings/security/mfa-email-enable/index.scss
- app/components/account-settings/security/mfa-switch-to-email/index.scss
- app/components/account-settings/security/mfa-disable/index.scss
- app/components/account-settings/security/mfa-appverify/index.ts
- app/components/account-settings/security/mfa-switch-to-app/index.hbs
- app/components/account-settings/security/mfa-appverify/index.hbs
- app/components/account-settings/security/mfa-email-enable/index.hbs
- app/components/account-settings/security/mfa-switch-to-email/index.hbs
- app/components/account-settings/security/multi-factor-auth/index.hbs
- app/components/account-settings/security/mfa-disable/index.hbs
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/components/account-settings/security/mfa-disable/index.ts (2)
app/models/user.ts (1)
UserModel(14-198)app/services/ajax.ts (2)
IreneAjaxService(143-385)AjaxError(33-41)
app/components/account-settings/security/mfa-switch-to-app/index.ts (2)
app/models/user.ts (1)
UserModel(14-198)app/services/ajax.ts (2)
IreneAjaxService(143-385)AjaxError(33-41)
🪛 Biome (1.9.4)
app/components/account-settings/security/mfa-app-enable/index.ts
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-disable/index.ts
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-email-enable/index.ts
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-switch-to-app/index.ts
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 165-165: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-switch-to-email/index.ts
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
tests/integration/components/account-setting/security/multi-factor-auth-test.js (1)
167-176: Addawait settled()after OTP confirmation to avoid race conditionsThe component triggers ember-concurrency tasks and Mirage requests once the confirm button is clicked. Immediately asserting for DOM updates can yield flakiness because the promises may not have resolved yet. Insert
await settled();(orawait waitUntil(...)) after theclickto guarantee the UI has re-rendered.await click('[data-test-mfa-email-enable-confirm-button]'); + await settled();
app/components/account-settings/security/mfa-switch-to-email/index.ts
Outdated
Show resolved
Hide resolved
tests/integration/components/account-setting/security/multi-factor-auth-test.js
Outdated
Show resolved
Hide resolved
SmitGala
force-pushed
the
PD-1654-Convert-MFA-to-Glimmer
branch
from
295c7ac to
0e30001
Compare
SmitGala
force-pushed
the
PD-1654-Convert-MFA-to-Glimmer
branch
from
0e30001 to
908889b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
app/components/account-settings/security/mfa-switch-to-email/index.ts (2)
27-30:⚠️ Potential issueMissing
NotificationServiceimport breaks TS compilation.
@service('notifications') declare notify: NotificationService;requiresNotificationServiceto be in scope.Add an explicit import:
import type IntlService from 'ember-intl/services/intl'; +import type NotificationService from 'ember-cli-notifications/services/notifications'; import ENUMS from 'irene/enums';
116-134:⚠️ Potential issueGuard against missing
tokenbefore verifying e-mail OTP.
tokencan beundefinedif the first step never succeeded or was cancelled.const otp = this.emailOTP; const token = this.emailTokenData?.token; + if (!token) { + this.notify.error(this.tsomethingWentWrong); + return false; + } if (!otp) { this.notify.error(this.tEnterOTP); return false; }app/components/account-settings/security/mfa-app-enable/index.ts (2)
27-30:⚠️ Potential issueMissing
NotificationServiceimport.Same missing import as in other components.
import type IntlService from 'ember-intl/services/intl'; +import type NotificationService from 'ember-cli-notifications/services/notifications'; import ENUMS from 'irene/enums';
52-53: 🛠️ Refactor suggestionRunning an async task in the constructor can cause race conditions.
Calling
this.noMFAEnableApp.perform()inside the constructor fires the task before the component's lifecycle hooks are ready.Move the invocation to a lifecycle hook or
did-insertmodifier in the template:constructor( owner: Owner, args: AccountSettingsSecurityMfaAppEnableSignature['Args'] ) { super(owner, args); this.tEnterOTP = this.intl.t('enterOTP'); this.tInvalidOTP = this.intl.t('invalidOTP'); this.tsomethingWentWrong = this.intl.t('somethingWentWrong'); - - this.noMFAEnableApp.perform(); } + @action + didInsert() { + this.noMFAEnableApp.perform(); + }Then in your template, add:
app/components/account-settings/security/mfa-email-enable/index.ts (2)
27-30:⚠️ Potential issueMissing
NotificationServiceimport.Same missing import as in other components.
import type IntlService from 'ember-intl/services/intl'; +import type NotificationService from 'ember-cli-notifications/services/notifications'; import ENUMS from 'irene/enums';
86-93:⚠️ Potential issueGuard against missing
tokenbefore verifying email OTP.Add a check for the token's existence before attempting to use it in the API call.
const otp = this.emailOTP; const token = this.emailTokenData?.token; +if (!token) { + this.notify.error(this.tsomethingWentWrong); + return false; +} if (!otp) { this.notify.error(this.tEnterOTP); return false; }app/components/account-settings/security/mfa-switch-to-app/index.ts (2)
82-100: Success path handling is missing instaInitialEmailtaskWhen the POST request succeeds, the UI state flags remain at their initial values, causing the modal to stay in limbo with no follow-up steps shown to the user.
Apply this fix to transition to the app verification screen after successful initial request:
await this.ajax.post(this.mfaEndpoint, { data: { method: ENUMS.MFA_METHOD.TOTP, }, }); + + // Proceed to app verification when server accepts without OTP + this.staShowVerifyApp();🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
141-149: Token presence not validated before final OTP submissionThe code doesn't validate if
this.mfaAppTokenexists before submitting it, which could cause a hard-to-explain backend 400 error.Add validation to check for token presence:
const otp = this.appOTP; const token = this.mfaAppToken; + if (!token) { + this.notify.error(this.tsomethingWentWrong); + return false; + } if (!otp) { this.notify.error(this.tEnterOTP);app/components/account-settings/security/mfa-disable/index.ts (1)
85-97:continueDisableMFAshould abort when no MFA method is enabledThe method doesn't check if both MFA methods are disabled before proceeding, which could lead to backend validation errors.
Add validation to abort early if no MFA method is enabled:
continueDisableMFA = task(async () => { try { + if (!this.isEmailMFAEnabled && !this.isAppMFAEnabled) { + this.notify.error(this.tsomethingWentWrong); + return; + } + if (this.isEmailMFAEnabled) { await this.sendDisableMFAOTPEmail.perform(); }app/components/account-settings/security/multi-factor-auth/index.ts (2)
34-41: Add error handling toloadMfaDatatask in constructorThe task is fire-and-forget with no error handling, which could result in a silent failure where the UI may stay blank.
Add error handling to provide feedback when data loading fails:
constructor( owner: Owner, args: AccountSettingsSecurityMultiFactorAuthSignature['Args'] ) { super(owner, args); - this.loadMfaData.perform(); + this.loadMfaData.perform().catch((e) => { + console.error('Could not load MFA records', e); + // Consider adding notification service call here + }); }
90-112: Improve type safety and method naming inenableMFAtaskThe method has two issues:
methodis typed as string and immediately coerced withparseInt()staShow()is not descriptive enoughApply these improvements:
- enableMFA = task(async (method: string) => { - switch (parseInt(method)) { + enableMFA = task(async (method: ENUMS.MFA_METHOD) => { + switch (method) { case ENUMS.MFA_METHOD.HOTP: if (this.isMFAEnabled) { this.showSwitchToEmail(); } else { this.noMFAEnableEmailDialog(); } break; case ENUMS.MFA_METHOD.TOTP: if (this.isMFAEnabled) { - this.staShow(); + this.showSwitchToApp(); } else { this.showAppOTPModel(); } break; default: break;Also, rename the method from
staShowtoshowSwitchToAppfor consistency and clarity:@action - staShow() { + showSwitchToApp() { this.showSwitchToAppModal = true; }
🧹 Nitpick comments (12)
app/components/account-settings/security/mfa-switch-to-email/index.ts (3)
32-34: Fix inconsistent variable naming case.The property
showSwitchTOEmailEmailVerifyhas inconsistent capitalization in "TO" which should be "To" to match the naming pattern of other variables.@tracked showSwitchToEmailConfirm = true; @tracked showSwitchToEmailAppVerify = false; -@tracked showSwitchTOEmailEmailVerify = false; +@tracked showSwitchToEmailEmailVerify = false;
81-114: Refactor to use optional chaining for error handling.In the error handling logic, use optional chaining to simplify the code and avoid potential null references.
try { this.emailTokenData = await this.ajax.post<TokenData>(this.mfaEndpoint, { data: { method: ENUMS.MFA_METHOD.HOTP, otp: otp || '', }, }); this.showSwitchToEmailVerifyEmail(); return true; } catch (error) { const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj?.otp?.[0]; if (otpMsg) { this.notify.error(this.tInvalidOTP); return; } this.notify.error(this.tsomethingWentWrong); }🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
141-148: Refactor to use optional chaining for error handling.Similar to the earlier task, use optional chaining for cleaner error handling.
catch (error) { const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj?.otp?.[0]; if (otpMsg) { this.notify.error(this.tInvalidOTP); } else { this.notify.error(this.tsomethingWentWrong); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-app-enable/index.ts (1)
111-118: Refactor to use optional chaining for error handling.Simplify error handling with optional chaining.
catch (error) { const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj?.otp?.[0]; if (otpMsg) { this.notify.error(this.tInvalidOTP); } else { this.notify.error(this.tsomethingWentWrong); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-email-enable/index.ts (2)
63-66: Add@actiondecorator to event handler method.The
showEmailOTPDialogmethod appears to be an action handler but is missing the@actiondecorator.+@action showEmailOTPDialog() { this.showEmailSendConfirm = false; this.showEmailOTPEnter = true; }
107-114: Refactor to use optional chaining for error handling.Use optional chaining for more concise and safer error handling.
catch (error) { const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj?.otp?.[0]; if (otpMsg) { this.notify.error(this.tInvalidOTP); } else { this.notify.error(this.tsomethingWentWrong); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-switch-to-app/index.ts (3)
91-91: Use optional chaining for safer property accessThe code should use optional chaining to safely access nested properties.
- const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj.otp?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
129-129: Use optional chaining for safer property accessThe code should use optional chaining to safely access nested properties.
- const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj.otp?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
165-165: Use optional chaining for safer property accessThe code should use optional chaining to safely access nested properties.
- const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj.otp?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 165-165: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-disable/index.ts (2)
111-111: Use optional chaining for safer property accessThe code should use optional chaining to safely access nested properties.
- if (payload.otp && payload.otp.length) { + if (payload.otp?.length) {🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
145-145: Use optional chaining for safer property accessThe code should use optional chaining to safely access nested properties.
- const otpMsg = errorObj.otp && errorObj.otp[0]; + const otpMsg = errorObj.otp?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
156-158: Rename method for better clarity and consistencyThe method name
staShowis not descriptive. It should follow the naming pattern of other show methods.@action - staShow() { + showSwitchToApp() { this.showSwitchToAppModal = true; }Also update any references to this method throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (20)
app/components/account-settings/security/mfa-app-enable/index.hbs(1 hunks)app/components/account-settings/security/mfa-app-enable/index.ts(1 hunks)app/components/account-settings/security/mfa-appverify/index.hbs(1 hunks)app/components/account-settings/security/mfa-appverify/index.ts(2 hunks)app/components/account-settings/security/mfa-disable/index.hbs(1 hunks)app/components/account-settings/security/mfa-disable/index.scss(1 hunks)app/components/account-settings/security/mfa-disable/index.ts(1 hunks)app/components/account-settings/security/mfa-email-enable/index.hbs(1 hunks)app/components/account-settings/security/mfa-email-enable/index.scss(1 hunks)app/components/account-settings/security/mfa-email-enable/index.ts(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.hbs(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.scss(1 hunks)app/components/account-settings/security/mfa-switch-to-app/index.ts(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.hbs(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.scss(1 hunks)app/components/account-settings/security/mfa-switch-to-email/index.ts(1 hunks)app/components/account-settings/security/multi-factor-auth/index.hbs(6 hunks)app/components/account-settings/security/multi-factor-auth/index.scss(0 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(2 hunks)tests/integration/components/account-setting/security/multi-factor-auth-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/components/account-settings/security/multi-factor-auth/index.scss
🚧 Files skipped from review as they are similar to previous changes (12)
- app/components/account-settings/security/mfa-switch-to-email/index.scss
- app/components/account-settings/security/mfa-switch-to-app/index.scss
- app/components/account-settings/security/mfa-email-enable/index.scss
- app/components/account-settings/security/mfa-app-enable/index.hbs
- app/components/account-settings/security/mfa-disable/index.scss
- app/components/account-settings/security/mfa-switch-to-app/index.hbs
- app/components/account-settings/security/mfa-appverify/index.ts
- app/components/account-settings/security/mfa-appverify/index.hbs
- app/components/account-settings/security/mfa-email-enable/index.hbs
- app/components/account-settings/security/mfa-switch-to-email/index.hbs
- app/components/account-settings/security/multi-factor-auth/index.hbs
- app/components/account-settings/security/mfa-disable/index.hbs
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/integration/components/account-setting/security/multi-factor-auth-test.js (6)
app/components/account-settings/security/mfa-app-enable/index.ts (1)
user(55-57)app/components/account-settings/security/mfa-email-enable/index.ts (1)
user(54-56)app/components/account-settings/security/mfa-disable/index.ts (1)
user(50-52)app/components/account-settings/security/mfa-switch-to-app/index.ts (1)
user(59-61)app/components/account-settings/security/multi-factor-auth/index.ts (1)
user(70-72)app/components/account-settings/security/mfa-switch-to-email/index.ts (1)
user(58-60)
app/components/account-settings/security/mfa-switch-to-app/index.ts (2)
app/models/user.ts (1)
UserModel(14-198)app/services/ajax.ts (2)
IreneAjaxService(143-385)AjaxError(33-41)
🪛 Biome (1.9.4)
app/components/account-settings/security/mfa-switch-to-app/index.ts
[error] 91-91: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 129-129: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 165-165: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-app-enable/index.ts
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-disable/index.ts
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-email-enable/index.ts
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/account-settings/security/mfa-switch-to-email/index.ts
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Cloudflare Pages
🔇 Additional comments (11)
tests/integration/components/account-setting/security/multi-factor-auth-test.js (11)
1-9: Well-organized test setup with appropriate imports.The test file includes all necessary imports for Ember testing, Mirage for API mocking, and internationalization support. The structure follows best practices for integration tests.
10-20: Good use of service stubs for testing isolation.The
NotificationsStubandOrganizationMeStubservices effectively isolate the component tests from external dependencies, allowing for controlled testing of notification messages and permission checks.Also applies to: 22-27
45-63: Well-implemented MFA state stubbing.The
stubMfaStatehelper function provides a flexible way to configure different MFA states for testing various scenarios. The implementation correctly mocks the API response structure.
79-97: Thorough basic rendering test.This test verifies all essential UI elements are present in their initial state, with appropriate data test selectors and text content verification.
99-126: Properly tests permission-based UI variations.The test correctly verifies that UI elements adapt when the user doesn't have permission to disable MFA, showing appropriate messaging.
128-174: Comprehensive email MFA enablement test.The test successfully verifies the complete flow of enabling email-based MFA, including modal transitions, form interactions, and final UI state updates.
176-227: Complete email MFA disablement test.This test covers the full disablement flow for email-based MFA, including the confirmation step, OTP verification, and final UI state validation.
229-258: Thorough app-based MFA enablement test.The test properly validates the authenticator app MFA enablement flow, including QR code presence, OTP verification, and UI state transitions.
260-311: Complete app-based MFA disablement test.This test thoroughly covers disabling app-based MFA, including permission checks, OTP verification, and UI state validation.
313-383: Comprehensive testing of MFA method switching from app to email.The test effectively verifies the complete flow of switching from app-based to email-based MFA, including confirmation, app verification, email verification, and final state validation.
385-466: Thorough testing of MFA method switching from email to app.This test covers the complete flow of switching from email-based to app-based MFA, including error handling for invalid OTP responses.
No description provided.