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 decprected route mixins (ember-simple-auth) #36

Merged
merged 21 commits into from
Mar 19, 2021

Conversation

aatauil
Copy link
Member

@aatauil aatauil commented Feb 26, 2021

No description provided.

Even though index should work as the other routes, the this.currectSession.load function does not get called automatically when redirected by mock-login. 

The requireAuth gets called because when logging out , it will otherwise not redirect you away from the page but stay with every module showing it is disabled
This gives the most issues. When logging out, the sessionInvalidated function should be called but it does not since it thinks the function resides in the session service. So what I think needs to happen is that functions need to be moved to be inside the session service
@aatauil aatauil changed the title [needs work] Replace decprected route mixins (ember-simple-auth) Replace decprected route mixins (ember-simple-auth) Mar 1, 2021
@nvdk
Copy link
Member

nvdk commented Mar 1, 2021

👀 following because we will want to do the same in GN

@aatauil
Copy link
Member Author

aatauil commented Mar 1, 2021

@nvdk

The PR has a pretty good explanation on what to do to upgrade
mainmatter/ember-simple-auth#2198

The only thing that wasn't very clear was replacing the ApplicationRouteMixin. If you want custom logic for e.g. sessionInvalidated, you will now have to create a new service that extends from ember-simple-auth's SessionService and there you can overwrite that method with you custom logic 😄

app/controllers/subsidy/applications/index.js Outdated Show resolved Hide resolved
app/routes/index.js Outdated Show resolved Hide resolved
app/routes/switch-login.js Outdated Show resolved Hide resolved
app/routes/supervision.js Show resolved Hide resolved
app/routes/supervision/submissions/index.js Outdated Show resolved Hide resolved
app/routes/subsidy/applications/index.js Outdated Show resolved Hide resolved
app/services/session.js Outdated Show resolved Hide resolved
app/services/session.js Outdated Show resolved Hide resolved
app/services/session.js Outdated Show resolved Hide resolved
app/routes/application.js Show resolved Hide resolved
@Windvis
Copy link
Contributor

Windvis commented Mar 5, 2021

Optional, and it's probably a nice premature DRY refactor 😄 :

Since the session service is already extended, you could override the requireAuthentication and prohibitAuthentication methods as well and provide default route names.

@aatauil
Copy link
Member Author

aatauil commented Mar 5, 2021

@Windvis Yeah actually that would be nice, but at the same time it should always be flexible to accept other routes in case this is needed in the future ( who knows ). So probable an if (routeIsSpecified) else (goToDefaultRoute) if possible?

@cecemel
Copy link
Member

cecemel commented Mar 17, 2021

The issue with switch-login, is known on chrome, where a pop-up is blocked when coming from a redirect. And will not be tackled here. (Unless someone has an idea) The plan would be to see how we can replace the popup by an iframe.

@HugaertsDries HugaertsDries merged commit 43bb25d into development Mar 19, 2021
@aatauil aatauil deleted the upgrade/auth-mixin branch April 12, 2021 11:17
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.

5 participants