Delete Account and Data #931
Conversation
|
WalkthroughThis change introduces a new feature allowing users to delete their accounts. It includes UI updates, analytics tracking, and backend changes for data deletion. The account deletion process is handled securely with Firebase Auth and SharedPreferences. Additionally, the code has been refactored to replace certain dialogs with screens and update Intercom article display methods. Changes
Uplevel your code reviews with Entelligence.AI ProEntelligence.AI ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (8)
- app/lib/pages/settings/delete_account.dart (1)
- app/lib/pages/settings/device_settings.dart (2)
- app/lib/pages/settings/profile.dart (2)
- app/lib/utils/analytics/mixpanel.dart (2)
- app/lib/widgets/dialog.dart (2)
- backend/database/users.py (1)
- backend/routers/users.py (1)
- backend/utils/other/endpoints.py (1)
Review comments generated (18)
- Review: 12
- LGTM: 6
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -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.
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'.
|
|
||
| void deleteAccountClicked() => track('Delete Account Clicked'); | ||
|
|
||
| void deleteAccountConfirmed() => track('Delete Account Confirmed'); | ||
|
|
||
| void deleteAccountCancelled() => track('Delete Account Cancelled'); | ||
|
|
||
| void deleteUser() => _mixpanel?.getPeople().deleteUser(); |
There was a problem hiding this comment.
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.
| import 'package:friend_private/utils/analytics/intercom.dart'; | ||
| import 'package:friend_private/utils/analytics/mixpanel.dart'; | ||
| import 'package:gradient_borders/gradient_borders.dart'; |
There was a problem hiding this comment.
The import statements are not properly formatted. According to the Dart style guide, Dart imports should come before other imports. Also, there's an extra space at the beginning of lines 11 and 12.
- import 'package:friend_private/utils/analytics/intercom.dart';
- import 'package:friend_private/utils/analytics/mixpanel.dart';
- import 'package:gradient_borders/gradient_borders.dart';
+ import 'package:friend_private/utils/analytics/intercom.dart';
+ import 'package:friend_private/utils/analytics/mixpanel.dart';
+ import 'package:gradient_borders/gradient_borders.dart';|
|
||
|
|
||
| def delete_account(uid: str): | ||
| auth.delete_user(uid) | ||
| return {"message": "User deleted"} |
There was a problem hiding this comment.
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.
| delete_user_data(uid) | ||
| # delete user from firebase auth | ||
| auth.delete_account(uid) |
There was a problem hiding this comment.
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))| 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() |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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()|
|
||
| class _DeleteAccountState extends State<DeleteAccount> { | ||
| bool checkboxValue = false; | ||
| bool isDeleteing = false; |
There was a problem hiding this comment.
There's a typo in the variable name isDeleteing. It should be isDeleting.
- bool isDeleteing = false;
+ bool isDeleting = false;| 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); | ||
| } |
There was a problem hiding this comment.
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);
+ }
+ }| 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), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Files selected (1)
- app/lib/pages/settings/profile.dart (2)
Review comments generated (2)
- Review: 1
- LGTM: 1
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
| singleButton: false, | ||
| ); | ||
| }); | ||
| Navigator.push(context, MaterialPageRoute(builder: (context) => const DeleteAccount())); |
There was a problem hiding this comment.
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
+ }
screen-20240927-183921.mp4
Summary by Entelligence.AI
DeleteAccountscreen in the settings, enhancing user experience and control over account deletion.ProfilePageclass to navigate to the newDeleteAccountscreen instead of showing a dialog.DeviceSettingsto useIntercomManager.instance.displayChargingArticlefor better article management.MixpanelManagerclass with functions to handle account deletion events for improved analytics.getDialogfunction indialog.dartto allow dynamic setting of cancel button text.