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

Fixing session and profile dependent providers initialization #1365

Merged
merged 2 commits into from
Oct 19, 2024

Conversation

DGoiana
Copy link
Collaborator

@DGoiana DGoiana commented Oct 16, 2024

Closes #1364

The ExamProvider is dependent on both the session and profile, so these must be initialized first. In the ensureInitialized function, the loadFromStorage method is called before loadFromRemote. However, the second method was not awaited, which meant that after login, ExamProvider was accessing an empty profile state returned by loadFromStorage. This could not be covered by loadFromRemote, as it would not have completed in time.

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

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 12%. Comparing base (77dca69) to head (4be63c1).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #1365   +/-   ##
=======================================
+ Coverage       12%     12%   +1%     
=======================================
  Files          266     266           
  Lines         7194    7198    +4     
=======================================
+ Hits           802     803    +1     
- Misses        6392    6395    +3     

@DGoiana DGoiana requested a review from a team October 16, 2024 22:18
@thePeras
Copy link
Member

humn, have we disabled the multiple providers parallel loading with this? For example, after session is available and home page is displayed, we want to get exams and lectures in parallel. Is this approach doing it?

@limwa
Copy link
Member

limwa commented Oct 18, 2024

Is this approach doing it?

That's a good point, I don't think providers are doing any parallel loading now

@limwa
Copy link
Member

limwa commented Oct 18, 2024

Nevermind, I think parallel loading is working.

These changes were made to the ensureInitialized method.
This method is used by other providers (and lazy consumers) to ensure that their dependencies initialized before using them. There is no dependency across all providers. If anything, these changes make the method actually correct because:

  1. There are no more mutations of the state without a lock acquisition
  2. Remote loading is now completed before ensureInitialized returns

This does bring another question to mind: don't we have a "no-unawaited-futures" lint enabled? If yes, why wasn't this caught earlier?

@DGoiana
Copy link
Collaborator Author

DGoiana commented Oct 18, 2024

This does bring another question to mind: don't we have a "no-unawaited-futures" lint enabled? If yes, why wasn't this caught earlier?

maybe because loadFromStorage is being awaited and loadFromRemote is inside the then clause

@limwa
Copy link
Member

limwa commented Oct 18, 2024

Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

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

Besides the currently discussing, I will approve this as it works!

@limwa limwa merged commit b6a9273 into develop Oct 19, 2024
6 checks passed
@limwa limwa deleted the fix/exams-load branch October 19, 2024 14:49
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.

On login, exams are not loaded until refresh
3 participants