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 infinite loading #389

Merged
merged 13 commits into from
Feb 21, 2022
Merged

Fix infinite loading #389

merged 13 commits into from
Feb 21, 2022

Conversation

MysticFragilist
Copy link
Contributor

Soooo I think I finally manage to find the bug that was making the cache throw an exception on launch (making the app unusable except if we cleared up the cache and storage of the app and starting over). 😄 @apomalyn look at this thread in flutter_secure_storage repo! It's basically the same error that we have when an error occurs on startup. If this PR don't fix it, we will probably need to review the cache manager entirely. I have hopes tho!

What this means

Right now, if a new version comes out and this bug happens on a device, the secure storage will be entirely wiped which means the user will have to login again.

However, it should prevent any new app download from having this issue from now on because of the change in the AndroidManifest.xml.

closes #387

@MysticFragilist MysticFragilist added bug Something isn't working platform: android labels Feb 8, 2022
@MysticFragilist MysticFragilist requested a review from a team February 8, 2022 03:45
eneiss
eneiss previously approved these changes Feb 8, 2022
Copy link
Contributor

@eneiss eneiss left a comment

Choose a reason for hiding this comment

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

LGTM! Hope this will fix the issue 😊

eneiss
eneiss previously approved these changes Feb 8, 2022
Copy link
Contributor

@eneiss eneiss left a comment

Choose a reason for hiding this comment

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

Looks even better to me 😆💯

Copy link
Member

@apomalyn apomalyn left a comment

Choose a reason for hiding this comment

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

LGTM!! But...
put testing hat
Could you modify the test already in place for the UserRepository so we could check the storage is correctly deleted when the exception is raised please 😄

@apomalyn
Copy link
Member

@MysticFragilist I just changed my phone and I'm hitting this issue, so when this will be available I could test it 😉

@MysticFragilist
Copy link
Contributor Author

@MysticFragilist I just changed my phone and I'm hitting this issue, so when this will be available I could test it 😉

Oh niiiice!

Je vais faire les tests en fin de semaine, ça devrait pas me prendre beaucoup de temps, je ferai une release tout de suite après 😄

@github-actions github-actions bot added size: M and removed size: S labels Feb 20, 2022
@github-actions github-actions bot added size: L and removed size: M labels Feb 20, 2022
Copy link
Contributor

@AlexandreMarkus AlexandreMarkus left a comment

Choose a reason for hiding this comment

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

I have a question, would it be possible, if the error occurs, to delete the secure storage but instead of asking the user to login again, try to login automatically since we already have the username and password in variables?

@@ -11,7 +11,7 @@ import 'package:notredame/core/managers/user_repository.dart';
import 'package:ets_api_clients/models.dart';

// OTHERS
import '../../locator.dart';
import 'package:notredame/locator.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

@MysticFragilist
Copy link
Contributor Author

I have a question, would it be possible, if the error occurs, to delete the secure storage but instead of asking the user to login again, try to login automatically since we already have the username and password in variables?

Yes it would be a great idea! I'll create the issue for this :) though I won't wait for this feature to be implemented as the bug is somewhat critical (some user can't even login and is probably the main cause for bad reviews right now 😅)

Copy link
Member

@apomalyn apomalyn left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@MysticFragilist MysticFragilist merged commit 9873b4b into master Feb 21, 2022
@MysticFragilist MysticFragilist deleted the fix/android-infinite-loading branch February 21, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working platform: android size: L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loading on app launch
4 participants