Convert Organization Compoonents to ts #1530
Conversation
WalkthroughThe changes remove legacy JavaScript components and introduce or update equivalent components in TypeScript. Multiple components are enhanced with explicit type annotations, improved service declarations using the declare syntax, refined asynchronous tasks with better error handling, and proper Glint environment module registrations. Overall, the updates aim to increase type safety and maintain consistency across the Ember application. Changes
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
app/components/organization-mfa/index.ts (2)
10-15: Consider renaming the interface for clarity.
The interface is named "OrganizationEmailDomainSignature," but the component is "OrganizationMfaComponent." Renaming the interface to match the component's purpose (e.g., "OrganizationMfaSignature") would improve consistency.
46-46: Remove unused parameter.
The first function parameter "_" is not utilized. Consider removing it or renaming it to underscore only to clarify intentional unused arguments.app/components/sso-settings/index.ts (1)
140-143: Use 'const' instead of 'let'.
Static analysis recommends using 'const' for variables that are never reassigned.Apply this diff to line 141:
- let idpFormData = new FormData(); + const idpFormData = new FormData();🧰 Tools
🪛 ESLint
[error] 141-141: 'idpFormData' is never reassigned. Use 'const' instead.
(prefer-const)
app/components/organization/settings/index.ts (1)
31-33: Consider adding JSDoc documentation.While the component implementation is correct, adding JSDoc comments would improve code documentation and IDE support.
+/** + * Component for managing organization settings. + */ export default class OrganizationSettingsComponent extends Component<OrganizationSettingsSignature> { + /** Service for accessing current user information */ @service declare me: MeService; }app/components/organization-email-domain/index.ts (2)
42-62: Consider improving error handling in tasks.The error handling could be more specific by typing the expected error and using a custom error type.
- addDomain = task(async () => { + addDomain = task<void, Error>(async () => { const domainName = this.tDomain; const domainInfo = this.store.createRecord('organization-email-domain', { domainName, }); try { await domainInfo.save(); - } catch (err) { + } catch (err: unknown) { + const error = err as Error; domainInfo.rollbackAttributes(); - this.notify.error(parseError(err, domainName)); + this.notify.error(parseError(error, domainName)); } });
26-27: Consider using a more specific type for constructor args.The constructor args type could be more specific than
unknown.- constructor(owner: unknown, args: OrganizationEmailDomainSignature['Args']) { + constructor( + owner: EmberRuntime, + args: OrganizationEmailDomainSignature['Args'] + ) {app/components/regulatory-preference-organization/index.ts (1)
93-113: Consider improving type safety in the saveReportPreference task.The task could be improved by:
- Defining a constant for regulator types
- Using proper event typing
- Avoiding type assertion
+const REGULATORS = [ + 'show_pcidss', + 'show_hipaa', + 'show_gdpr', + 'show_nist', + 'show_sama', +] as const; +type Regulator = typeof REGULATORS[number]; saveReportPreference = task( async ( - regulator: - | 'show_pcidss' - | 'show_hipaa' - | 'show_gdpr' - | 'show_nist' - | 'show_sama', - event + regulator: Regulator, + event: { target: HTMLInputElement } ) => { - const preference = this.orgPreference as OrganizationPreferenceModel; + const preference = this.orgPreference; + if (!preference) { + return; + }app/components/jira-account/index.ts (1)
53-57: Consider moving translation strings to a constant.The translation strings could be moved to a constant object for better maintainability.
+const TRANSLATIONS = { + INVALID_CREDENTIALS: 'tInValidCredentials', + JIRA_INTEGRATED: 'jiraIntegrated', + JIRA_WILL_BE_REVOKED: 'jiraWillBeRevoked', + PLEASE_ENTER_ALL_DETAILS: 'pleaseEnterAllDetails', + PLEASE_TRY_AGAIN: 'pleaseTryAgain', +} as const; - tInValidCredentials: string; - tJiraIntegrated: string; - tJiraWillBeRevoked: string; - tPleaseEnterAllDetails: string; - tPleaseTryAgain: string; + readonly translations: Record<keyof typeof TRANSLATIONS, string>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/components/github-account/index.js(0 hunks)app/components/github-account/index.ts(1 hunks)app/components/jira-account/index.ts(4 hunks)app/components/organization-email-domain/index.ts(3 hunks)app/components/organization-mfa/index.js(0 hunks)app/components/organization-mfa/index.ts(1 hunks)app/components/organization/settings/index.js(0 hunks)app/components/organization/settings/index.ts(1 hunks)app/components/regulatory-preference-organization/index.ts(2 hunks)app/components/sso-settings/index.ts(6 hunks)
💤 Files with no reviewable changes (3)
- app/components/organization/settings/index.js
- app/components/organization-mfa/index.js
- app/components/github-account/index.js
🧰 Additional context used
🪛 ESLint
app/components/sso-settings/index.ts
[error] 141-141: 'idpFormData' is never reassigned. Use 'const' instead.
(prefer-const)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Run Integration Tests - Part 7
- GitHub Check: Run Integration Tests - Part 6
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Integration Tests - Part 4
- GitHub Check: Run Integration Tests - Part 3
- GitHub Check: Run Integration Tests - Part 2
- GitHub Check: Run Integration Tests - Part 1
- GitHub Check: Run Acceptance Tests
- GitHub Check: Run Linting
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests - Part 3
- 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: Cloudflare Pages
🔇 Additional comments (7)
app/components/organization-mfa/index.ts (1)
60-60: Confirm the type import for AdapterError.
You are casting the error to "AdapterError," but there is no import for it in this file. Ensure "AdapterError" is correctly imported or globally available to avoid runtime issues in strict TypeScript projects.app/components/github-account/index.ts (2)
66-79: Check external redirect usage.
When redirecting the user via "window.location.href," verify if state or route transitions need to be handled in Ember’s router. If a straightforward redirect is sufficient, this is fine; otherwise, consider using Ember transitions or storing state to resume after returning from GitHub.
87-102: Validate error structure.
The error is accessed through "err.payload.detail," which assumes that "payload.detail" is always present. Confirm that all error responses include this field. Otherwise, consider defensive checks and default messages.app/components/sso-settings/index.ts (2)
19-24: Interfaces look well-defined.
The "SpMetadata" structure is clear, providing a strong contract for your SSO metadata.
188-221: Overall SSO toggle logic looks good.
The tasks for toggling SSO and enforcement are straightforward. Just ensure that "peekRecord" is suitable in your scenario and that you do not require a fresh fetch.app/components/organization/settings/index.ts (1)
8-23: LGTM! Well-structured type definitions.The types are clearly defined with proper property types and nullability handling.
app/components/jira-account/index.ts (1)
150-153: Verify the analytics type.The
CsbAnalyticsFeatureDatatype needs to be verified for correctness.✅ Verification successful
🌐 Web query:
What is the correct type for CSB analytics feature data in Ember.js applications?💡 Result:
In Ember.js applications handling analytics feature data (like CSB analytics), the correct typing depends on the specific data structure being tracked. Based on common patterns in Ember Data modeling and analytics implementations shown in the search results: **Core recommended types:** - **Strings** for textual data (event names, page URLs, titles) - **Numbers** for quantitative metrics (event values, durations) - **Booleans** for flags/toggles (consent statuses) - **Dates** for timestamps (event time, session start/end) **Implementation examples from search results:** ```javascript // Google Analytics tracking (Search Result [5]) ga('send', 'pageview', { 'page': this.get('url'), // String type 'title': this.get('url') // String type }); // Event tracking with ember-metrics (Search Result [2]) this.metrics.trackEvent({ event: 'purchase', // String value: 49.99, // Number items: 3 // Number }); // User metadata tracking (Search Result [8]) buildRouteInfoMetadata() { return { trackingKey: 'page_profile', // String user: { id: this.user.id, // Number/String type: this.user.type // String } }; } ``` **Key considerations:** 1. Use `DS.attr('date')` for ISO 8601 timestamps when tracking event times[1] 2. Convert complex objects to JSON strings if needed using `DS.attr('string')`[4] 3. For CSV exports (common in analytics), format numbers without type prefixes[6] 4. Follow service-specific requirements (e.g., Fathom requires string IDs[7]) **Type validation pattern from Ember Data[1][10]:** ```javascript // app/models/analytics-event.js import Model, { attr } from '@ember-data/model'; export default class AnalyticsEventModel extends Model { @attr('string') eventName; @attr('number') value; @attr('date') timestamp; @attr('boolean') isConversion; } ``` When implementing CSB analytics specifically, confirm if there are any special typing requirements in their documentation, but these core Ember.js typing patterns should provide a solid foundation[1][2][5][7]. Citations: - 1: https://guides.emberjs.com/v2.14.0/models/defining-models/ - 2: https://github.com/adopted-ember-addons/ember-metrics - 3: https://www.fusioncharts.com/blog/charts-emberjs-web-application/ - 4: https://discuss.emberjs.com/t/custom-datatype-support-in-view-attributebindings-and-bindattr/5012 - 5: https://guides.emberjs.com/v1.10.0/cookbook/helpers_and_components/adding_google_analytics_tracking/ - 6: https://discuss.emberjs.com/t/whats-the-best-strategy-for-letting-users-download-ember-data-returns-as-csvs/14767 - 7: https://usefathom.com/docs/integrations/ember - 8: https://blog.emberjs.com/ember-3-10-released/ - 9: https://rfcs.emberjs.com/id/0507-embroider-v2-package-format/ - 10: https://guides.emberjs.com/v5.7.0/models/
CsbAnalyticsFeatureData Type Confirmation
Based on Ember.js best practices—as reflected in Ember Data models and common analytics implementations—the CSB analytics feature data type should include properties with the following types:
• Textual data (e.g., event names, identifiers) as strings
• Quantitative metrics (e.g., event values, durations) as numbers
• Flags (e.g., for conversion or toggles) as booleans
• Timestamps (e.g., event time) as datesPlease verify that the definition of CsbAnalyticsFeatureData accurately reflects the structure of ENV.csb.integrateJIRA according to these recommendations. In other words, ensure that its properties are typed in line with Ember Data conventions (using, for example, DS.attr('string'), DS.attr('number'), etc.) so that analytics events are tracked correctly.
Deploying irenestaging with
|
| Latest commit: |
3175dbb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ad445e8e.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1655-convert-org-componen.irenestaging.pages.dev |
SmitGala
force-pushed
the
PD-1655-Convert-Org-Components-to-ts
branch
from
bcb5bec to
b836454
Compare
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1655-Convert-Org-Components-to-ts
|
| Run status |
|
| Run duration | 05m 22s |
| Commit |
|
| Committer | SmitGala |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
1
|
Passing
|
30
|
| 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 apk file: 58062 |
Test Replay
Screenshots
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/components/sso-settings/index.ts (1)
128-163: Improve file upload error handling.The file upload error handling could be more robust with type checking and proper error messages.
uploadIdPMetadata = task({ restartable: true }, async (event) => { event.preventDefault(); try { - $.parseXML(this.idpMetadataXml!); + if (!this.idpMetadataXml) { + this.notify.error('No XML file provided'); + return; + } + + const parser = new DOMParser(); + const xmlDoc = parser.parseFromString(this.idpMetadataXml, 'text/xml'); + + if (xmlDoc.getElementsByTagName('parsererror').length > 0) { + throw new Error('Invalid XML format'); + } } catch (err) { - this.notify.error('Please enter a valid XML file'); + this.notify.error('Please provide a valid XML file'); return; } try { const blob = new Blob([this.idpMetadataXml ?? ''], { type: 'text/xml' }); const idpFormData = new FormData(); idpFormData.append('filename', blob); const url = [ ENV.endpoints['organizations'], this.args.organization.id, ENV.endpoints['saml2IdPMetadata'], ].join('/'); await this.ajax.post(url, { data: idpFormData, contentType: 'multipart/form-data', }); this.idpMetadataXml = null; this.notify.success('Uploaded IdP Metadata Config successfully'); this.getIdPMetadata.perform(); } catch (err) { - this.notify.error(parseError(err, this.tPleaseTryAgain)); + const error = err as AjaxError; + this.notify.error( + parseError(error, 'Failed to upload IdP Metadata: ' + this.tPleaseTryAgain) + ); } });
🧹 Nitpick comments (5)
app/components/organization-mfa/index.ts (1)
46-71: Enhance error handling insetMandatoryMFAtask.The error handling could be improved in the following ways:
- Add type definition for
AdapterError.- Consider using a more specific error type than
errMsgfor better type safety.+import type { AdapterError } from '@ember-data/adapter/error'; setMandatoryMFA = task(async (_, mandatoryStateChecked) => { try { // ... existing code ... } catch (err) { this.isSavingStatus = false; const error = err as AdapterError; - let errMsg = this.tPleaseTryAgain; + let errorMessage: string = this.tPleaseTryAgain; if (error.errors?.length) { - errMsg = error.errors[0]?.detail || errMsg; + errorMessage = error.errors[0]?.detail ?? errorMessage; } else if (error.message) { - errMsg = error.message; + errorMessage = error.message; } - this.notify.error(errMsg); + this.notify.error(errorMessage); } });app/components/jira-account/index.ts (2)
42-43: Initializeuserandchangesetproperties correctly.The
userproperty is initialized but never used, andchangesetlacks initialization in the class definition.- user: null = null; - changeset: ChangesetBufferProps; + changeset!: ChangesetBufferProps;
96-110: Improve error handling incheckJIRAtask.The error handling could be more specific about the 404 case.
checkJIRA = task(async () => { try { const data = await this.ajax.request<JiraCheckResponse>(this.baseURL); this.isJIRAConnected = true; this.connectedHost = data.host; this.connectedUsername = data.username; } catch (err) { const error = err as AjaxError; if (error.status === 404) { this.isJIRAConnected = false; + // Silent fail for 404 as it indicates no JIRA integration + } else { + this.notify.error(this.tPleaseTryAgain); } } });app/components/sso-settings/index.ts (2)
1-2: Consider replacing jQuery with modern alternatives.The component uses jQuery for XML parsing. Consider using modern alternatives like
DOMParser.-/* eslint-disable ember/no-jquery */ -import $ from 'jquery';Example implementation:
const parser = new DOMParser(); try { parser.parseFromString(xmlString, 'text/xml'); } catch (err) { // Handle error }
187-221: Refactor similar code in toggle tasks.The
toggleSSOEnableandtoggleSSOEnforcetasks share similar structure. Consider extracting common logic into a reusable function.+ private async toggleSSOSetting( + setting: 'enabled' | 'enforced', + value: boolean + ): Promise<void> { + try { + const ssoObj = this.store.peekRecord( + 'organization-sso', + this.args.organization.id + ); + ssoObj?.set(setting, value); + await ssoObj?.save(); + + const message = setting === 'enabled' + ? `SSO authentication ${value ? 'enabled' : 'disabled'}` + : `SSO enforce turned ${value ? 'ON' : 'OFF'}`; + + this.notify.success(message); + } catch (err) { + this.notify.error(parseError(err, this.tPleaseTryAgain)); + } + } - toggleSSOEnable = task({ restartable: true }, async (event, value) => { - try { - const ssoObj = this.store.peekRecord( - 'organization-sso', - this.args.organization.id - ); - ssoObj?.set('enabled', value); - await ssoObj?.save(); - this.notify.success(`SSO authentication ${value ? 'enabled' : 'disabled'}`); - } catch (err) { - this.notify.error(parseError(err, this.tPleaseTryAgain)); - } - }); + toggleSSOEnable = task({ restartable: true }, async (_, value) => { + await this.toggleSSOSetting('enabled', value); + }); - toggleSSOEnforce = task({ restartable: true }, async (event, value) => { - try { - const ssoObj = this.store.peekRecord( - 'organization-sso', - this.args.organization.id - ); - ssoObj?.set('enforced', value); - await ssoObj?.save(); - this.notify.success(`SSO enforce turned ${value ? 'ON' : 'OFF'}`); - } catch (err) { - this.notify.error(parseError(err, this.tPleaseTryAgain)); - } - }); + toggleSSOEnforce = task({ restartable: true }, async (_, value) => { + await this.toggleSSOSetting('enforced', value); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/components/github-account/index.js(0 hunks)app/components/github-account/index.ts(1 hunks)app/components/jira-account/index.ts(4 hunks)app/components/organization-email-domain/index.ts(3 hunks)app/components/organization-mfa/index.js(0 hunks)app/components/organization-mfa/index.ts(1 hunks)app/components/organization/settings/index.js(0 hunks)app/components/organization/settings/index.ts(1 hunks)app/components/regulatory-preference-organization/index.ts(2 hunks)app/components/sso-settings/index.ts(6 hunks)
💤 Files with no reviewable changes (3)
- app/components/organization/settings/index.js
- app/components/organization-mfa/index.js
- app/components/github-account/index.js
🚧 Files skipped from review as they are similar to previous changes (4)
- app/components/regulatory-preference-organization/index.ts
- app/components/organization/settings/index.ts
- app/components/organization-email-domain/index.ts
- app/components/github-account/index.ts
⏰ 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
SmitGala
changed the title
SmitGala
force-pushed
the
PD-1655-Convert-Org-Components-to-ts
branch
from
b836454 to
3175dbb
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
app/components/organization/settings/index.ts (1)
17-23: Consider makingmeproperty optional.The
meproperty inOrganizationSettingsTypemight not always be required since it's also injected as a service in the component.type OrganizationSettingsType = { integratedUser: IntegratedUser | null; reconnect: boolean; user: UserModel; organization: OrganizationModel; - me: MeService; + me?: MeService; };app/components/jira-account/index.ts (2)
42-43: Remove redundant null initialization and improve type safety.The
userproperty initialization can be improved.- user: null = null; + user = null as null;
150-153: Consider extracting analytics configuration to a type-safe constant.The analytics configuration could be made more type-safe.
+const JIRA_ANALYTICS_CONFIG = ENV.csb['integrateJIRA'] as CsbAnalyticsFeatureData; triggerAnalytics( 'feature', - ENV.csb['integrateJIRA'] as CsbAnalyticsFeatureData + JIRA_ANALYTICS_CONFIG );app/components/sso-settings/index.ts (1)
1-2: Consider removing jQuery dependency.The component still relies on jQuery for XML parsing. Consider using the native
DOMParserinstead, which is well-supported across modern browsers.Here's how you can replace jQuery's XML parsing:
-/* eslint-disable ember/no-jquery */ -import $ from 'jquery'; +const domParser = new DOMParser();Then update the XML parsing in the
uploadIdPMetadatatask:- $.parseXML(this.idpMetadataXml!); + domParser.parseFromString(this.idpMetadataXml!, 'text/xml');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/components/github-account/index.js(0 hunks)app/components/github-account/index.ts(1 hunks)app/components/jira-account/index.ts(4 hunks)app/components/organization-email-domain/index.ts(3 hunks)app/components/organization-mfa/index.js(0 hunks)app/components/organization-mfa/index.ts(1 hunks)app/components/organization/settings/index.js(0 hunks)app/components/organization/settings/index.ts(1 hunks)app/components/regulatory-preference-organization/index.ts(2 hunks)app/components/sso-settings/index.ts(6 hunks)
💤 Files with no reviewable changes (3)
- app/components/organization/settings/index.js
- app/components/github-account/index.js
- app/components/organization-mfa/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/regulatory-preference-organization/index.ts
- app/components/organization-email-domain/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Run Integration Tests - Part 7
- GitHub Check: Run Integration Tests - Part 6
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Integration Tests - Part 4
- GitHub Check: Run Integration Tests - Part 3
- GitHub Check: Run Integration Tests - Part 2
- GitHub Check: Run Integration Tests - Part 1
- GitHub Check: Run Acceptance Tests
- GitHub Check: Run Unit Tests
- GitHub Check: Run Linting
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
app/components/organization/settings/index.ts (2)
8-15: LGTM! Well-structured type definition.The
IntegratedUsertype is well-defined with clear property types.
31-33: Consider adding computed properties or methods.The component currently only injects the
meservice but doesn't implement any functionality. Consider whether additional computed properties or methods are needed.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usage of OrganizationSettings component to understand expected functionality rg -A 5 "Organization::Settings"Length of output: 1428
Verification of "Consider adding computed properties or methods" for OrganizationSettingsComponent
Based on the context gathered:
- The shell script output confirms that OrganizationSettingsComponent is used as a simple container in the application.
- The component's sole responsibility is to inject the “me” service, and there is no evidence in related files or templates that additional computed properties or methods are expected or utilized.
- The design appears intentional, and adding extra computed properties or methods is unnecessary unless further functionality is introduced.
app/components/github-account/index.ts (1)
96-100: Consider adding error type assertion.The error handling in
removeIntegrationtask assumes the error has apayload.detailproperty.app/components/sso-settings/index.ts (3)
19-31: LGTM! Well-defined interfaces.The
SpMetadataandSsoSettingsSignatureinterfaces are well-structured and provide good type safety.
224-228: LGTM! Proper Glint setup.The Glint registry declaration ensures type safety in templates.
65-67: Handle task failures in constructor.Tasks performed in the constructor should handle failures to prevent silent errors.
Consider wrapping the task calls in try-catch blocks or using
catchhandlers:- this.SPMetadata.perform(); - this.getIdPMetadata.perform(); - this.getSSOData.perform(); + this.SPMetadata.perform().catch((error) => + this.notify.error(parseError(error, this.tPleaseTryAgain)) + ); + this.getIdPMetadata.perform().catch((error) => + this.notify.error(parseError(error, this.tPleaseTryAgain)) + ); + this.getSSOData.perform().catch((error) => + this.notify.error(parseError(error, this.tPleaseTryAgain)) + );
| export interface OrganizationEmailDomainSignature { | ||
| Args: { | ||
| user: UserModel; | ||
| organization: OrganizationModel; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Interface name doesn't match component purpose.
The interface is named OrganizationEmailDomainSignature but is used for MFA component.
-export interface OrganizationEmailDomainSignature {
+export interface OrganizationMfaSignature {
Args: {
user: UserModel;
organization: OrganizationModel;
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface OrganizationEmailDomainSignature { | |
| Args: { | |
| user: UserModel; | |
| organization: OrganizationModel; | |
| }; | |
| } | |
| export interface OrganizationMfaSignature { | |
| Args: { | |
| user: UserModel; | |
| organization: OrganizationModel; | |
| }; | |
| } |
| setMandatoryMFA = task(async (_, mandatoryStateChecked) => { | ||
| try { | ||
| this.isSavingStatus = true; | ||
|
|
||
| const org = this.organization; | ||
| org.set('mandatoryMfa', mandatoryStateChecked); | ||
|
|
||
| await org.save(); | ||
|
|
||
| this.isSavingStatus = false; | ||
| this.notify.success(this.tChangedMandatoryMFA); | ||
| } catch (err) { | ||
| this.isSavingStatus = false; | ||
|
|
||
| const error = err as AdapterError; | ||
| let errMsg = this.tPleaseTryAgain; | ||
|
|
||
| if (error.errors?.length) { | ||
| errMsg = error.errors[0]?.detail || errMsg; | ||
| } else if (error.message) { | ||
| errMsg = error.message; | ||
| } | ||
|
|
||
| this.notify.error(errMsg); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve task parameter types and error handling.
The task parameters and error handling could be more specific:
- The first parameter is unused (marked as
_) AdapterErrortype is used but not imported
+import type AdapterError from '@ember-data/adapter/error';
- setMandatoryMFA = task(async (_, mandatoryStateChecked) => {
+ setMandatoryMFA = task(async (_event: unknown, mandatoryStateChecked: boolean) => {| integrateGithub = task(async () => { | ||
| try { | ||
| triggerAnalytics( | ||
| 'feature', | ||
| ENV.csb['integrateGithub'] as CsbAnalyticsFeatureData | ||
| ); | ||
|
|
||
| const data = await this.redirectAPI.perform(); | ||
|
|
||
| window.location.href = data.url; | ||
| } catch (error) { | ||
| this.notify.error(this.tGithubErrorIntegration); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in integrateGithub task.
The error handling could be more specific and provide better user feedback.
integrateGithub = task(async () => {
try {
triggerAnalytics(
'feature',
ENV.csb['integrateGithub'] as CsbAnalyticsFeatureData
);
const data = await this.redirectAPI.perform();
window.location.href = data.url;
- } catch (error) {
+ } catch (e) {
+ const error = e as AjaxError;
+ const errMsg = error.payload?.detail || this.tGithubErrorIntegration;
- this.notify.error(this.tGithubErrorIntegration);
+ this.notify.error(errMsg);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| integrateGithub = task(async () => { | |
| try { | |
| triggerAnalytics( | |
| 'feature', | |
| ENV.csb['integrateGithub'] as CsbAnalyticsFeatureData | |
| ); | |
| const data = await this.redirectAPI.perform(); | |
| window.location.href = data.url; | |
| } catch (error) { | |
| this.notify.error(this.tGithubErrorIntegration); | |
| } | |
| }); | |
| integrateGithub = task(async () => { | |
| try { | |
| triggerAnalytics( | |
| 'feature', | |
| ENV.csb['integrateGithub'] as CsbAnalyticsFeatureData | |
| ); | |
| const data = await this.redirectAPI.perform(); | |
| window.location.href = data.url; | |
| } catch (e) { | |
| const error = e as AjaxError; | |
| const errMsg = error.payload?.detail || this.tGithubErrorIntegration; | |
| this.notify.error(errMsg); | |
| } | |
| }); |
| @@ -98,29 +129,27 @@ export default class SsoSettingsComponent extends Component { | |||
| event.preventDefault(); | |||
|
|
|||
| try { | |||
| $.parseXML(this.idpMetadataXml); | |||
| $.parseXML(this.idpMetadataXml!); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid non-null assertion operator.
Replace the non-null assertion operator (!) with proper null checking to prevent runtime errors.
- $.parseXML(this.idpMetadataXml!);
+ if (!this.idpMetadataXml) {
+ throw new Error('No XML content provided');
+ }
+ $.parseXML(this.idpMetadataXml);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $.parseXML(this.idpMetadataXml!); | |
| if (!this.idpMetadataXml) { | |
| throw new Error('No XML content provided'); | |
| } | |
| $.parseXML(this.idpMetadataXml); |
| await this.ajax.post(url, { | ||
| data: idpFormData, | ||
| processData: false, | ||
| contentType: false, | ||
| mimeType: 'multipart/form-data', | ||
| contentType: 'multipart/form-data', | ||
| }); |
There was a problem hiding this comment.
Fix FormData content type.
The contentType should not be set when sending FormData. The browser will automatically set the correct Content-Type header with the boundary.
await this.ajax.post(url, {
data: idpFormData,
- contentType: 'multipart/form-data',
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.ajax.post(url, { | |
| data: idpFormData, | |
| processData: false, | |
| contentType: false, | |
| mimeType: 'multipart/form-data', | |
| contentType: 'multipart/form-data', | |
| }); | |
| await this.ajax.post(url, { | |
| data: idpFormData, | |
| }); |
| export default class SsoSettingsComponent extends Component<SsoSettingsSignature> { | ||
| @service declare intl: IntlService; | ||
| @service declare ajax: IreneAjaxService; | ||
| @service declare store: Store; | ||
| @service('notifications') declare notify: NotificationService; |
There was a problem hiding this comment.
Add missing NotificationService type import.
The NotificationService type used in the service declaration is not imported.
Add the following import at the top of the file:
+import type NotificationService from 'ember-cli-notifications/services/notifications';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default class SsoSettingsComponent extends Component<SsoSettingsSignature> { | |
| @service declare intl: IntlService; | |
| @service declare ajax: IreneAjaxService; | |
| @service declare store: Store; | |
| @service('notifications') declare notify: NotificationService; | |
| +import type NotificationService from 'ember-cli-notifications/services/notifications'; | |
| export default class SsoSettingsComponent extends Component<SsoSettingsSignature> { | |
| @service declare intl: IntlService; | |
| @service declare ajax: IreneAjaxService; | |
| @service declare store: Store; | |
| @service('notifications') declare notify: NotificationService; | |
| } |
No description provided.