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

feat: integrate formbricks in help feedback box #12276

Merged

Conversation

ShubhamPalriwala
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala commented Nov 8, 2023

What does this PR do?

The Help Feedback Box currently sends the data to our email service. This PR sends it to a pre-created Formbricks Survey for easier visibility and availability in the Formbricks App.

image

We use the @formbricks/api package for minimal dependencies and create a response in the app.

Requirement/Documentation

We introduce 2 new environment variables that needs to be set:

  1. FORMBRICKS in the form of [apiHost]+[environmentId]
  2. FORMBRICKS_FEEDBACK_SURVEY_ID in the form of surveyId

For testing, use:

FORMBRICKS=https://app.formbricks.com+cloob69in56synz0fg6l5vqmx
FORMBRICKS_FEEDBACK_SURVEY_ID=cloob6fn356u0nz0fl0mm1wa5

Before merging to main,

  • Create a Survey on Formbricks
  • Create 2 questions (1 Open Text with Question ID as: formbricks-share-comments-question and 1 Rating question with Range of 4 and ID as formbricks-rating-question )

Type of change

  • New feature (non-breaking change which adds functionality)

How should this be tested?

  • Set the env variable as suggested above
  • Fill the Feedback Box inside the Help Button
  • View in the Formbricks App and the response should be reflected

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Copy link

vercel bot commented Nov 8, 2023

@ShubhamPalriwala is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Nov 8, 2023

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

@github-actions github-actions bot added the ❗️ .env changes contains changes to env variables label Nov 8, 2023
Copy link
Contributor

github-actions bot commented Nov 8, 2023

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link

socket-security bot commented Nov 8, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@formbricks/api 1.4.0 network +0 102 kB matthiasnannt

Copy link
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

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

looks very good and minimal, will try today!

@PeerRich
Copy link
Member

PeerRich commented Nov 8, 2023

lets also add userid and email

@PeerRich
Copy link
Member

PeerRich commented Nov 8, 2023

added .env to both prod and staging, added formbricks survey with the question ID

.env.example Outdated
@@ -119,6 +119,12 @@ NEXT_PUBLIC_SENDGRID_SENDER_NAME=
# Used for capturing exceptions and logging messages
NEXT_PUBLIC_SENTRY_DSN=

# Formbricks Experience Management Integration
# [apiHost]+[environmentId]
FORMBRICKS=
Copy link
Member

@PeerRich PeerRich Nov 8, 2023

Choose a reason for hiding this comment

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

does this have to be FORMBRICKS or NEXT_PUBLIC_FORMBRICKS ?

Copy link

vercel bot commented Nov 8, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2023 9:38am

Copy link
Member

@alishaz-polymath alishaz-polymath left a comment

Choose a reason for hiding this comment

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

Tested, works!
NIT: We could also add userId and/or email as a hidden field (auto-populates internally).

Thank you for your contribution 🚀

@ShubhamPalriwala
Copy link
Contributor Author

Hey, thanks for the quick reviews and testing! Glad it works for everyone 🚀
We're currently updating our API endpoint to support the linking of userId and email as well via the API. Will ping the reviewers here once the PR is again ready for review

@PeerRich PeerRich marked this pull request as draft November 9, 2023 10:43
@PeerRich
Copy link
Member

PeerRich commented Nov 9, 2023

draft until API is ready for userID and email

@PeerRich PeerRich marked this pull request as ready for review November 11, 2023 11:17
PeerRich
PeerRich previously approved these changes Nov 11, 2023
Copy link
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

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

env and keys added to vercel prod

@PeerRich PeerRich enabled auto-merge (squash) November 12, 2023 17:19
@PeerRich PeerRich dismissed stale reviews from alishaz-polymath and themself via 778a1cd November 16, 2023 13:09
.env.example Outdated
@@ -120,6 +120,12 @@ NEXT_PUBLIC_SENDGRID_SENDER_NAME=
# Used for capturing exceptions and logging messages
NEXT_PUBLIC_SENTRY_DSN=

# Formbricks Experience Management Integration
# [apiHost]+[environmentId]
FORMBRICKS=
Copy link
Member

Choose a reason for hiding this comment

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

Not quite since is used on the server only. But I would go with something more descriptive.

Suggested change
FORMBRICKS=
FORMBRICKS_URL=

zomars
zomars previously requested changes Nov 16, 2023
Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 🙏

Quick note. Let's prioritize legibility over clever code in this case. Left some suggestions to address.

.env.example Outdated Show resolved Hide resolved
packages/lib/formbricks.ts Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
@zomars zomars added the Low priority Created by Linear-GitHub Sync label Nov 16, 2023
auto-merge was automatically disabled November 22, 2023 10:47

Head branch was pushed to by a user without write access

.env.example Outdated
@@ -120,6 +120,11 @@ NEXT_PUBLIC_SENDGRID_SENDER_NAME=
# Used for capturing exceptions and logging messages
NEXT_PUBLIC_SENTRY_DSN=

# Formbricks Experience Management Integration
FORMBRICKS_HOST_URL=
Copy link
Member

@PeerRich PeerRich Nov 22, 2023

Choose a reason for hiding this comment

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

Suggested change
FORMBRICKS_HOST_URL=
FORMBRICKS_HOST_URL="https://app.formbricks.com"

@ShubhamPalriwala
Copy link
Contributor Author

update, discussed with @mattinannt and restructuring things a bit

@ShubhamPalriwala
Copy link
Contributor Author

Hey team, pushed the latest commit. Now,

  • Feedback is submitted as a response to Formbricks
  • A person is created in Formbricks when a feedback is submitted with the ID of cal's user.id & attached to the response
  • Person gets their attributes to have an email field as well as a username field that has both values from cal for easier filtering and identification
  • Also updated the env example as suggested above

@mattinannt
Copy link
Contributor

@PeerRich During our testing, we discovered a potential security issue with the Formbricks API updates we pushed to enable this PR. We will update you as soon as this is fixed. You can review this before then, but please don't merge yet.

@mattinannt
Copy link
Contributor

@PeerRich We fixed the issues, re-tested the PR and it's now ready for review 😊

PeerRich
PeerRich previously approved these changes Dec 11, 2023
Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

LGTM

@zomars
Copy link
Member

zomars commented Dec 11, 2023

We need to fix types tho
image

zomars
zomars previously requested changes Dec 11, 2023
Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Type checks are failing

@ShubhamPalriwala
Copy link
Contributor Author

Thanks for the feedback @zomars, I've made the type fixes (apologies for overlooking it) and tested the flow again 💪🏼
cc: @PeerRich

@PeerRich PeerRich dismissed zomars’s stale review December 21, 2023 13:12

review again please

@PeerRich PeerRich enabled auto-merge (squash) January 2, 2024 13:59
@PeerRich PeerRich merged commit f848a44 into calcom:main Jan 2, 2024
14 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗️ .env changes contains changes to env variables Low priority Created by Linear-GitHub Sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants