Skip to content

Conversation

@Dhruwang
Copy link
Member

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

vercel bot commented Oct 30, 2023

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
Contributor

github-actions bot commented Oct 30, 2023

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

@review-agent-prime
Copy link
Contributor

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.

Copy link
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
Copy link
Member Author

Got it, made the changes 😊

Copy link
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
* 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants