NIAEFEUP / uni

Mobile app designed to help students of the University of Porto to manage their academic life.
GNU General Public License v3.0
50 stars 18 forks source link

Fixing session and profile dependent providers initialization #1365

Closed DGoiana closed 1 month ago

DGoiana commented 1 month ago

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

codecov[bot] commented 1 month ago

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 ```diff @@ Coverage Diff @@ ## develop #1365 +/- ## ======================================= + Coverage 12% 12% +1% ======================================= Files 266 266 Lines 7194 7198 +4 ======================================= + Hits 802 803 +1 - Misses 6392 6395 +3 ```
thePeras commented 1 month ago

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 commented 1 month ago

Is this approach doing it?

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

limwa commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Hmm, maybe I've confirmed that we do have the rule enabled https://github.com/leancodepl/flutter_corelibrary/blob/master/packages%2Fleancode_lint%2Flib%2Fanalysis_options.yaml#L146