Skip to content
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: Warning banner & post-login redirect both fail when starting Take Now while already logged into Web App as wrong respondent (M2-8029) #539

Merged

Conversation

LashaunnaS
Copy link
Contributor

@LashaunnaS LashaunnaS commented Oct 22, 2024

📝 Description

🔗 Jira Ticket M2-8029

  • Fix to the useSessionBanners hook to ensure that logic such as banner removal only occurs after authorization has changed. Replacing the useEffect with useMemo should ensure that any side effects that get triggered are due the state of the application once any further auth logic has been resolved.
  • Fix to redirecting the user after correct authentication to navigate them to the 'Take Now' route.

📸 Screenshots

Before:

Screen.Recording.2024-10-22.at.17.21.15.mov

After:

Screen.Recording.2024-10-22.at.17.53.42.mov

🪤 Peer Testing

𝐏𝐫𝐞𝐜𝐨𝐧𝐝𝐢𝐭𝐢𝐨𝐧𝐬:

  • Have an applet with at least one Team Member and one Full Account participant.
  • Be already logged into the Web App as the Team Member.

𝐒𝐭𝐞𝐩𝐬 𝐭𝐨 𝐫𝐞𝐩𝐫𝐨𝐝𝐮𝐜𝐞:

  • Perform Take Now on any activity, specifying the Full Account as the respondent, and anyone as the subject (self-report is otherwise).
  • Observe the banner briefly shown at the top of the Web App when redirected to login screen.
  • Log into Web App as the Full Account.
  • Observe the next page you are led to in the Web App.

… removal only occurs after authorization has changed. Replacing the useEffect with useMemo should ensure that any side effects that get triggered are due the state of the application once any further auth logic has been resolved.
…uthenticated user, the newly authenticated user will get redirected to the route originally specified.
@LashaunnaS LashaunnaS self-assigned this Oct 22, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-539.d15zn9do8xbzga.amplifyapp.com

Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works great! Thanks for adding the needed code to get backRedirectPath to actually be useful (seems it was a loose end that was never fully implemented prior to Metalab coming onto the project). Tested multiple scenarios, including trying to log in as the wrong user, and then finally, the right user, and Take Now was still triggered properly.

I just had one question about the useMemo that I hope you can help me understand!

@farmerpaul farmerpaul self-requested a review October 23, 2024 13:49
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad it's working. I'd still like to get someone else's opinion here – @JustinNusca or @sultanofcardio, what do you think about this change?

Copy link
Contributor

@sultanofcardio sultanofcardio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farmerpaul thanks for the question. I think the particular change you referenced is fine - the useEffect didn't seem to be adding any value so the outcome is the same. In fact, it is what fixes the disappearing banner so great work there @LashaunnaS.

This PR implements a general post-login redirection inside the useLogout hook, which will now apply in all cases of being logged out (not just take now). So if a user is idle while completing an activity and is logged out, it will result in them being redirected to that activity. I think this is a great addition, but is not strictly necessary for take now.

Post-login redirection in the take now scenario was never implemented. It just seems that way because the multiInformantState stays in redux as long as the tab remains open. After logging back in, the process for launching take now just starts over again using that existing state. This still works for me on the dev branch

@mbanting mbanting merged commit de90b96 into dev Nov 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants