fix: email handling & e2e CI bug

[ShubhamPalriwala]

What does this PR do?

• If SMTP was not configured, a lot of functionality such as an invited user signing up would get blocked because the email service tried to send a "You have a team member" email which threw an error hence resulting in bad UX. This handles that by logging if the email was not sent, hence keeping the core functionality working!
• The playwright config had retries enabled so for failed tests, it assumed it to be flaky and passed, however the tests were actually failing. This fixes that.
• Adds a validation check in the team invite test for a member being removed from the team

Fixes # (issue)

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]

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		Jan 17, 2024 6:27am
formbricks-com	⬜️ Ignored (Inspect)	Visit Preview		Jan 17, 2024 6:27am

[github-actions]

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

[review-agent-prime]

apps/web/playwright/team.spec.ts

There is a lot of repetition in the test code, especially when it comes to the dropdownTrigger and dropdownContentWrapper locators. It would be more maintainable and readable to create a helper function that handles the dropdown menu interaction.
Create Issue
See the diff
Checkout the fix

    async function interactWithDropdown(page: Page) {
      const dropdownTrigger = page.locator("#userDropdownTrigger");
      await expect(dropdownTrigger).toBeVisible();
      await dropdownTrigger.click();

      const dropdownContentWrapper = page.locator("#userDropdownContentWrapper");
      await expect(dropdownContentWrapper).toBeVisible();
    }

git fetch origin && git checkout -b ReviewBot/Impro-6p69d5u origin/ReviewBot/Impro-6p69d5u

packages/lib/emails/emails.ts

The transporter object is created every time the sendEmail function is called. This could be a performance issue if the function is called frequently. It would be more efficient to create the transporter object once and reuse it.
Create Issue
See the diff
Checkout the fix

    let transporter: nodemailer.Transporter | null = null;

    if (IS_SMTP_CONFIGURED) {
      transporter = nodemailer.createTransport({
        host: SMTP_HOST,
        port: SMTP_PORT,
        secure: SMTP_SECURE_ENABLED,
        auth: {
          user: SMTP_USER,
          pass: SMTP_PASSWORD,
        },
      });
    }

    export const sendEmail = async (emailData: sendEmailData) => {
      if (!transporter) {
        console.log(`Could not Email :: SMTP not configured :: ${emailData.subject}`);
        return;
      }

      const emailDefaults = {
        from: `Formbricks <${MAIL_FROM || "noreply@formbricks.com"}>`,
      };

      await transporter.sendMail({ ...emailDefaults, ...emailData });
    };

git fetch origin && git checkout -b ReviewBot/Impro-p87fcne origin/ReviewBot/Impro-p87fcne

playwright.config.ts

Setting the retries to 0 in the test configuration might lead to flaky tests, especially if the tests are run in a CI/CD environment where network conditions might vary. It would be more reliable to allow for a few retries.
Create Issue
See the diff
Checkout the fix

    retries: 2,

git fetch origin && git checkout -b ReviewBot/Impro-lljlyoi origin/ReviewBot/Impro-lljlyoi

[review-agent-prime]

Setting the retries to 2 in the test configuration to improve the reliability of the test suite, especially in a CI/CD environment where network conditions might vary.

Suggested change

	retries: 0,
	retries: 2,

[mattinannt]

@ShubhamPalriwala thank your for this improvement :-) Just two small things I think we should change:

[mattinannt]

please change to console.error

[mattinannt]

Now that you touched this file I would either remove the logging completely or uncomment them.
Or can we maybe couple them with the env variable DEBUG?

logger: process.env.DEBUG === "1",
pass: process.env.DEBUG === "1"

(it would be good to add the debug variable to the constant file in that case to make this easier readable)

I would prefer this option.

[ShubhamPalriwala]

done, please take a look 🤞🏼