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: fixed magic link issues in dev environment #32

Closed
wants to merge 24 commits into from

Conversation

JazzarKarim
Copy link
Collaborator

@JazzarKarim JazzarKarim commented Nov 27, 2024

Issue #: /bcgov/entity#24363

  • Fixed a lot of bugs with the magic link that've been occurring in DEV
  • Refactored the whole flow; made things much cleaner
  • Fixed unit tests

image

I've been getting page not found non-stop in DEV. I changed the whole flow to make things cleaner and simpler.

New flow:

  • I removed the old directory + file. I deal now everything in index.vue (if there is a magic link). It's all conditional.
  • I've also noticed something. Before, when we click on the magic link, it sticks in the browser:
    image
    If a user presses refresh by mistake for example, the link will run again. That is not ideal.
  • New flow: Magic link is pressed --> Set the token in session storage (using a middleware) --> Redirect to home --> Check if token exists in session storage --> Yes? then process it then clear it from season storage. No? then skip the process.

@JazzarKarim JazzarKarim self-assigned this Nov 27, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@JazzarKarim JazzarKarim changed the title testing config values fix: fixed magic link issues in dev environment Nov 29, 2024
@JazzarKarim
Copy link
Collaborator Author

/gcbrun

@bcgov bcgov deleted a comment from bcregistry-sre Nov 29, 2024
@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://business-registry-dev--pr-32-64iszjjg.web.app

@severinbeauvais
Copy link
Collaborator

Karim, on the preview link, are you able to search for businesses (to affiliate them)?

@JazzarKarim
Copy link
Collaborator Author

Karim, on the preview link, are you able to search for businesses (to affiliate them)?

No Sev, I can't. I've been using kind of a workaround to test this.

What I'm doing is I'm going to the regular dev, https://business-registry-dev.web.app/en-CA, do the steps, get the link there by email, then replace the dev URL with the one here before the token.

@bcgov bcgov deleted a comment from bcregistry-sre Dec 2, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Dec 2, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Dec 2, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Dec 2, 2024
@JazzarKarim
Copy link
Collaborator Author

/gcbrun

@bcgov bcgov deleted a comment from bcregistry-sre Dec 2, 2024
@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://business-registry-dev--pr-32-64iszjjg.web.app

@JazzarKarim JazzarKarim marked this pull request as ready for review December 2, 2024 22:10
@JazzarKarim
Copy link
Collaborator Author

JazzarKarim commented Dec 2, 2024

@severinbeauvais I haven't been able to fix the search issue in the preview link 😞

I'm pretty sure the issue is with the key. I've tried the API key I have locally and the one from 1Pass, both didn't work. I'm probably going to ping Thor or Dietrich and see what they think.

@@ -3,7 +3,7 @@ export default defineNuxtRouteMiddleware((to) => {
const { $keycloak } = useNuxtApp()
const localePath = useLocalePath()
if (to.meta.order !== 0 && !$keycloak.authenticated) {
return navigateTo(localePath('/'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this old function do anything extra (like set URL parameters)?

Does it matter that the old code sometimes returns something and the new code doesn't?

Copy link
Collaborator Author

@JazzarKarim JazzarKarim Dec 2, 2024

Choose a reason for hiding this comment

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

So I changed this Sev to use the new window.location.href only if the URL has a token.

What the old one is doing basically is the same as the new one except for the fact that window.location.href = localePath('/') causes a full page reload which is needed in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still needed?

if (token) {
sessionStorage.setItem('affiliationToken', token)
}
window.location.href = localePath('/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, if the path contains a token then you save it to session storage and then reload everything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, exactly.

Copy link
Collaborator

@severinbeauvais severinbeauvais Dec 2, 2024

Choose a reason for hiding this comment

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

Why so complicated? Why can't the token be handled directly? Why is a reload needed? Could you just push a new route?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because any route that is not that the main route will give a page 404 error:
image

Whenever that link is pressed, we will get an error if we don't handle it using a middleware or something else. It's not possible to handle it from the index.vue file directly as we won't even make it to that file.

When I tried handling that via an actual route, I was getting that error as well since the email returned has a double slash //. The browser was standardizing the URL and removing org ID part. It's a weird thing really. That's why I moved away from that design as I tried countless things but couldn't manage to deal with all the edge cases properly.

Copy link

Choose a reason for hiding this comment

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

This shouldn't be an issue now

Copy link
Collaborator

@severinbeauvais severinbeauvais Dec 4, 2024

Choose a reason for hiding this comment

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

I love the new design!

Please resolve this conversation.

I want to understand the new design.

console.error('Error accepting affiliation invitation:', e)
}
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love this design of "check URL parameters + set session storage + reload + check session storage + handle token". It feels complicated and I keep thinking there are edge cases.

Can you check this design with someone else, like Travis?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, will check with Travis now and see what he thinks.

Copy link

Choose a reason for hiding this comment

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

This shouldn't be an issue now

Copy link
Collaborator

@severinbeauvais severinbeauvais Dec 4, 2024

Choose a reason for hiding this comment

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

I love the new design!

Please resolve this conversation.

So what's the new design? Does it still update the URL, reload everything, etc?

@JazzarKarim
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://business-registry-dev--pr-32-64iszjjg.web.app

@JazzarKarim
Copy link
Collaborator Author

I'm closing this PR. This is because the whole thing changed. We're moving away from this sessionStorage solution and instead, the magic link format was changed in AUTH API: bcgov/sbc-auth#3166

So, the new PR and what I'm going to merge is this: #34

@JazzarKarim JazzarKarim closed this Dec 4, 2024
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