Skip to content

Conversation

@PratikAwaik
Copy link
Contributor

@PratikAwaik PratikAwaik commented Oct 15, 2023

What does this PR do?

  • Adds notion integration

Fixes #598

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?

  • Test A
  • Test B

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

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 15, 2023

@PratikAwaik is attempting to deploy a commit to the formbricks Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 15, 2023

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

@jobenjada jobenjada changed the title Feat/notion integration feat: Notion Integration Oct 15, 2023
@jobenjada
Copy link
Member

@PratikAwaik thanks a ton, really excited to try this out :)

Can you add a barebones docs page so I know what to do to test this locally? :) I can then just rewrite it into a fully fledged docs page and it can go live together 😍

@PratikAwaik
Copy link
Contributor Author

@jobenjada Sure thing. Will let you know once done

@jobenjada jobenjada self-assigned this Oct 16, 2023
@jobenjada
Copy link
Member

Hey @PratikAwaik - thanks a lot for shipping this :))


  1. Databases not pulled in after setup - delay:

When I set it up, I couldn't select any of the databases for a good minute. Suddenly, it worked. Any idea why this could be? And how we could prevent the impression that it doesnt work?

I think we should have some kind of loading indicator, because when I checked back in later on, I found 20 in the list. We should convey to the user that we're still loading all databases from Notion.


  1. Wording: "Map Formbricks fields to Notion property"

Im not really sure what this tells me :)

image

Oh I think there is the mapping UI missing, no?

image
  1. Error message when editing existing integration
image

When I click on an integration to make changes to it, I'm getting the above error. In order to keep the solution simple, let's just change the error message to this:

"A connection with this database is live. Please make changes with caution."


I wasn't able to test the integration end-to-end.

If you can please also resolve merge conflicts and merge main :)

@jobenjada jobenjada added hacktoberfest complete these issues to gather points for Hacktoberfest hacktoberfest-accepted formtribe-2023 labels Oct 18, 2023
@jobenjada
Copy link
Member

jobenjada commented Oct 19, 2023

Hey!

Now the UI is here :)

Does the Notion API allow to request an updated version of a database? I think we need some kind of 'Refresh' button. I just created new fields in the Notion database but they dont show up in the Formbricks integration:

image

Secondly: Great job on the mapping UI, it looks really slick!! 🔥

In-between testing, the mapping UI got lost:

https://tella.video/johanness-video-8419


Questions do not get reduced from the list when they are selected once:

image

I think it makes sense, maybe people want to add the same info twice. Just double checking with you if this was the intention :)


Just double checking if "URI" is correct or if it should be "URL":

image
image

Great to validate that early! What would I map CTA on? I thought its either boolean ( clicked / dismissed )

Do you have a list of what question types you can map on which fields in Notion? Would love to include it to the Docs!


Please disable the button Submit button in the modal if an error message is visible:

image

The new map input is not added below the entry where we hit +

https://tella.video/johanness-video-dz5k


Lastly: The integration doesnt pipe the answers to Notion :(

Not sure what causes it, didnt get an error in the console either.

https://tella.video/johanness-video-6yso


Thanks!!

It's really coming together, excited to have this in the upcoming weekly review 🤓

@PratikAwaik
Copy link
Contributor Author

  1. Checking
  2. The mapping UI disappearing is not reproducible from my end. Not sure why that would be happening.
  3. That's correct. People might want to add the same info twice.
  4. Notion docs says redirect URI, so I've kept URI.
  5. Mapping List (its in constants.ts as well)
TYPE_MAPPING = {
 cta: [],
 multipleChoiceMulti: ["multi_select"],
 multipleChoiceSingle: ["select", "status"],
 openText: [
   "created_by",
   "created_time",
   "date",
   "email",
   "last_edited_by",
   "last_edited_time",
   "number",
   "phone_number",
   "rich_text",
   "title",
   "url",
 ],
 nps: ["number"],
 consent: ["checkbox"],
 rating: ["number"],
};
  1. Disabled the button when there are errors present.
Screenshot 2023-10-20 at 5 22 56 PM

I've fixed it like this.

P.S: Typeform also does the same

  1. Sometimes, it might take some time for the answers to reflect in the database. Did you check notion after some time?

I've done fixes locally and working on other fixes. I'll update you once done.

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.

@PratikAwaik thank you for the PR :-) We also did some changes to the folder structure for the google sheets and airtable integration last week. Please check out the updated structure and make sure your integration fits right in :-)

