fix deprecations in analytics, billing & account-settings #1481
Conversation
future-pirate-king
requested review from
Praseetha-KR,
cosmosgenius and
Yibaebi
WalkthroughThis pull request introduces several changes across multiple components in the application. Key modifications include the adjustment of methods for handling personal tokens and multi-factor authentication, the addition of a new registry entry for a date picker component, and updates to subscription handling in the organization billing component. Additionally, there are changes to the template rendering of tables, affecting their styling. The overall structure and functionality of the components remain intact, with no significant alterations to exported or public entities. Changes
Possibly related PRs
Suggested reviewers
Poem
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: |
7bba24d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://310fb04c.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1559-fix-deprecation-anal.irenestaging.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
app/components/organization-billing/index.ts (1)
46-48: LGTM! Consider adding return type annotation.The getter properly handles immutability and null cases. For better type safety, consider adding an explicit return type annotation.
- get subscriptionList() { + get subscriptionList(): SubscriptionModel[] { return this.subscriptions?.slice() || []; }app/components/account-settings/security/multi-factor-auth/index.ts (5)
75-75: Consider a more concise implementation.The double negation could be replaced with Boolean conversion for better readability.
- return !!this.mfas.records?.find((it) => it.enabled === true); + return Boolean(this.mfas.records?.find((it) => it.enabled === true));
Line range hint
1-1000: Consider breaking down this large component.This component is handling multiple responsibilities (email MFA, app MFA, switching between methods, disabling MFA) and has grown quite large. Consider splitting it into smaller, focused sub-components:
- EmailMFAHandler
- AppMFAHandler
- MFASwitcher
- MFADisabler
This would improve maintainability and testability.
Line range hint
32-45: Consider using an enum for modal states.The component uses multiple boolean flags to track modal states, which could lead to inconsistent states. Consider using an enum to represent the modal state:
enum ModalState { None, EmailEnable, EmailSendConfirm, EmailOTPEnter, AppEnable, SwitchToEmail, SwitchToEmailConfirm, SwitchToEmailAppVerify, SwitchToEmailEmailVerify // ... other states } export default class AccountSettingsSecurityMultiFactorAuthComponent extends Component.extend(Evented) { @tracked modalState: ModalState = ModalState.None; // ... }
Line range hint
149-149: Remove debug statements from production code.Debug statements should be removed or wrapped in development-only conditions.
- debug('noMFAEnableEmail: In side otp loop'); + // Use appropriate logging service or remove in productionAlso applies to: 186-186, 428-428, 471-471, 651-651
Line range hint
167-184: Standardize error handling.Error handling patterns vary across different methods. Consider creating a shared error handling utility:
private handleMFAError(error: AdapterError, defaultMessage: string = this.tsomethingWentWrong): void { const errorObj = error.payload || {}; const otpMsg = errorObj.otp?.[0]; if (otpMsg) { this.notify.error(this.tInvalidOTP); } else { this.notify.error(defaultMessage); } }Also applies to: 297-314, 520-537, 744-761
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- app/components/account-settings/developer-settings/personal-token-list/index.ts (1 hunks)
- app/components/account-settings/security/multi-factor-auth/index.ts (1 hunks)
- app/components/ak-date-picker/calendar-nav/index.ts (1 hunks)
- app/components/ak-date-picker/index.hbs (2 hunks)
- app/components/organization-billing/index.ts (2 hunks)
- app/components/organization-billing/invoice-list/index.hbs (1 hunks)
- app/components/organization-billing/subscription/index.hbs (1 hunks)
- app/models/analytics/recent-issue.ts (1 hunks)
🔇 Additional comments (9)
app/models/analytics/recent-issue.ts (1)
1-2: LGTM! Import changes follow TypeScript best practices.The changes properly separate runtime and type-level imports, which is the recommended approach for TypeScript projects.
app/components/organization-billing/invoice-list/index.hbs (1)
9-9: LGTM! Verify deprecation documentation.The change from 'bordered' to 'full-bordered' variant appears to be addressing a deprecation in the AkTable component.
Let's verify if this change is part of documented deprecations:
app/components/ak-date-picker/calendar-nav/index.ts (1)
38-38: LGTM! Consider documenting deprecation timeline.The addition of the kebab-case path format (
ak-date-picker/calendar-nav) alongside the existing double-colon format (AkDatePicker::CalendarNav) is a good practice for deprecation handling. This allows for a gradual migration while maintaining backward compatibility.Consider adding a comment indicating when the
::syntax will be deprecated, helping other developers plan their migrations.Let's verify the component usage in templates:
✅ Verification successful
Component registration is correct and follows migration pattern
The verification confirms that:
- The new kebab-case path (
ak-date-picker/calendar-nav) is correctly referenced in the main component'snavComponentgetter- The component is properly registered in Glint for both syntaxes
- All current template usage is using the
AkDatePickersyntax, indicating this is a forward-compatible change for future migrations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check current usage patterns of the calendar nav component # This helps ensure we know which syntax is currently in use # Search for both old and new component invocation syntaxes echo "Checking component usage patterns..." rg -g '*.hbs' -A 2 'AkDatePicker::CalendarNav|ak-date-picker/calendar-nav'Length of output: 159
Script:
#!/bin/bash # Let's try a broader search to understand the component usage patterns # First, search for any ak-date-picker related imports or registrations echo "Checking component registrations and imports..." rg -g '*.{ts,js}' "AkDatePickerCalendarNav|ak-date-picker/calendar-nav" # Also search for potential template usage with more variations echo -e "\nChecking template usage with broader patterns..." rg -g '*.hbs' "AkDatePicker|ak-date-picker" # Look for any Glint registry entries related to date picker echo -e "\nChecking Glint registry entries..." rg -g '*.{ts,js}' "'ak-date-picker|AkDatePicker" -A 2 -B 2Length of output: 11260
app/components/organization-billing/subscription/index.hbs (1)
3-3: LGTM! Verify similar components for consistency.The update from
'bordered'to'full-bordered'appears to address the deprecation correctly.Let's verify if other table components need similar updates:
✅ Verification successful
No other tables using deprecated 'bordered' variant found
The search results confirm that no other tables in the codebase are using the deprecated 'bordered' variant. The AkTable components found use either 'full-bordered' or 'semi-bordered' variants. The change from 'bordered' to 'full-bordered' in this file is consistent with the variants used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other tables that might need the variant update # Test: Look for AkTable components still using the deprecated 'bordered' variant rg -g '*.hbs' "AkTable.*@variant='bordered'"Length of output: 52
Script:
#!/bin/bash # Let's try a broader search to find all AkTable variant usages rg -g '*.hbs' "@variant=['\"].*['\"]" -A 1 -B 1Length of output: 206500
Script:
#!/bin/bash # Also search for any AkTable components to ensure we're not missing any patterns rg -g '*.hbs' "<AkTable" -A 2 -B 1Length of output: 15286
app/components/organization-billing/index.ts (3)
54-56: LGTM! Good simplification.The change improves readability and efficiency by leveraging the
subscriptionListgetter instead of duplicating the slice logic.
66-68: Verify the type safety of sortBy method.While the change correctly ensures immutability by copying before sorting, let's verify the type safety of the
sortBymethod which appears to be an Ember extension.#!/bin/bash # Description: Check the origin and type definitions of sortBy # Test 1: Look for sortBy type definitions rg -t ts "sortBy" types/ # Test 2: Check for other usages of sortBy in the codebase ast-grep --pattern 'sortBy($$$)'
50-52: Verify handling of undefined subscription.The change from
firstObjectto array indexing is good, but ensure that consumers of this getter properly handle the undefined case when there are no subscriptions.✅ Verification successful
No issues with undefined subscription handling
The code properly handles the case when there is no subscription:
- The parent component uses
hasSubscriptionguard before rendering the subscription component- The subscription component is designed to handle undefined subscription through optional chaining
- The getter
hasSubscriptioncheckssubscriptionCount > 0before using the subscription🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the undefined subscription case is handled in the codebase # Test: Search for direct usage of the subscription getter rg -t ts -t hbs "subscription\s*[^L]" app/components/organization-billing/Length of output: 59
Script:
#!/bin/bash # Let's try a more comprehensive search to find usages of the subscription getter # Search for subscription getter usage in TypeScript files rg -t ts "\.subscription\b" app/components/organization-billing/ # Search in template files rg -t hbs "subscription" app/components/organization-billing/ # Also check the component's template directly fd "organization-billing.hbs" app/components/ --exec cat {}Length of output: 1150
Script:
#!/bin/bash # Let's check how the subscription component handles undefined values # Check the subscription component's TypeScript implementation cat app/components/organization-billing/subscription/index.ts # Check if there are any conditional checks before using the subscription in the parent component cat app/components/organization-billing/index.hbsLength of output: 4102
Script:
#!/bin/bash # Let's check the implementation of hasSubscription and hasNoSubscription getters cat app/components/organization-billing/index.tsLength of output: 2927
app/components/ak-date-picker/index.hbs (2)
1-1: LGTM! Modern approach to component instantiation.The direct usage of the component helper is cleaner and follows current Ember.js best practices.
29-29: LGTM! Enhanced component resolution.Using the component helper for
@navComponentimproves flexibility by supporting both string names and component definitions.
app/components/account-settings/developer-settings/personal-token-list/index.ts
Show resolved
Hide resolved
Irene
|
||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1559-fix-deprecation-analytics-billing-ac-settings
|
| Run status |
|
| Run duration | 13m 32s |
| Commit |
|
| Committer | Sam David |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
31
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Tests for review
future-pirate-king
force-pushed
the
PD-1559-fix-deprecation-analytics-billing-ac-settings
branch
from
c4a5004 to
7bba24d
Compare
|
future-pirate-king
deleted the
PD-1559-fix-deprecation-analytics-billing-ac-settings
branch
No description provided.