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

Delete Account and Data #931

Merged
merged 5 commits into from
Sep 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 187 additions & 0 deletions app/lib/pages/settings/delete_account.dart
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;

Choose a reason for hiding this comment

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

Image description Entelligence.AI

There's a typo in the variable name isDeleteing. It should be isDeleting.

- bool isDeleteing = false;
+ bool isDeleting = 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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

The deleteAccountNow function does not handle exceptions. If any of the operations fail (like deleting the account, signing out, or routing to another page), the application could crash or end up in an inconsistent state. Consider adding error handling to this function.

- 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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

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, DeleteAccountButton is a new widget that encapsulates all the UI logic for the delete button. You would need to create this new widget and move the corresponding code there.

const SizedBox(
height: 70,
),
],
),
),
),
);
}
}
18 changes: 3 additions & 15 deletions app/lib/pages/settings/profile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand Down Expand Up @@ -179,19 +179,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()));

Choose a reason for hiding this comment

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

Image description Entelligence.AI

The change from a dialog to a new page for account deletion is a significant UX change. It's important to ensure that the DeleteAccount page provides clear instructions and warnings to the user about the implications of deleting their account. Also, it would be beneficial to add error handling in case the navigation to the DeleteAccount page fails.

- Navigator.push(context, MaterialPageRoute(builder: (context) => const DeleteAccount()));
+ try {
+   Navigator.push(context, MaterialPageRoute(builder: (context) => const DeleteAccount()));
+ } catch (e) {
+   // Handle navigation error here
+ }

},
)
],
Expand Down
11 changes: 9 additions & 2 deletions app/lib/utils/analytics/mixpanel.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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),
});

Expand Down Expand Up @@ -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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

The new analytics tracking functions for account deletion events look good. However, I would suggest adding some error handling around the deleteUser() function. If the Mixpanel API call fails for any reason (network issues, API changes, etc.), it could cause an unhandled exception and crash the app.

- 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.

}
3 changes: 2 additions & 1 deletion app/lib/widgets/dialog.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ getDialog(
String content, {
bool singleButton = false,
String okButtonText = 'Ok',
String cancelButtonText = 'Cancel',
}) {
var actions = singleButton
? [
Expand All @@ -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)),

Choose a reason for hiding this comment

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

Image description Entelligence.AI

The change is good as it allows customization of the cancel button text. However, consider handling the case where cancelButtonText is null to avoid potential null reference exceptions.

- child: Text(cancelButtonText, style: TextStyle(color: Colors.white)),
+ child: Text(cancelButtonText ?? 'Cancel', style: TextStyle(color: Colors.white)),

This way, if cancelButtonText is not provided, it will default to 'Cancel'.

),
TextButton(
onPressed: () => onConfirm(), child: Text(okButtonText, style: const TextStyle(color: Colors.white))),
Expand Down
31 changes: 31 additions & 0 deletions backend/database/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

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'}
7 changes: 4 additions & 3 deletions backend/routers/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

The delete_user_data(uid) and auth.delete_account(uid) functions are called without any error handling. If an exception occurs in either of these functions, the user will receive a 500 error response, which is not very informative. It would be better to catch exceptions separately for each function call and return more specific error messages.

-    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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

Using print statements for logging errors is not a good practice. It's better to use a logging library that can provide more features like log levels, log to different destinations, and more structured logs.

-    print('delete_account', str(e))
+    logger.error('delete_account: %s', str(e))

raise HTTPException(status_code=500, detail=str(e))


Expand Down
5 changes: 5 additions & 0 deletions backend/utils/other/endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

Image description Entelligence.AI

The delete_account function deletes the user from Firebase Auth but it doesn't seem to delete the user's data from other collections. This could lead to orphaned data in your database. It would be better to call the delete_user_data function before deleting the user account to ensure all related data is cleaned up.

+ 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 auth.delete_user(uid) to handle any exceptions that might occur during the deletion process.

Loading