-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: hubspot signup form issue when user is invited #5317
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
hey @jainpawan21 👋
Please make sure to verify these cases:
- Invited the existing user (registered by email) that is currently logged, the form should not be shown.
- Invited the existing user (registered by email) that is not logged in, the form should not be shown.
- Invited the existing user (registered by GH) that is currently logged, the form should not be shown.
- Invited the existing user (registered by GH) that is not logged in, the form should not be shown.
- Invited the new user register by email, the form should be shown.
- Invited the new user register by GH, the form should be shown.
The case number four is failing, attaching the video.
Screen.Recording.2024-03-21.at.11.12.17.mov
I would appreciate if you can cover the new logic with Cypress E2E tests, as far as I see we are lucking tests for the invitation logic, but it would be much appreciated if we can add them. Excluding GH flow as it's complex and requires account confirmation in E2E tests.
@@ -45,7 +45,7 @@ export default function InvitationPage() { | |||
useEffect(() => { | |||
// auto accept invitation when logged in as invited user | |||
if (isLoggedInAsInvitedUser) { | |||
submitToken(tokensRef.current.token as string, tokensRef.current.invitationToken as string, true); | |||
submitToken(tokensRef.current.token as string, tokensRef.current.invitationToken as string, true, false); |
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.
it's hard to understand what are there true/false
arguments, please change it to the object, which will improve readability
} | ||
} | ||
}, [token, navigate, isFromVercel, startVercelSetup]); | ||
|
||
return () => {}; |
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.
not needed
|
||
const { isLoading, mutateAsync, error, isError } = useMutation<string, IResponseError, string>((tokenItem) => | ||
api.post(`/v1/invites/${tokenItem}/accept`, {}) | ||
); | ||
|
||
const submitToken = useCallback( | ||
async (token: string, invitationToken: string, refetch = false) => { | ||
async (token: string, invitationToken: string, refetch = false, isSignUp = true) => { |
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.
by default isSignUp
should be false
, we only have it set to true
in one place - sign up form
@@ -77,9 +77,7 @@ export function SignUpForm({ invitationToken, email }: SignUpFormProps) { | |||
applyToken(token); | |||
|
|||
if (invitationToken) { | |||
submitToken(token, invitationToken); | |||
|
|||
return true; |
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.
why did we remove this return
statement?
@jainpawan21 can we push to merge or close this PR? 🙏 |
Closing this PR. I will create a new PR with requested changes. |
What change does this PR introduce?
Why was this change needed?
Other information (Screenshots)