Copy link
Member

Choose a reason for hiding this comment

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

please put all the images in a subfolder images

Copy link
Member

Choose a reason for hiding this comment

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

please put this component in a subfolder components

Copy link
Member

Choose a reason for hiding this comment

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

please put this component in a subfolder components

Copy link
Member

Choose a reason for hiding this comment

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

please put this component in a subfolder components

Copy link
Member

Choose a reason for hiding this comment

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

please put this component in a subfolder components

}
};

export const getNotionIntegration = cache(
Copy link
Member

Choose a reason for hiding this comment

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

please check the latest changes from main. We now use the integration service to get the integration by type and only have the service for the integration, e.g. here the notion service, to communicate with notion

Copy link
Member

Choose a reason for hiding this comment

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

Please check the latest changes in the main branch and how we reorganised the types for the different integrations

export const NOTION_OAUTH_CLIENT_ID = env.NOTION_OAUTH_CLIENT_ID;
export const NOTION_OAUTH_CLIENT_SECRET = env.NOTION_OAUTH_CLIENT_SECRET;
export const NOTION_AUTH_URL = env.NOTION_AUTH_URL;
export const NOTION_REDIRECT_URI = env.NOTION_REDIRECT_URI;
Copy link
Member

Choose a reason for hiding this comment

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

isn't the NOTION_REDIRECT_URI always the same and ${WEBAPP_URL}/api/...callback?
If so, please use this instead of a new environment variable

Copy link
Member

Choose a reason for hiding this comment

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

please check the latest changes in main and move the notion route to apps/web/app/api/v1/integrations/notion/callback/route.ts

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 you for making the changes 😊💪 Can you please solve the latest merge conflicts and check the few things that I pointed out?
@jobenjada will do a final check on the functionality and then we can merge this :-)

}),
});
} catch (error) {
if (error instanceof Prisma.PrismaClientKnownRequestError) {
Copy link
Member

Choose a reason for hiding this comment

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

@Dhruwang please check

}
return results;
} catch (error) {
if (error instanceof Prisma.PrismaClientKnownRequestError) {
Copy link
Member

Choose a reason for hiding this comment

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

@Dhruwang please check

@jobenjada
Copy link
Member

As discussed in Discord, for the record:

  1. Pls let Consent questions fill a checkbox field at Notion (true if clicked, false if skipped)
  2. PictureSelect: Can we pipe the image through to Notion?
  3. File Upload coming soon, but can be done once its live
  4. Hidden Fields: Add hidden fields to the select list and map them on openText in Notion

Thanks!! 🙏

@Dhruwang Dhruwang marked this pull request as draft November 15, 2023 09:37
@jobenjada jobenjada requested a review from mattinannt December 1, 2023 18:24
@jobenjada
Copy link
Member

@PratikAwaik works smoooooothly, great work!!

Matti pls have a look if the setup process is intuitive and well documented and last look at the migration and then LGTM por fiiiin :)

@jobenjada jobenjada marked this pull request as ready for review December 1, 2023 18:25
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.

@PratikAwaik Great job! 👏🚀
Sorry that it took us so long to merge this PR.

@mattinannt mattinannt enabled auto-merge December 14, 2023 13:08
@mattinannt mattinannt added this pull request to the merge queue Dec 14, 2023
Merged via the queue into formbricks:main with commit 8fd78bc Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

❗️ .env changes hacktoberfest complete these issues to gather points for Hacktoberfest hacktoberfest-accepted ❗️ migrations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE][FormTribe 🔥][1000 Points] Build direct Notion integration

4 participants