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 redirect on first login #287

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Fix redirect on first login #287

merged 2 commits into from
Sep 27, 2024

Conversation

nsklikas
Copy link
Contributor

@nsklikas nsklikas commented Sep 24, 2024

IAM-1039

The issue is fixed by passing the original return to, to the settings flow. I moved some stuff around, to more clearly separate the responsibilities of the frontend and the backend.

There is some reload happening in the setup_secure page when you get there (you momentarily see the spinning thingy). Not sure how to fix it (@edlerd please have a look)

SInce Natalia is not here this week @edlerd and @BarcoMasile please try it out, as I may have broken something

ui/pages/setup_secure.tsx Outdated Show resolved Hide resolved
@lukasSerelis
Copy link

Pasting reply on this issue too for UX requirements

It'd be best from UX point of view to add a confirmation screen before the automatic redirect to the application. So the flow should be something like this: image Or in screens it would be

  1. Trying to sign in for the first time
    image
  2. Required TOTP setup
    image
  3. Setup complete, message to confirm that action has been successful (also good place to show any errors if some occur). And automatic redirect within 3 seconds.
    image
  4. Landing on the 3rd party application page
    image

ui/pages/setup_secure.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Mostly looks good, some non-blocking nitpicks and one concern below.

ui/pages/setup_secure.tsx Outdated Show resolved Hide resolved
ui/pages/setup_secure.tsx Outdated Show resolved Hide resolved
ui/pages/setup_secure.tsx Show resolved Hide resolved
@nsklikas nsklikas force-pushed the IAM-1039 branch 2 times, most recently from 86d8c97 to 3c0b50f Compare September 27, 2024 08:32
Copy link
Contributor

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fixes!

@nsklikas nsklikas merged commit 3883fa3 into main Sep 27, 2024
6 checks passed
@nsklikas nsklikas deleted the IAM-1039 branch September 27, 2024 14:38
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