-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix broken tests batch 2 #6573
Fix broken tests batch 2 #6573
Conversation
@@ -134,7 +134,7 @@ const testCases = [ | |||
{ loc: AppPath.InviteTeam, isLoggedIn: true, subscriptionStatus: SubscriptionStatus.Active, onboardingStatus: OnboardingStatus.Completed, res: defaultHomePagePath }, | |||
|
|||
{ loc: AppPath.PlanRequired, isLoggedIn: true, subscriptionStatus: undefined, onboardingStatus: OnboardingStatus.PlanRequired, res: undefined }, | |||
{ loc: AppPath.PlanRequired, isLoggedIn: true, subscriptionStatus: SubscriptionStatus.Canceled, onboardingStatus: OnboardingStatus.Completed, res: undefined }, | |||
{ loc: AppPath.PlanRequired, isLoggedIn: true, subscriptionStatus: SubscriptionStatus.Canceled, onboardingStatus: OnboardingStatus.Completed, res: defaultHomePagePath }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to check with @charlesBochet and @martmull if it still makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should redirect to '/settings/billing' in that case? Or 'undefined' seems like an acceptable option too. This is very theoretical, probably doesn't matter much, but one of the two options would be best imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we should not redirect in order to be able to pay for a new subscription, so undefined is the proper expected response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
The pull request "Fix broken tests batch 2" addresses multiple test files and a utility function to ensure correct behavior and fix broken tests.
/packages/twenty-front/src/hooks/__tests__/usePageChangeEffectNavigateLocation.test.ts
: Updated expected results forAppPath.PlanRequired
whenSubscriptionStatus.Canceled
andOnboardingStatus.Completed
todefaultHomePagePath
./packages/twenty-front/src/modules/activities/hooks/__tests__/useActivities.test.tsx
: ChangedmockActivity
type from 'Note' to 'Task' to align withuseActivities
hook./packages/twenty-front/src/modules/object-metadata/hooks/__tests__/useColumnDefinitionsFromFieldMetadata.test.ts
: Removed a test case and updated expected lengths of definitions./packages/twenty-front/src/modules/object-metadata/hooks/__tests__/useFilteredObjectMetadataItems.test.tsx
: Modified test data forfindObjectMetadataItemById
to ensure correct validation./packages/twenty-front/src/modules/object-metadata/hooks/__tests__/useGetRelationMetadata.test.tsx
: Updated field names in test cases to match metadata structure./packages/twenty-front/src/modules/object-metadata/hooks/__tests__/useObjectMetadataItem.test.tsx
: Updated expected object metadata item ID./packages/twenty-front/src/modules/object-record/spreadsheet-import/__tests__/useOpenObjectRecordsSpreasheetImportDialog.test.tsx
: Updated GraphQL mutation and test cases to include additional fields./packages/twenty-front/src/modules/settings/data-model/fields/preview/utils/__tests__/getFieldPreviewValue.test.ts
: Updated expected results to match example values from settings field type configuration./packages/twenty-front/src/modules/ui/input/editor/utils/getFirstNonEmptyLineOfRichText.ts
: Added check to ensure trimmed text value is not empty before returning./packages/twenty-front/src/utils/format/__tests__/formatDate.test.ts
: Updated expected values for date formatting functions to reflect correct outputs.
10 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks for taking care of this! One small comment but can be merged after that!
As title