-
Notifications
You must be signed in to change notification settings - Fork 471
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
Delete Account and Data #931
Changes from 4 commits
45d8e77
b0fe933
094fba5
650d26e
b5ccf01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,187 @@ | ||
import 'package:firebase_auth/firebase_auth.dart'; | ||
import 'package:flutter/material.dart'; | ||
import 'package:friend_private/backend/http/api/users.dart'; | ||
import 'package:friend_private/backend/preferences.dart'; | ||
import 'package:friend_private/main.dart'; | ||
import 'package:friend_private/utils/analytics/mixpanel.dart'; | ||
import 'package:friend_private/utils/other/temp.dart'; | ||
import 'package:friend_private/widgets/dialog.dart'; | ||
import 'package:gradient_borders/gradient_borders.dart'; | ||
|
||
class DeleteAccount extends StatefulWidget { | ||
const DeleteAccount({super.key}); | ||
|
||
@override | ||
State<DeleteAccount> createState() => _DeleteAccountState(); | ||
} | ||
|
||
class _DeleteAccountState extends State<DeleteAccount> { | ||
bool checkboxValue = false; | ||
bool isDeleteing = false; | ||
|
||
void updateCheckboxValue(bool value) { | ||
setState(() { | ||
checkboxValue = value; | ||
}); | ||
} | ||
|
||
Future deleteAccountNow() async { | ||
setState(() { | ||
isDeleteing = true; | ||
}); | ||
MixpanelManager().deleteAccountConfirmed(); | ||
MixpanelManager().deleteUser(); | ||
SharedPreferencesUtil().clear(); | ||
await deleteAccount(); | ||
await FirebaseAuth.instance.signOut(); | ||
setState(() { | ||
isDeleteing = false; | ||
}); | ||
routeToPage(context, const DeciderWidget(), replace: true); | ||
} | ||
Comment on lines
+28
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The - Future deleteAccountNow() async {
- setState(() {
- isDeleteing = true;
- });
- MixpanelManager().deleteAccountConfirmed();
- MixpanelManager().deleteUser();
- SharedPreferencesUtil().clear();
- await deleteAccount();
- await FirebaseAuth.instance.signOut();
- setState(() {
- isDeleteing = false;
- });
- routeToPage(context, const DeciderWidget(), replace: true);
- }
+ Future deleteAccountNow() async {
+ try {
+ setState(() {
+ isDeleting = true;
+ });
+ MixpanelManager().deleteAccountConfirmed();
+ MixpanelManager().deleteUser();
+ SharedPreferencesUtil().clear();
+ await deleteAccount();
+ await FirebaseAuth.instance.signOut();
+ } catch (e) {
+ // Log the exception and show an error message to the user
+ print(e);
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text('An error occurred while deleting your account. Please try again.')),
+ );
+ } finally {
+ setState(() {
+ isDeleting = false;
+ });
+ }
+ if (!isDeleting) {
+ routeToPage(context, const DeciderWidget(), replace: true);
+ }
+ } |
||
|
||
@override | ||
Widget build(BuildContext context) { | ||
return PopScope( | ||
canPop: isDeleteing, | ||
child: Scaffold( | ||
backgroundColor: Theme.of(context).colorScheme.primary, | ||
appBar: AppBar( | ||
backgroundColor: Theme.of(context).colorScheme.primary, | ||
title: const Text('Delete Account'), | ||
), | ||
body: Padding( | ||
padding: const EdgeInsets.symmetric(horizontal: 10), | ||
child: Column( | ||
children: [ | ||
const SizedBox( | ||
height: 10, | ||
), | ||
const Padding( | ||
padding: EdgeInsets.symmetric(horizontal: 50), | ||
child: Text( | ||
"Are you sure you want to delete your account?", | ||
style: TextStyle( | ||
fontSize: 24, | ||
), | ||
textAlign: TextAlign.center, | ||
), | ||
), | ||
const SizedBox( | ||
height: 10, | ||
), | ||
const Text( | ||
"This cannot be undone.", | ||
style: TextStyle(fontSize: 18), | ||
), | ||
const SizedBox( | ||
height: 30, | ||
), | ||
const ListTile( | ||
leading: Icon(Icons.message_rounded), | ||
title: Text("All of your memories and conversations will be permanently erased."), | ||
), | ||
const ListTile( | ||
leading: Icon(Icons.person_pin_outlined), | ||
title: Text("Your plugins and integrations will be disconnected effectively immediately."), | ||
), | ||
const ListTile( | ||
leading: Icon(Icons.upload_file_outlined), | ||
title: Text( | ||
"You can export your data before deleting your account, but once deleted, it cannot be recovered."), | ||
), | ||
const Spacer(), | ||
Row( | ||
children: [ | ||
Checkbox( | ||
value: checkboxValue, | ||
onChanged: (value) { | ||
if (value != null) { | ||
updateCheckboxValue(value); | ||
} | ||
}, | ||
), | ||
SizedBox( | ||
width: MediaQuery.of(context).size.width * 0.80, | ||
child: const Text( | ||
"I understand that deleting my account is permanent and all data, including memories and conversations, will be lost and cannot be recovered. "), | ||
), | ||
], | ||
), | ||
const SizedBox( | ||
height: 30, | ||
), | ||
isDeleteing | ||
? const CircularProgressIndicator( | ||
color: Colors.white, | ||
) | ||
: Container( | ||
decoration: BoxDecoration( | ||
border: const GradientBoxBorder( | ||
gradient: LinearGradient(colors: [ | ||
Color.fromARGB(127, 208, 208, 208), | ||
Color.fromARGB(127, 188, 99, 121), | ||
Color.fromARGB(127, 86, 101, 182), | ||
Color.fromARGB(127, 126, 190, 236) | ||
]), | ||
width: 2, | ||
), | ||
borderRadius: BorderRadius.circular(12), | ||
), | ||
child: ElevatedButton( | ||
onPressed: () { | ||
if (checkboxValue) { | ||
showDialog( | ||
context: context, | ||
builder: (c) { | ||
return getDialog(context, () { | ||
MixpanelManager().deleteAccountCancelled(); | ||
Navigator.of(context).pop(); | ||
}, () { | ||
deleteAccountNow(); | ||
Navigator.of(context).pop(); | ||
}, "Are you sure?\n", | ||
"This action is irreversible and will permanently delete your account and all associated data. Are you sure you want to proceed?", | ||
okButtonText: 'Delete Now', cancelButtonText: 'Go Back'); | ||
}); | ||
} else { | ||
ScaffoldMessenger.of(context).showSnackBar( | ||
const SnackBar( | ||
content: Text( | ||
'Check the box to confirm you understand that deleting your account is permanent and irreversible.'), | ||
), | ||
); | ||
} | ||
}, | ||
style: ElevatedButton.styleFrom( | ||
backgroundColor: Colors.transparent, | ||
shadowColor: const Color.fromARGB(255, 17, 17, 17), | ||
shape: RoundedRectangleBorder( | ||
borderRadius: BorderRadius.circular(12), | ||
), | ||
), | ||
child: Container( | ||
width: double.infinity, | ||
height: 45, | ||
alignment: Alignment.center, | ||
child: const Text( | ||
'Delete Account', | ||
style: TextStyle( | ||
fontWeight: FontWeight.w400, | ||
fontSize: 18, | ||
color: Color.fromARGB(255, 255, 255, 255), | ||
), | ||
), | ||
), | ||
), | ||
), | ||
Comment on lines
+114
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The UI code for the delete button is quite complex and could be simplified by extracting it into a separate widget. This would improve readability and maintainability. - isDeleteing
- ? const CircularProgressIndicator(
- color: Colors.white,
- )
- : Container(
- decoration: BoxDecoration(
- border: const GradientBoxBorder(
- gradient: LinearGradient(colors: [
- Color.fromARGB(127, 208, 208, 208),
- Color.fromARGB(127, 188, 99, 121),
- Color.fromARGB(127, 86, 101, 182),
- Color.fromARGB(127, 126, 190, 236)
- ]),
- width: 2,
- ),
- borderRadius: BorderRadius.circular(12),
- ),
- child: ElevatedButton(
- onPressed: () {
- if (checkboxValue) {
- showDialog(
- context: context,
- builder: (c) {
- return getDialog(context, () {
- MixpanelManager().deleteAccountCancelled();
- Navigator.of(context).pop();
- }, () {
- deleteAccountNow();
- Navigator.of(context).pop();
- }, "Are you sure?\n",
- "This action is irreversible and will permanently delete your account and all associated data. Are you sure you want to proceed?",
- okButtonText: 'Delete Now', cancelButtonText: 'Go Back');
- });
- } else {
- ScaffoldMessenger.of(context).showSnackBar(
- const SnackBar(
- content: Text(
- 'Check the box to confirm you understand that deleting your account is permanent and irreversible.'),
- ),
- );
- }
- },
- style: ElevatedButton.styleFrom(
- backgroundColor: Colors.transparent,
- shadowColor: const Color.fromARGB(255, 17, 17, 17),
- shape: RoundedRectangleBorder(
- borderRadius: BorderRadius.circular(12),
- ),
- ),
- child: Container(
- width: double.infinity,
- height: 45,
- alignment: Alignment.center,
- child: const Text(
- 'Delete Account',
- style: TextStyle(
- fontWeight: FontWeight.w400,
- fontSize: 18,
- color: Color.fromARGB(255, 255, 255, 255),
- ),
- ),
- ),
- ),
- ),
+ DeleteAccountButton(isDeleting: isDeleting, checkboxValue: checkboxValue, deleteAccountNow: deleteAccountNow) In the above diff, |
||
const SizedBox( | ||
height: 70, | ||
), | ||
], | ||
), | ||
), | ||
), | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,8 @@ import 'package:friend_private/pages/settings/recordings_storage_permission.dart | |
import 'package:friend_private/pages/speech_profile/page.dart'; | ||
import 'package:friend_private/utils/analytics/mixpanel.dart'; | ||
import 'package:friend_private/utils/other/temp.dart'; | ||
import 'package:friend_private/widgets/dialog.dart'; | ||
import 'package:url_launcher/url_launcher.dart'; | ||
|
||
import 'delete_account.dart'; | ||
|
||
class ProfilePage extends StatefulWidget { | ||
const ProfilePage({super.key}); | ||
|
@@ -205,19 +205,7 @@ class _ProfilePageState extends State<ProfilePage> { | |
), | ||
onTap: () { | ||
MixpanelManager().pageOpened('Profile Delete Account Dialog'); | ||
showDialog( | ||
context: context, | ||
builder: (ctx) { | ||
return getDialog( | ||
context, | ||
() => Navigator.of(context).pop(), | ||
() => launchUrl(Uri.parse('mailto:[email protected]?subject=Delete%20My%20Account')), | ||
'Deleting Account?', | ||
'Please send us an email at [email protected]', | ||
okButtonText: 'Open Email', | ||
singleButton: false, | ||
); | ||
}); | ||
Navigator.push(context, MaterialPageRoute(builder: (context) => const DeleteAccount())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The change from a dialog to a new page for account deletion is a significant UX change. It's important to ensure that the - Navigator.push(context, MaterialPageRoute(builder: (context) => const DeleteAccount()));
+ try {
+ Navigator.push(context, MaterialPageRoute(builder: (context) => const DeleteAccount()));
+ } catch (e) {
+ // Handle navigation error here
+ } |
||
}, | ||
), | ||
ListTile( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,8 +140,7 @@ class MixpanelManager { | |
|
||
void bottomNavigationTabClicked(String tab) => track('Bottom Navigation Tab Clicked', properties: {'tab': tab}); | ||
|
||
void deviceConnected() => | ||
track('Device Connected', properties: { | ||
void deviceConnected() => track('Device Connected', properties: { | ||
..._preferences.btDeviceStruct.toJson(fwverAsString: true), | ||
}); | ||
|
||
|
@@ -278,4 +277,12 @@ class MixpanelManager { | |
void assignedSegment(String assignType) => track('Assigned Segment $assignType'); | ||
|
||
void unassignedSegment() => track('Unassigned Segment'); | ||
|
||
void deleteAccountClicked() => track('Delete Account Clicked'); | ||
|
||
void deleteAccountConfirmed() => track('Delete Account Confirmed'); | ||
|
||
void deleteAccountCancelled() => track('Delete Account Cancelled'); | ||
|
||
void deleteUser() => _mixpanel?.getPeople().deleteUser(); | ||
Comment on lines
+280
to
+287
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new analytics tracking functions for account deletion events look good. However, I would suggest adding some error handling around the - void deleteUser() => _mixpanel?.getPeople().deleteUser();
+ void deleteUser() {
+ try {
+ _mixpanel?.getPeople().deleteUser();
+ } catch (e) {
+ print('Failed to delete user from Mixpanel: $e');
+ }
+ } This way, if the deletion fails, the error will be logged and the app will continue running. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ getDialog( | |
String content, { | ||
bool singleButton = false, | ||
String okButtonText = 'Ok', | ||
String cancelButtonText = 'Cancel', | ||
}) { | ||
var actions = singleButton | ||
? [ | ||
|
@@ -23,7 +24,7 @@ getDialog( | |
: [ | ||
TextButton( | ||
onPressed: () => onCancel(), | ||
child: const Text('Cancel', style: TextStyle(color: Colors.white)), | ||
child: Text(cancelButtonText, style: TextStyle(color: Colors.white)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The change is good as it allows customization of the cancel button text. However, consider handling the case where - child: Text(cancelButtonText, style: TextStyle(color: Colors.white)),
+ child: Text(cancelButtonText ?? 'Cancel', style: TextStyle(color: Colors.white)), This way, if |
||
), | ||
TextButton( | ||
onPressed: () => onConfirm(), child: Text(okButtonText, style: const TextStyle(color: Colors.white))), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,3 +43,34 @@ def update_person(uid: str, person_id: str, name: str): | |
def delete_person(uid: str, person_id: str): | ||
person_ref = db.collection('users').document(uid).collection('people').document(person_id) | ||
person_ref.update({'deleted': True}) | ||
|
||
|
||
def delete_user_data(uid: str): | ||
user_ref = db.collection('users').document(uid) | ||
memories_ref = user_ref.collection('memories') | ||
# delete all memories | ||
batch = db.batch() | ||
for doc in memories_ref.stream(): | ||
batch.delete(doc.reference) | ||
batch.commit() | ||
# delete chat messages | ||
messages_ref = user_ref.collection('messages') | ||
batch = db.batch() | ||
for doc in messages_ref.stream(): | ||
batch.delete(doc.reference) | ||
batch.commit() | ||
# delete facts | ||
batch = db.batch() | ||
facts_ref = user_ref.collection('facts') | ||
for doc in facts_ref.stream(): | ||
batch.delete(doc.reference) | ||
batch.commit() | ||
# delete processing memories | ||
processing_memories_ref = user_ref.collection('processing_memories') | ||
batch = db.batch() | ||
for doc in processing_memories_ref.stream(): | ||
batch.delete(doc.reference) | ||
batch.commit() | ||
Comment on lines
+48
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current approach to deleting user data involves multiple batch operations, each of which is committed separately. This could lead to partial deletion if an error occurs during one of the batch commits. To ensure atomicity, consider using a single batch operation for all deletions. - # delete all memories
- batch = db.batch()
- for doc in memories_ref.stream():
- batch.delete(doc.reference)
- batch.commit()
- # delete chat messages
- messages_ref = user_ref.collection('messages')
- batch = db.batch()
- for doc in messages_ref.stream():
- batch.delete(doc.reference)
- batch.commit()
- # delete facts
- batch = db.batch()
- facts_ref = user_ref.collection('facts')
- for doc in facts_ref.stream():
- batch.delete(doc.reference)
- batch.commit()
- # delete processing memories
- processing_memories_ref = user_ref.collection('processing_memories')
- batch = db.batch()
- for doc in processing_memories_ref.stream():
- batch.delete(doc.reference)
- batch.commit()
+ # Initialize a single batch operation
+ batch = db.batch()
+
+ # Delete all memories
+ for doc in memories_ref.stream():
+ batch.delete(doc.reference)
+
+ # Delete chat messages
+ messages_ref = user_ref.collection('messages')
+ for doc in messages_ref.stream():
+ batch.delete(doc.reference)
+
+ # Delete facts
+ facts_ref = user_ref.collection('facts')
+ for doc in facts_ref.stream():
+ batch.delete(doc.reference)
+
+ # Delete processing memories
+ processing_memories_ref = user_ref.collection('processing_memories')
+ for doc in processing_memories_ref.stream():
+ batch.delete(doc.reference)
+
+ # Commit all deletions in a single batch operation
+ batch.commit() |
||
# delete user | ||
user_ref.delete() | ||
Comment on lines
+74
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deleting the user document from Firestore before ensuring that all associated data has been deleted could lead to orphaned data if an error occurs during the deletion of associated data. Consider moving the user document deletion after all associated data has been successfully deleted. - # delete user
- user_ref.delete()
+ # Delete user after all associated data has been deleted
+ user_ref.delete() |
||
return {'status': 'ok', 'message': 'Account deleted successfully'} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,12 @@ | |
@router.delete('/v1/users/delete-account', tags=['v1']) | ||
def delete_account(uid: str = Depends(auth.get_current_user_uid)): | ||
try: | ||
# Set account as deleted in Firestore | ||
update_user(uid, {"account_deleted": True}) | ||
# TODO: delete user data from the database | ||
delete_user_data(uid) | ||
# delete user from firebase auth | ||
auth.delete_account(uid) | ||
Comment on lines
+20
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The - try:
- delete_user_data(uid)
- # delete user from firebase auth
- auth.delete_account(uid)
- except Exception as e:
- print('delete_account', str(e))
- raise HTTPException(status_code=500, detail=str(e))
+ try:
+ delete_user_data(uid)
+ except Exception as e:
+ print('delete_user_data', str(e))
+ raise HTTPException(status_code=500, detail="Failed to delete user data: " + str(e))
+
+ try:
+ auth.delete_account(uid)
+ except Exception as e:
+ print('delete_account', str(e))
+ raise HTTPException(status_code=500, detail="Failed to delete user account: " + str(e)) |
||
return {'status': 'ok', 'message': 'Account deleted successfully'} | ||
except Exception as e: | ||
print('delete_account', str(e)) | ||
Comment on lines
24
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using - print('delete_account', str(e))
+ logger.error('delete_account: %s', str(e)) |
||
raise HTTPException(status_code=500, detail=str(e)) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,3 +86,8 @@ def measure_time(*args, **kw): | |
return result | ||
|
||
return measure_time | ||
|
||
|
||
def delete_account(uid: str): | ||
auth.delete_user(uid) | ||
return {"message": "User deleted"} | ||
Comment on lines
+89
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The + from .user_data import delete_user_data
def delete_account(uid: str):
+ delete_user_data(uid)
auth.delete_user(uid)
return {"message": "User deleted"} Also, consider adding error handling around |
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.
Entelligence.AI
There's a typo in the variable name
isDeleteing
. It should beisDeleting
.