Skip to content

Conversation

@natuan9
Copy link
Contributor

@natuan9 natuan9 commented Nov 25, 2025

No description provided.

@sbidoul
Copy link
Member

sbidoul commented Nov 26, 2025

@natuan9 Thanks for this migration. We could probably merge this, but it would be nice if you could try to include #821 here. Since in 18 we have deep links, a pure backend redirect should work, and it is simpler and provide a better user experience.

@natuan9 natuan9 force-pushed the 18.0-mig-auth_oauth_autologin branch from 895538c to c637c01 Compare November 27, 2025 12:29
@natuan9
Copy link
Contributor Author

natuan9 commented Nov 27, 2025

Hi @sbidoul , I have refactored the module to use a pure backend redirect
Here is the testing record

v18-autologin.mp4

@sbidoul
Copy link
Member

sbidoul commented Nov 30, 2025

I have some doubts with the redirect implementation, in particular when the user is already logged in .

Could you pick-up the tests from #821 ?

The implementation in #821 is a bit different, as is the original implementation in f25f5d4 which calls super first. I don't know which approach is better in 18.0.

@sbidoul
Copy link
Member

sbidoul commented Nov 30, 2025

Can you also remove the old icon so the bot will generate a new one after merge?

@natuan9 natuan9 force-pushed the 18.0-mig-auth_oauth_autologin branch from c637c01 to 8c3d84e Compare December 4, 2025 10:56
@natuan9 natuan9 force-pushed the 18.0-mig-auth_oauth_autologin branch from 8c3d84e to 7376795 Compare December 4, 2025 10:59
@natuan9
Copy link
Contributor Author

natuan9 commented Dec 4, 2025

Hi @sbidoul , I've removed the icon and added the tests
I didn't call super first, unlike the original implementation, because I want to avoid unnecessary handling from the parent method

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.

6 participants