Fix Deprecation in Organization and Service Account Routes #1486
Conversation
WalkthroughThis pull request introduces modifications across multiple components and models. The primary change involves altering the data retrieval methods from Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@SmitGala has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
error Error: http://10.0.0.23:4873/@typescript-eslint%2feslint-plugin: ESOCKETTIMEDOUT 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (17)
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 (
|
Deploying irenestaging with
|
| Latest commit: |
57a75c3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://966fc462.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1560-fix-deprecation-in-o.irenestaging.pages.dev |
Irene
|
||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1560-Fix-Deprecation-in-Organization-and-Service-Account
|
| Run status |
|
| Run duration | 05m 58s |
| Commit |
|
| Committer | SmitGala |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
32
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
app/components/organization-team/project-list/project-info/index.ts (1)
32-43: Add loading state tracking for better UX.The task should track its loading state to provide feedback to users during the fetch operation.
Consider adding a loading state:
export default class OrganizationTeamProjectListProjectInfo extends Component<OrganizationTeamProjectListProjectInfoComponentSignature> { + @tracked isLoading = false; + fetchProjectDetail = task(async () => { + this.isLoading = true; try { const project = await this.store.findRecord( 'project', this.args.project.id ); this.teamProject = project; } catch (e) { this.notify.error(parseError(e)); } finally { + this.isLoading = false; } }); }This allows the template to show loading indicators when appropriate.
app/components/organization-member/list/add-to-team/index.ts (1)
Line range hint
52-60: Consider type and maintainability improvements.While the search implementation is solid, consider these minor enhancements:
- Use a more specific event type:
InputEventinstead ofEvent- Extract the debounce timeout (500ms) as a named constant
- handleSearchQueryChange(event: Event) { + private readonly SEARCH_DEBOUNCE_MS = 500; + + handleSearchQueryChange(event: InputEvent) { debounce( this, this.setSearchQuery, (event.target as HTMLInputElement)?.value, - 500 + this.SEARCH_DEBOUNCE_MS ); }app/components/organization-team/member-list/index.ts (1)
55-57: Consider enhancing type safety for the getter return value.The current implementation could be more explicit about its return type to ensure type safety throughout the component.
- get teamMemberList() { + get teamMemberList(): OrganizationTeamMemberModel[] { return this.teamMemberResponse?.slice() || []; }app/components/organization-team/project-list/index.ts (1)
52-52: Consider adding a comment about immutabilityTo help future maintainers understand the intent, consider adding a comment explaining why we're using
slice().+ // Use slice() to create a shallow copy, preventing direct mutations of the underlying RecordArray return this.teamProjectResponse?.slice() || [];app/components/organization-namespace/index.ts (2)
57-57: LGTM! Consider type annotation for clarity.The change from
toArray()toslice()is correct and follows Ember Data's modern best practices for creating array copies. This helps future-proof the code by removing the deprecated method.Consider adding an explicit return type annotation for better type safety:
- get namespaceList() { + get namespaceList(): OrganizationNamespaceModel[] { return this.namespaceResponse?.slice() || []; }
Line range hint
134-144: Consider extracting duplicated error handling logic.The error handling pattern is duplicated in both
fetchNamespaceandconfirmRejecttasks. Consider extracting this into a reusable utility function to improve maintainability and reduce code duplication.Here's a suggested implementation:
// Add to a utility file, e.g., 'utils/handle-adapter-error.ts' export function getErrorMessage(err: AdapterError, intl: IntlService): string { let errMsg = intl.t('pleaseTryAgain'); if (err.errors?.[0]?.detail) { errMsg = err.errors[0].detail; } else if (err.message) { errMsg = err.message; } return errMsg; } // Then in your component: import { getErrorMessage } from 'utils/handle-adapter-error'; // Usage in tasks: catch (e) { const err = e as AdapterError; this.notify.error(getErrorMessage(err, this.intl)); }Also applies to: 182-192
app/components/organization-team/add-team-member/index.ts (3)
Line range hint
115-125: Consider extracting error handling logic into a utility function.The error handling pattern is duplicated in both
addTeamMemberandfetchUserstasks. Consider creating a reusable utility function to handle AdapterError consistently across the application.Here's a suggested implementation:
// utils/handle-adapter-error.ts import IntlService from 'ember-intl/services/intl'; import { AdapterError } from '@ember-data/adapter/error'; export function getErrorMessage(error: AdapterError, intl: IntlService): string { const defaultMessage = intl.t('pleaseTryAgain'); if (error.errors?.[0]?.detail) { return error.errors[0].detail; } return error.message || defaultMessage; }Then use it in your tasks:
} catch (e) { const err = e as AdapterError; - let errMsg = this.intl.t('pleaseTryAgain'); - - if (err.errors && err.errors.length) { - errMsg = err.errors[0]?.detail || errMsg; - } else if (err.message) { - errMsg = err.message; - } + const errMsg = getErrorMessage(err, this.intl); this.notify.error(errMsg); }Also applies to: 190-200
Line range hint
29-29: Consider enhancing type safety for selected members.The
SelectedMemberModeltype could be more specific thanRecord<string, OrganizationUserModel>. Consider using a Map or a more precise type definition.Here's a suggested improvement:
type SelectedMemberMap = Map<string, OrganizationUserModel>; export default class OrganizationTeamAddTeamMemberComponent extends Component<OrganizationTeamAddTeamMemberComponentSignature> { @tracked selectedMembers: SelectedMemberMap = new Map(); // Update the selection change handler @action selectionChange(member: OrganizationUserModel, event: Event) { const newMap = new Map(this.selectedMembers); if ((event.target as HTMLInputElement).checked) { newMap.set(member.id, member); } else { newMap.delete(member.id); } this.selectedMembers = newMap; } get hasNoSelection() { return this.selectedMembers.size === 0; } }Also applies to: 144-150
Line range hint
144-150: Consider extracting analytics trigger into a service.The analytics trigger is embedded directly in the component. Consider moving this to a dedicated analytics service for better reusability and testing.
Here's a suggested approach:
// services/analytics.ts import Service from '@ember/service'; import { CsbAnalyticsFeatureData } from 'types/analytics'; export default class AnalyticsService extends Service { triggerFeature(feature: CsbAnalyticsFeatureData) { triggerAnalytics('feature', feature); } } // Then in your component: @service declare analytics: AnalyticsService; // Usage: this.analytics.triggerFeature(ENV.csb['addTeamMember']);app/components/organization-team/add-team-project/index.ts (1)
Line range hint
59-64: Consider memoizing the mapped project list.Since the getter performs a map operation on every access, consider memoizing the result to optimize performance, especially if this component re-renders frequently.
Here's a suggested implementation using tracked properties:
+ @tracked private _mappedProjects: Array<{ project: OrganizationProjectModel; checked: boolean }> = []; + + @action + private updateMappedProjects() { + const list = this.projectResponse?.slice() || []; + this._mappedProjects = list.map((project: OrganizationProjectModel) => ({ + project, + checked: !!this.selectedProjects[project.id], + })); + } get projectList() { - const list = this.projectResponse?.slice() || []; - return list.map((project: OrganizationProjectModel) => ({ - project, - checked: !!this.selectedProjects[project.id], - })); + return this._mappedProjects; }Then call
updateMappedProjects()wheneverprojectResponseorselectedProjectschanges:
- After
fetchProjects.perform()- In
selectionChangeaction
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
app/components/organization-invitation-list/index.ts(1 hunks)app/components/organization-member/list/add-to-team/index.ts(1 hunks)app/components/organization-member/list/index.ts(1 hunks)app/components/organization-member/list/member-details/index.ts(1 hunks)app/components/organization-namespace/index.ts(1 hunks)app/components/organization-team/add-team-member/index.ts(1 hunks)app/components/organization-team/add-team-project/index.ts(1 hunks)app/components/organization-team/index.ts(1 hunks)app/components/organization-team/member-list/index.ts(1 hunks)app/components/organization-team/project-list/index.ts(1 hunks)app/components/organization-team/project-list/project-info/index.ts(2 hunks)app/models/organization-invitation.ts(1 hunks)app/models/organization-team-invitation.ts(1 hunks)
🔇 Additional comments (10)
app/models/organization-team-invitation.ts (1)
14-14: LGTM! Consistent with deprecation fixes.
The update to use { async: true, inverse: null } is correct and aligns with the deprecation fix pattern.
Let's verify the consistency of this pattern across related models:
✅ Verification successful
Consistent pattern confirmed across organization relationships
The @belongsTo('organization', { async: true, inverse: null }) pattern is consistently implemented across all organization relationships in the codebase:
- app/models/organization-invitation.ts
- app/models/organization-team-invitation.ts
- app/models/organization-project.ts
- app/models/organization-namespace.ts
- app/models/amconfiguration.ts
The implementation in organization-team-invitation.ts follows the established pattern correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of async: true, inverse: null in related models
# Expected: All @belongsTo decorators for organization relationships should follow the same pattern
# Search for @belongsTo decorators with organization relationships
ast-grep --pattern '@belongsTo("organization", $_)'
# Also check for single-quoted variant
ast-grep --pattern "@belongsTo('organization', $_)"
Length of output: 141
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all belongsTo relationships with 'organization'
rg "@belongsTo\(['\"]organization" -A 1 --type ts
# Also search for any other belongsTo relationships to compare patterns
rg "@belongsTo\(" -A 1 --type ts
Length of output: 13447
app/models/organization-invitation.ts (2)
15-16: LGTM: Proper handling of async team relationship.
The update to use { async: true, inverse: null } correctly addresses the deprecation by explicitly declaring the async nature of the relationship and the absence of an inverse relationship.
18-19: LGTM: Proper handling of async organization relationship.
Similar to the team relationship, this change appropriately addresses the deprecation warning while maintaining the correct type safety with AsyncBelongsTo.
Let's verify that similar changes have been made consistently across related models:
✅ Verification successful
All organization relationships are consistently using async: true
The verification confirms that all @belongsTo relationships with 'organization' across the codebase are properly configured with async: true and have the correct type safety using AsyncBelongsTo. This includes:
- organization-invitation.ts
- organization-team-invitation.ts
- organization-project.ts
- organization-namespace.ts
- amconfiguration.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent async relationship handling in related models
# Expected: All @belongsTo decorators for organization relationships should use async: true
# Search for organization belongsTo decorators in all model files
rg -A 2 "@belongsTo\('organization'[^)]*\)" app/models/
Length of output: 1230
app/components/organization-team/project-list/project-info/index.ts (2)
4-6: LGTM: Imports are appropriate for the new implementation.
The new imports support the transition to tracked properties and task-based async operations.
Also applies to: 8-9
32-43: Verify consistent task pattern usage across similar components.
As this change is part of a broader deprecation fix, we should ensure other components follow the same pattern.
✅ Verification successful
Let me gather more information about the other component to ensure a thorough verification.
No consistency concerns with task pattern usage
The other project info component (add-team-project/project-info) is a simple presentational component without any data fetching logic. The task pattern implementation in the reviewed component is specific to its unique requirement of fetching project details, and there's no inconsistency to address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar components that might need the same update
echo "Searching for similar project info components..."
rg -t ts "class.*ProjectInfo.*extends Component" --glob "app/components/**/*"
echo "Checking for potentially outdated findRecord usage..."
ast-grep --pattern 'store.findRecord($_, $_)'
Length of output: 777
Script:
#!/bin/bash
# Check the implementation of the other project info component
echo "Checking add-team-project/project-info implementation..."
cat app/components/organization-team/add-team-project/project-info/index.ts
# Look for any task usage in this component
echo -e "\nChecking for task usage in the component..."
rg -t ts "task\(" app/components/organization-team/add-team-project/project-info/index.ts
Length of output: 1032
app/components/organization-member/list/add-to-team/index.ts (1)
Line range hint 119-131: Robust error handling implementation!
The error handling in the fetchTeams task follows best practices with:
- Proper type-safe error handling
- Well-structured fallback chain for error messages
- Internationalization support
app/components/organization-team/member-list/index.ts (1)
56-56: LGTM! The change from toArray() to slice() is appropriate.
The change maintains the same immutability guarantees while aligning with modern practices. Both methods create a new array, preventing mutations of the original data.
app/components/organization-team/project-list/index.ts (1)
52-52: LGTM! Good transition from toArray() to slice()
The change from toArray() to slice() is appropriate and aligns with modern Ember Data practices while maintaining the same immutability guarantees.
app/components/organization-team/add-team-member/index.ts (1)
Line range hint 61-66: LGTM! Verify deprecation documentation.
The change from toArray() to slice() is appropriate for creating a shallow copy of the array. The transformation logic remains correct and maintains the same functionality.
Let's verify if this change is part of documented deprecation:
app/components/organization-team/add-team-project/index.ts (1)
Line range hint 59-64: LGTM! Good deprecation fix.
The change from toArray() to slice() is appropriate as it maintains the same shallow copy functionality while addressing the deprecation. This aligns with the PR's objective of fixing deprecations in the codebase.
app/components/organization-team/project-list/project-info/index.ts
Outdated
Show resolved
Hide resolved
SmitGala
force-pushed
the
PD-1560-Fix-Deprecation-in-Organization-and-Service-Account
branch
2 times, most recently
from
e639938 to
0def473
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/components/organization-email-domain/index.js (2)
29-29: Consider improving error handling.The error handling in
addDomainis robust, usingrollbackAttributesto revert changes on failure. However, consider adding more specific error messages or logging for debugging purposes to enhance maintainability and traceability of issues.
29-29: Consider improving error handling.The error handling in
deleteDomaineffectively manages failures by re-inserting the domain back into the list. However, consider adding more specific error messages or logging for debugging purposes to enhance maintainability and traceability of issues.app/components/sso-settings/index.js (1)
78-86: Well-implemented task for fetching SSO data with robust error handling.The
getSSODatatask is correctly set up to fetch SSO data asynchronously and handle errors effectively. The use ofparseErrorto format and display errors is a good practice.Consider adding error logging to an external monitoring service for better observability and debugging in production environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
app/components/organization-email-domain/index.js(1 hunks)app/components/organization-invitation-list/index.ts(1 hunks)app/components/organization-member/list/add-to-team/index.ts(1 hunks)app/components/organization-member/list/index.ts(1 hunks)app/components/organization-member/list/member-details/index.ts(1 hunks)app/components/organization-namespace/index.ts(1 hunks)app/components/organization-team/add-team-member/index.ts(1 hunks)app/components/organization-team/add-team-project/index.ts(1 hunks)app/components/organization-team/index.ts(1 hunks)app/components/organization-team/member-list/index.ts(1 hunks)app/components/organization-team/project-list/index.ts(1 hunks)app/components/organization-team/project-list/project-info/index.ts(2 hunks)app/components/sso-settings/index.js(3 hunks)app/models/organization-invitation.ts(1 hunks)app/models/organization-team-invitation.ts(1 hunks)app/models/service-account-project.ts(1 hunks)app/models/service-account.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- app/components/organization-invitation-list/index.ts
- app/components/organization-member/list/add-to-team/index.ts
- app/components/organization-member/list/index.ts
- app/components/organization-member/list/member-details/index.ts
- app/components/organization-namespace/index.ts
- app/components/organization-team/add-team-member/index.ts
- app/components/organization-team/add-team-project/index.ts
- app/components/organization-team/index.ts
- app/components/organization-team/member-list/index.ts
- app/components/organization-team/project-list/index.ts
- app/models/organization-invitation.ts
- app/models/organization-team-invitation.ts
🔇 Additional comments (14)
app/models/service-account-project.ts (3)
7-7: Approved: Asynchronous relationship handling for project.
The update to the @belongsTo decorator for the project property is correctly implemented to handle the relationship asynchronously without an inverse. This change aligns with modern Ember.js practices and should help in improving performance and maintainability.
10-10: Approved: Asynchronous relationship handling for service-account.
The update to the @belongsTo decorator for the service-account property is correctly implemented to handle the relationship asynchronously without an inverse. This change aligns with modern Ember.js practices and should help in improving performance and maintainability.
7-10: Verify application compatibility with updated model relationships.
Given the changes to the relationship handling in ServiceAccountProjectModel, it is crucial to verify that the rest of the application correctly handles these asynchronous relationships. This includes checking that there are no assumptions about synchronous loading or inverse relationships that could lead to runtime errors.
Run the following script to verify the application's compatibility:
app/components/organization-team/project-list/project-info/index.ts (5)
4-6: Approved new imports for enhanced functionality
The new imports tracked, task, and ProjectModel are appropriately used in the component to support its reactive and asynchronous features.
Also applies to: 8-8
19-19: Approved service injection with a reminder
The injection of the notify service using @service('notifications') is correctly implemented. Ensure that NotificationService is properly imported to avoid any TypeScript errors.
21-21: Approved tracked property declaration
The teamProject property is correctly declared as a tracked property, ensuring reactive UI updates when the project data changes.
23-29: Approved constructor update and task invocation
The update to the constructor to invoke fetchProjectDetail.perform() is a positive change, aligning with asynchronous patterns and improving the component's reliability in data fetching.
32-43: Approved task definition with a suggestion
The fetchProjectDetail task is well-defined, with robust error handling and appropriate assignment of fetched data. Ensure that the parseError utility effectively extracts meaningful error messages for the best user experience.
app/components/organization-email-domain/index.js (1)
29-29: Approved change from .toArray() to .slice().
The update to use .slice() instead of .toArray() is a good practice for ensuring data immutability. This change aligns with the systematic updates across other components and should help maintain consistency in data handling.
app/models/service-account.ts (3)
36-36: Approved: Asynchronous relationship for projects.
The update to make the projects relationship asynchronous and without an inverse is a good practice for performance optimization and simplifying model relationships where a direct link back is not necessary.
63-63: Approved: Asynchronous relationship for updatedByUser.
The update to make the updatedByUser relationship asynchronous and without an inverse is beneficial for UI responsiveness and simplifies the model design when a back-reference is not required.
66-66: Approved: Asynchronous relationship for createdByUser.
The update to make the createdByUser relationship asynchronous and without an inverse is beneficial for UI responsiveness and simplifies the model design when a back-reference is not required.
app/components/sso-settings/index.js (2)
23-23: Good addition of the tracked property for SSO data.
The addition of the @tracked sso = null; property is well-implemented. It ensures that any changes to the sso property will automatically trigger the UI to update, which is essential for reactive data handling in Ember.js.
88-90: Correct implementation of the computed property for IdP metadata readiness.
The isIdPMetadataReady computed property is well-implemented. It effectively checks both the presence of idpMetadata and the non-active state of the getIdPMetadata task, ensuring that the UI components dependent on this data can react appropriately.
SmitGala
force-pushed
the
PD-1560-Fix-Deprecation-in-Organization-and-Service-Account
branch
2 times, most recently
from
43e56ca to
171408f
Compare
app/components/organization-team/project-list/project-info/index.ts
Outdated
Show resolved
Hide resolved
SmitGala
force-pushed
the
PD-1560-Fix-Deprecation-in-Organization-and-Service-Account
branch
from
171408f to
57a75c3
Compare
|
future-pirate-king
deleted the
PD-1560-Fix-Deprecation-in-Organization-and-Service-Account
branch
No description provided.