-
Notifications
You must be signed in to change notification settings - Fork 8
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
Feature/delete account #142
Conversation
… were fired on click
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello thank you for this @piotrstanny
This looks great, the service looks perfect but can we add-in a little bit extra error handling to the viewmodel. If you want you could also customize the error depending on the status code in service and then give different messages for different types of errors.
One other git thing is I'm not sure why there are so many file changes to the ios folder?
ios/Flutter/Generated.xcconfig
Outdated
// This is a generated file; do not edit or check into version control. | ||
FLUTTER_ROOT=/Users/tristanclarke/development/flutter | ||
FLUTTER_APPLICATION_PATH=/Users/tristanclarke/development/github.com/now-u/now-u-app | ||
FLUTTER_TARGET=/Users/tristanclarke/development/github.com/now-u/now-u-app/lib/main.dart | ||
FLUTTER_ROOT=/Users/piotrstanny/flutterdev/flutter | ||
FLUTTER_APPLICATION_PATH=/Users/piotrstanny/AndroidStudioProjects/now-u-app | ||
FLUTTER_TARGET=/Users/piotrstanny/AndroidStudioProjects/now-u-app/lib/main.dart | ||
FLUTTER_BUILD_DIR=build | ||
SYMROOT=${SOURCE_ROOT}/../build/ios | ||
OTHER_LDFLAGS=$(inherited) -framework Flutter | ||
FLUTTER_FRAMEWORK_DIR=/Users/tristanclarke/development/flutter/bin/cache/artifacts/engine/ios | ||
FLUTTER_BUILD_NAME=1.0.9 | ||
FLUTTER_BUILD_NUMBER=20 | ||
FLUTTER_BUILD_NAME=1.1.9 | ||
FLUTTER_BUILD_NUMBER=33 | ||
DART_DEFINES=flutter.inspector.structuredErrors%3Dtrue | ||
DART_OBFUSCATION=false | ||
TRACK_WIDGET_CREATION=true | ||
DART_DEFINES=flutter.inspector.structuredErrors=true | ||
TREE_SHAKE_ICONS=false | ||
PACKAGE_CONFIG=/Users/piotrstanny/AndroidStudioProjects/now-u-app/.dart_tool/package_config.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add this to the git ignore
#!/bin/sh | ||
# This is a generated file; do not edit or check into version control. | ||
export "FLUTTER_ROOT=/Users/tristanclarke/development/flutter" | ||
export "FLUTTER_APPLICATION_PATH=/Users/tristanclarke/development/github.com/now-u/now-u-app" | ||
export "FLUTTER_TARGET=/Users/tristanclarke/development/github.com/now-u/now-u-app/lib/main.dart" | ||
export "FLUTTER_ROOT=/Users/piotrstanny/flutterdev/flutter" | ||
export "FLUTTER_APPLICATION_PATH=/Users/piotrstanny/AndroidStudioProjects/now-u-app" | ||
export "FLUTTER_TARGET=/Users/piotrstanny/AndroidStudioProjects/now-u-app/lib/main.dart" | ||
export "FLUTTER_BUILD_DIR=build" | ||
export "SYMROOT=${SOURCE_ROOT}/../build/ios" | ||
export "OTHER_LDFLAGS=$(inherited) -framework Flutter" | ||
export "FLUTTER_FRAMEWORK_DIR=/Users/tristanclarke/development/flutter/bin/cache/artifacts/engine/ios" | ||
export "FLUTTER_BUILD_NAME=1.0.9" | ||
export "FLUTTER_BUILD_NUMBER=20" | ||
export "FLUTTER_BUILD_NAME=1.1.9" | ||
export "FLUTTER_BUILD_NUMBER=33" | ||
export "DART_DEFINES=flutter.inspector.structuredErrors%3Dtrue" | ||
export "DART_OBFUSCATION=false" | ||
export "TRACK_WIDGET_CREATION=true" | ||
export "DART_DEFINES=flutter.inspector.structuredErrors=true" | ||
export "TREE_SHAKE_ICONS=false" | ||
export "PACKAGE_CONFIG=/Users/piotrstanny/AndroidStudioProjects/now-u-app/.dart_tool/package_config.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
lib/services/auth.dart
Outdated
Future<bool> deleteUserAccount() async { | ||
try { | ||
http.Response response = await http.delete( | ||
domainPrefix + 'users/me', | ||
headers: <String, String>{ | ||
'Content-Type': 'application/json; charset=UTF-8', | ||
'token': currentUser.getToken(), | ||
}, | ||
); | ||
if (response.statusCode != 200) { | ||
print("Returned false, no error but status not 200"); | ||
return false; | ||
} | ||
print("Returned true, account should be deleted"); | ||
return true; | ||
} catch (e) { | ||
print("Error"); | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
void delete() async { | ||
setBusy(true); | ||
await _authenticationService.deleteUserAccount(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some error handling in here?
So for example
await _authenticationService.deleteUserAccount(); | |
if (!await _authenticationService.deleteUserAccount()) { | |
_dialog_service -> something when wrong message | |
} else { | |
await _analyticsService.logUserAccountDeleted(); | |
logout(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other thing we should think about is testing. I think it would be nice if we make sure we add tests for each new feature we are working on. Let have a chat about this after Christmas though as I think this is gonna require some thinking?
Yes definitely agree. And as you say we can explore the topic after Christmas. |
* - added account details page - created basic ui - new package for toggle button and created component * - Updated edit account details page - added all text fiels and buttons - removed toggle button from external package * - worked out padding on account details page - added delete text button on account details page * - latest update on profile page * A few profile page fixes * name now saves on profile page * Added dob to account details * Updates to dob * Added location to profile page and remote config * Fix for http in webview issue * Added organisations to home page * Feature/delete account (#142) * Create button action and methods in model and auth confirming methods were fired on click * Logout tested, methods triggering * Log deleting account with analytics * Testing deleting with staging api * Implement confirmation modal confirming deletion * Uncomment all methods and remove testing prints - code ready * Added basic viewmodel test * Added test workflow * Workflow fix * Updated action to run on any pr * Changed flutter version * Remove java and flutter version * Test for failed save * Broke the tests * Assigining new user to _currentUser; Saving user to Shared Preferences * Add appropriate console logs to confirm logging in and sources of getting user * Correct login method to call updateUser instead of directly saving prefs * Remove user from SharedPrefs on account deletion * Minor methods order changed * Display error message if returned false from auth * Error handling with different messages * Model test for account deletion * Unsure * save and delete tests * Started some tests that will be finished later Co-authored-by: jelgar <[email protected]> * Commit from GitHub Actions (Flutter tests) Co-authored-by: tristanclarke <[email protected]> Co-authored-by: Piotr Stanny <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Description
Please include a brief summary of what the issue is and what has been done to solve it
Fixes #83
Checklist:
Creator
Reviewer
After pull