Skip to content

fix: account deletion failing issue#1509

Merged
mattinannt merged 5 commits intomainfrom
account-deletion-issue
Oct 30, 2023
Merged

fix: account deletion failing issue#1509
mattinannt merged 5 commits intomainfrom
account-deletion-issue

Conversation

@Dhruwang
Copy link
Copy Markdown
Member

@Dhruwang Dhruwang commented Oct 30, 2023

What does this PR do?

Account deletion was failing for the ones who were having any of the integrations for which we create a integration object associated with an environment (google sheet or airtable)

So now before deleting an account we, first delete integrations associated with it

Fixes FOR-1462

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change adds a new database migration
  • This change requires a documentation update

How should this be tested?

Connect an integration (GS or airtable)
Try to delete account

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues
  • First PR at Formbricks? Please sign the CLA! Without it we wont be able to merge it 🙏

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

@vercel
Copy link
Copy Markdown

vercel bot commented Oct 30, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
formbricks-cloud ⬜️ Ignored (Inspect) Visit Preview Oct 30, 2023 5:11pm
formbricks-com ⬜️ Ignored (Inspect) Visit Preview Oct 30, 2023 5:11pm

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 30, 2023 •
edited
Loading

Thank you for following the naming conventions for pull request titles! 🙏

@review-agent-prime
Copy link
Copy Markdown
Contributor

review-agent-prime bot commented Oct 30, 2023

The PR looks good overall, but there are a few areas where error handling could be improved.

  1. In the deleteIntegrations function in actions.ts, you are not handling any potential errors that could occur during the deletion of integrations. It would be beneficial to wrap the deleteIntegrationsByEnvironmentId function call in a try-catch block to handle any errors that might occur.
export async function deleteIntegrations(environmentId) {
  const session = await getServerSession(authOptions);
  if (!session) throw new AuthorizationError("Not authorized");

  try {
    return await deleteIntegrationsByEnvironmentId(environmentId);
  } catch (error) {
    console.error(error);
    throw new Error("Failed to delete integrations");
  }
}
  1. In the deleteAccount function in DeleteAccount.tsx, you are not handling any potential errors that could occur during the deletion of the account and integrations. It would be beneficial to wrap the deleteProfileAction and deleteIntegrations function calls in a try-catch block to handle any errors that might occur.
const deleteAccount = async () => {
  try {
    setDeleting(true);
    await deleteIntegrations(environmentId);
    await deleteProfileAction();
    await signOut();
    await formbricksLogout();
  } catch (error) {
    toast.error("Something went wrong: " + error.message);
  } finally {
    setDeleting(false);
    setOpen(false);
  }
};
  1. In the deleteIntegrationsByEnvironmentId function in service.ts, you are not handling any potential errors that could occur during the deletion of integrations. It would be beneficial to wrap the prisma.integration.deleteMany function call in a try-catch block to handle any errors that might occur.
export const deleteIntegrationsByEnvironmentId = async (environmentId: string): Promise<void> => {
  validateInputs([environmentId, ZId]);
  try {
    await prisma.integration.deleteMany({
      where: {
        environmentId,
      },
    });
  } catch (error) {
    if (error instanceof Prisma.PrismaClientKnownRequestError) {
      throw new DatabaseError(error.message);
    }
    console.error(error);
    throw new Error("Failed to delete integrations");
  }
};

These changes will help to ensure that any errors that occur during the deletion process are properly handled and communicated to the user.

mattinannt
mattinannt requested changes Oct 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mattinannt mattinannt left a comment

Choose a reason for hiding this comment

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

@Dhruwang normally we have prisma relations that cascade the delete. e.g. when environment gets deleted, also the surveys get deleted due to this line in the survey model:

  environment         Environment             @relation(fields: [environmentId], references: [id], onDelete: Cascade)

this cascade delete is missing in the integration model. I think it would be the easiest and most consistent way to add this onDelete: Cascade also to the integration model

@Dhruwang Dhruwang requested a review from mattinannt October 30, 2023 10:57
@Dhruwang
Copy link
Copy Markdown
Member Author

Dhruwang commented Oct 30, 2023

Got it, made the changes 😊

mattinannt
mattinannt approved these changes Oct 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mattinannt mattinannt left a comment

Choose a reason for hiding this comment

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

@Dhruwang thanks a lot for the changes :-) 🙏

@mattinannt mattinannt enabled auto-merge October 30, 2023 17:12
@mattinannt mattinannt added this pull request to the merge queue Oct 30, 2023
Merged via the queue into main with commit eec986f Oct 30, 2023
@mattinannt mattinannt deleted the account-deletion-issue branch October 30, 2023 17:26
kevinkong91 added a commit to kevinkong91/formbricks that referenced this pull request Oct 30, 2023
@kevinkong91
Merge branch 'main' into dev
3f28fac 
* main: (28 commits)
  chore: Add Table of Contents to README (formbricks#1427)
  fix: account deletion failing issue (formbricks#1509)
  fix: remove welcome card from email preview (formbricks#1495)
  fix(bug): default role implemented (formbricks#1524)
  fix: changing description of Code Action (formbricks#1522)
  refactor: Migrate activity service (formbricks#1471)
  fix: Error in Docs navigation formbricks#1518 (formbricks#1521)
  feat: dynamic title and description (formbricks#1459)
  fix: Spelling Errors (formbricks#1517)
  fix: added scrollbar whenever overflowed in the settings/profile page (formbricks#1498)
  fix: long url not getting reset after closing modal (formbricks#1502)
  fix: Unexpected Behavior when Toggling Italics in Text Editor and improve clarity of formatting status (formbricks#1506)
  fix: zod pin validation failing (formbricks#1507)
  fix: Error message on Login not shown  (formbricks#1508)
  fix: downgrade nextjs to fix error with react email (formbricks#1516)
  chore: downgrade next version in formbricks-com (formbricks#1513)
  feat: picture selection question (formbricks#1388)
  feat: formtribe leaderboard update as per today (formbricks#1505)
  fix: Added if statement for preventing use of reserved word in question ID (formbricks#1435)
  fix: Disabling Welcome Card leads buggy preview (formbricks#1320)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattinannt mattinannt mattinannt approved these changes

Assignees

No one assigned

Labels

❗️ migrations

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Dhruwang @mattinannt