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 relogin logic #881

Merged
merged 1 commit into from
Jul 29, 2023
Merged

Fix relogin logic #881

merged 1 commit into from
Jul 29, 2023

Conversation

LuisDuarte1
Copy link
Member

@LuisDuarte1 LuisDuarte1 commented Jul 29, 2023

There was an issue with the relogin logic, that breaks almost every fetcher that depends on the student number on the app.

When the user login info is saved, the username is saved as upxxxxxxxxx being x digits. When the app loads, it loads a fake session on SessionProvider.restoreSession() with username being the one loaded from user preferences (so therefore it loads upxxxxxxxxx) , when NetworkRouter.getWithCookies is called for the first time, it will try to do the request but it will know that this session is not valid (because on app start, it will have no cookies) and try to relogin. The cookies are copied from the newSession to the session that was given as an argument. The problem is that, the username stays the same so in the upxxxxxxxxx form, and every api to sigarra that needs a student number expects only the number part, breaking almost all fetchers.

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@LuisDuarte1
Copy link
Member Author

LuisDuarte1 commented Jul 29, 2023

The current fix, is just to copy the newSession username everytime, but maybe the relogin logic needs a refactor, because on app load if there isn't a session we shouldn't create a fake one because it's prone to errors like this one

Copy link
Member

@bdmendes bdmendes left a comment

Choose a reason for hiding this comment

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

Awesome, good catch

@thePeras thePeras merged commit 949d0e2 into develop Jul 29, 2023
4 checks passed
@thePeras thePeras deleted the hotfix/relogin branch July 29, 2023 19:09
@bdmendes
Copy link
Member

@LuisDuarte1 If you want to, please create an issue about the refactor. I think we should try to make an enum with pattern matching: user that can have up or only numbers.

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.

3 participants