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

Change credentials storage to FlutterSecureStorage and remove WillPopScope #935

Closed
wants to merge 9 commits into from

Conversation

Sanat2002
Copy link

Issue #[873,851]
The WillPopScope is removed from the first route and the userCrendentials are secured utilizing flutter secure storage , all read and delete operations on the userCrendentials have been modified accordingly.

@bdmendes
Copy link
Member

bdmendes commented Sep 7, 2023

Hey @Sanat2002,
Thank you very much for your contribution. I'll let the CI run as a first step.
About the code, can you remove the comments, and avoid the bang operator?
About the PR description, please follow the template. You can see examples in open PRs in this repo.

@Sanat2002
Copy link
Author

Sanat2002 commented Sep 9, 2023

Changed the flutter secure storage version. Review this one again so I can fix the problem if it is generated again.

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

@Sanat2002
Copy link
Author

Removed the unused import . Run the check again please

@bdmendes
Copy link
Member

bdmendes commented Sep 9, 2023

@Sanat2002 you can edit the original PR comment instead of creating a new one.

@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Merging #935 (7209a13) into develop (683a6a8) will increase coverage by 1%.
Report is 1 commits behind head on develop.
The diff coverage is 0%.

Additional details and impacted files
@@          Coverage Diff           @@
##           develop   #935   +/-   ##
======================================
+ Coverage       22%    22%   +1%     
======================================
  Files          136    136           
  Lines         3549   3537   -12     
======================================
  Hits           761    761           
+ Misses        2788   2776   -12     

This was linked to issues Sep 9, 2023
@bdmendes
Copy link
Member

bdmendes commented Sep 9, 2023

As a side advice, it's best not to fix two unrelated issues in the same PR like this. It makes it more difficult to reason about the changes.

uni/lib/main.dart Outdated Show resolved Hide resolved
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.

The secure credentials are working well, however there is still an exit confirmation on the app. Search for WillPopScope

@bdmendes bdmendes changed the title issue-fixed 873,851 Change credentials storage to FlutterSecureStorage Dec 5, 2023
@bdmendes bdmendes changed the title Change credentials storage to FlutterSecureStorage Change credentials storage to FlutterSecureStorage and remove WillPopScopre Dec 5, 2023
@bdmendes bdmendes changed the title Change credentials storage to FlutterSecureStorage and remove WillPopScopre Change credentials storage to FlutterSecureStorage and remove WillPopScope Dec 5, 2023
@bdmendes
Copy link
Member

bdmendes commented Dec 5, 2023

Dear @Sanat2002, we'll be working on this due to the lack of activity. Thanks for your contribution!

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.

Remove on will pop from first route Change credentials storage
5 participants