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

Replace verification form with FEIDE auth #4063

Merged
merged 5 commits into from Aug 9, 2023
Merged

Replace verification form with FEIDE auth #4063

merged 5 commits into from Aug 9, 2023

Conversation

LudvigHz
Copy link
Member

@LudvigHz LudvigHz commented Jul 29, 2023

Description

Feide verification 😬

Result

Start page

image

No valid study programmes:

Screencast.from.2023-07-30.18-12-10.webm

Valid study programme:

Notice that the result from the previous verification is still present. The user may re-verify with correct data to get the proper result.

Screencast.from.2023-07-30.18-14-34.webm

Testing

  • I have thoroughly tested my changes.

Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.


Resolves ABA-521

@linear
Copy link

linear bot commented Jul 29, 2023

@github-actions github-actions bot added the review-needed Pull requests that need review label Jul 29, 2023
Comment on lines +467 to +457
endpoint: `/oidc/validate/?code=${code}&state=${state}`,
method: 'GET',
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm considering changing this to a POST. There isn't really much practical difference here, more of a design choice.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it doesn't matter much but I think a POST would make more sense.

Copy link
Member

Choose a reason for hiding this comment

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

I think keeping it as a GET is fine

@LudvigHz LudvigHz enabled auto-merge July 29, 2023 19:29
@LudvigHz LudvigHz disabled auto-merge July 29, 2023 19:29
@LudvigHz LudvigHz added flawless Pull requests with "flawless" code, e.g. formatting or simple renaming. Should be used with caution! future-was-2-years-ago Pull requests that introduce technologies which should have been "long" ago new-feature Pull requests that introduce a new feature labels Jul 30, 2023
@LudvigHz LudvigHz changed the title [WIP] Replace verification form with FEIDE auth Replace verification form with FEIDE auth Jul 31, 2023
@LudvigHz LudvigHz force-pushed the feide branch 4 times, most recently from c08ec09 to f3a0ad3 Compare July 31, 2023 20:14
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Great work! 🏅

app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
cypress/e2e/router_spec.js Show resolved Hide resolved
app/routes/users/components/UserProfile.css Outdated Show resolved Hide resolved
app/routes/users/components/UserProfile.tsx Outdated Show resolved Hide resolved
app/routes/users/components/UserProfile.tsx Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
@LudvigHz LudvigHz force-pushed the feide branch 2 times, most recently from 8cf213f to 5eeb4da Compare August 7, 2023 09:23
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

I just noticed it would be nice if you would use the Card.Header as card headers instead of a custom one.

app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
app/routes/users/components/UserProfile.css Outdated Show resolved Hide resolved
app/routes/users/components/StudentConfirmation.tsx Outdated Show resolved Hide resolved
Navigation around user settings was a bit difficult and hard to find.
This should make it easier to navigate to the settings page and back.
The user now just authenticates with FEIDE and we handle the rest
automatically.
@LudvigHz LudvigHz merged commit e0e8262 into master Aug 9, 2023
4 checks passed
@LudvigHz LudvigHz deleted the feide branch August 9, 2023 14:16
@falbru
Copy link
Contributor

falbru commented Aug 9, 2023

LGTM :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flawless Pull requests with "flawless" code, e.g. formatting or simple renaming. Should be used with caution! future-was-2-years-ago Pull requests that introduce technologies which should have been "long" ago new-feature Pull requests that introduce a new feature review-needed Pull requests that need review
Projects
None yet
4 participants