Skip to content

fix: routing-forms protected with LicenseRequired#9521

Merged
hariombalhara merged 11 commits into
calcom:mainfrom
Meetcpatel:routing-forms-protected
Jul 24, 2023
Merged

fix: routing-forms protected with LicenseRequired#9521
hariombalhara merged 11 commits into
calcom:mainfrom
Meetcpatel:routing-forms-protected

Conversation

@Meetcpatel
Copy link
Copy Markdown
Contributor

What does this PR do?

This PR protect
/apps/routing-forms/forms
/apps/routing-forms/form-edit
/apps/routing-forms/route-builder
/apps/routing-forms/reporting
with LicenseRequired

Fixes #9480

Loom Video: https://www.loom.com/share/62643aa0db6049c892db0d92c0e6ef52?sid=2b8ac4c9-8bf3-4dd9-bfa0-70a370ed631b

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  • Test A
    • visit /apps/routing-forms/forms, /apps/routing-forms/form-edit, /apps/routing-forms/route-builder, /apps/routing-forms/reporting

Mandatory Tasks

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

Checklist

(all completed)

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 14, 2023

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

Name Status Preview Comments Updated (UTC)
ui ❌ Failed (Inspect) Jul 6, 2023 5:22pm

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 14, 2023

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

A member of the Team first needs to authorize it.

@github-actions github-actions Bot added ✅ good first issue Good for newcomers 🐛 bug Something isn't working labels Jun 14, 2023
@Meetcpatel Meetcpatel changed the title routing-forms protected with LicenseRequired fix: routing-forms protected with LicenseRequired Jun 14, 2023
@Meetcpatel Meetcpatel closed this Jun 14, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 14, 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! 🙌

@Meetcpatel Meetcpatel reopened this Jun 14, 2023
@CarinaWolli CarinaWolli requested a review from a team June 16, 2023 06:40
@PeerRich
Copy link
Copy Markdown
Member

hey @Meetcpatel can you look into the merge conflict? 🙏 @hariombalhara will then review and merge!

@PeerRich PeerRich added the Low priority Created by Linear-GitHub Sync label Jun 17, 2023
@Meetcpatel
Copy link
Copy Markdown
Contributor Author

hey @Meetcpatel can you look into the merge conflict? 🙏 @hariombalhara will then review and merge!

Done! Fixed conflict.

Comment thread yarn.lock
Comment thread packages/app-store/routing-forms/pages/reporting/[...appPages].tsx Outdated
if (!form) {
return null;
}
<LicenseRequired>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SingleForm is used by form-edit, reporting, route-builder. So, we can add LicenseRequired there itself and we won't need to duplicate this everywhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

reverted LicenseRequired from routing-forms/pages/forms

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure why you reverted /forms. I wanted you to revert form-edit, reporting, route-builder because all of them use a common wrapper component SingleForm which we can wrap in LicenseRequired. It would automatically add LicenseRequired to form-edit, reporting and route-builder

Screenshot 2023-07-06 at 10.17.25 AM.png

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see SingleForm used only with form-edit, reporting, and route-builder so wrapping SingleForm in LicenseRequired works for all three routes.

and /forms should be wrapped in LicenseRequired right?

@hariombalhara

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah exatly.

Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Thankyou @Meetcpatel. I left some comments.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 23, 2023

Thank you for following the naming conventions! 🙏

@Meetcpatel Meetcpatel marked this pull request as draft June 23, 2023 14:52
@Meetcpatel Meetcpatel marked this pull request as ready for review June 24, 2023 05:11
@PeerRich PeerRich dismissed hariombalhara’s stale review June 25, 2023 15:12

try again please

@PeerRich PeerRich requested review from a team and hariombalhara June 25, 2023 15:12
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Requesting changes again.

fixit

@github-actions
Copy link
Copy Markdown
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions Bot added the Stale label Jul 22, 2023
@hariombalhara hariombalhara self-requested a review July 24, 2023 14:13
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

LGTM now !! Thanks @Meetcpatel 🙏

@hariombalhara hariombalhara merged commit 41850c7 into calcom:main Jul 24, 2023
fritterhoff pushed a commit to hm-edu/cal.com that referenced this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working ✅ good first issue Good for newcomers Low priority Created by Linear-GitHub Sync Stale

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

[CAL-1938] /apps/routing-forms/forms not protected with LicenseRequired.

3 participants