fix: Route Builder rules should be case insensitive #9040

jemiluv8 · 2023-05-22T15:29:18Z

What does this PR do?

Override jsonLogic operators on string operands to allow for case insensitive comparisons

Fixes #5439

Case insensitive "equals" comparison
Case insensitive "not equals" comparison

Case insensitive "contains" comparison
Case insensitive "not contains" comparison

Type of change

Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

Install the "Routing Forms" app
Create a new form
Create form fields under the "Form" tab
Go to the routing tab and create a rule for any of the string operators ("contains", "not contains", "equals", "not equals")
Use the test preview on the left sidebar to fill out an example form
Use the "Test Routing" button to confirm the case insensitive comparison of the form values for relevant rules

Edit by @hariombalhara

We haven't implemented the same for Reporting because Prisma as of this moment doesn't support case insensitive JSON querying.

Checklist

I haven't added tests that prove my fix is effective or that my feature works

/claim #5439

…ensitive comparisons. Affected Operators: "==", "===", "!=", "!==", "in"

…s-should-be-case-insensitive

vercel · 2023-05-22T15:29:23Z

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

Name	Status	Preview	Comments	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 23, 2023 10:22pm

vercel · 2023-05-22T15:29:24Z

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

A member of the Team first needs to authorize it.

github-actions · 2023-05-22T15:39:40Z

📦 Next.js Bundle Analysis for @calcom/web

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

One Page Changed Size

The following page changed size from the code in this PR compared to its base branch:

Page	Size (compressed)	First Load	% of Budget (`350 KB`)
`/auth/login`	`149.41 KB`	300.35 KB	85.81% (🟢 -0.18%)

Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored.

…code there will be from jsonLogic and may not meet our coding style. Majority of overrides require us to copy over functions and their signatures from jsonLogic and then modify their implementation. The signature of functions implementing most operators take the operands typed as "any", which is intended, but doesn't adhere to our coding style. Hence the need to override the eslint rule

…nsensitive

packages/app-store/routing-forms/lib/jsonLogicOverrides.ts

hariombalhara

Looks good so far except 1 bug where MultiSelect equals stopped working.

That was because 'in' operator got second argument as an array of string which wasn't handled in normalize. So, one operand got lowercased but not the other one causing in operator to always fail. I have fixed that case.

@jemiluv8 I would suggest you to do one more round of testing with all the operators and field types combination.

Also, there is an open comment.

Note: this deviates from the original implementation in the jsonLogic library because our current useage ensures that the second operand is always a string or string[] and will therefore always have .index function. Whenever our invariants change in the future, make sure to modify this implementation to prevent any unexpected

jemiluv8 · 2023-05-23T22:17:02Z

@hariombalhara, I just completed another round of testing and all looks fine.
I admit I didn't pay much attention to select/multiselect field types and their
respective operations

About the redundant check in our override for the "in" operator
I decided to remove the redundant check for the typeof second.indexOf === "undefined".

Justification
Like I mentioned in the comment above I tried to copy over the exact implementation of
various jsonLogic operators from the repo. For the "in" operator, the library assumed the
second operand could be of any type and was therefore guarding against situations where
it may not be a string or an array.

Our current useage of the jsonLogic library ensures that the "in" operator will always get either a
string or string[]. That makes the extra check redundant.

hariombalhara · 2023-05-26T12:15:48Z

playwright.config.ts

-const DEFAULT_NAVIGATION_TIMEOUT = process.env.CI ? 15000 : 50000;
-const DEFAULT_EXPECT_TIMEOUT = process.env.CI ? 15000 : 50000;
+const DEFAULT_NAVIGATION_TIMEOUT = process.env.CI ? 15000 : 120000;
+const DEFAULT_EXPECT_TIMEOUT = process.env.CI ? 15000 : 120000;


Added this change here because sometimes dev server can be too slow and it times out unncessarily.

jemiluv8 added 2 commits May 22, 2023 14:27

override jsonLogic operators on string operands to allow for case ins…

c56ac32

…ensitive comparisons. Affected Operators: "==", "===", "!=", "!==", "in"

Merge remote-tracking branch 'origin' into CAL-336-Route-builder-rule…

3b66a06

…s-should-be-case-insensitive

jemiluv8 requested a review from hariombalhara as a code owner May 22, 2023 15:29

jemiluv8 requested a review from a team May 22, 2023 15:29

algora-pbc bot mentioned this pull request May 22, 2023

[CAL-336] Route Builder rules should be case insensitive #5439

Closed

vercel bot deployed to Preview – ui May 22, 2023 15:33 View deployment

vercel bot deployed to Preview – ui May 22, 2023 23:15 View deployment

run linter to fix issues

41d0732

vercel bot deployed to Preview – ui May 23, 2023 00:38 View deployment

Merge branch 'main' into CAL-336-Route-builder-rules-should-be-case-i…

2d527a8

…nsensitive

vercel bot deployed to Preview – ui May 23, 2023 09:50 View deployment

hariombalhara reviewed May 23, 2023

View reviewed changes

packages/app-store/routing-forms/lib/jsonLogicOverrides.ts Outdated Show resolved Hide resolved

Fix bug in in operator when second arg is an array

49f23b6

hariombalhara reviewed May 23, 2023

View reviewed changes

packages/app-store/routing-forms/lib/jsonLogicOverrides.ts Show resolved Hide resolved

vercel bot deployed to Preview – ui May 23, 2023 11:09 View deployment

hariombalhara requested changes May 23, 2023

View reviewed changes

vercel bot deployed to Preview – ui May 23, 2023 22:22 View deployment

PeerRich changed the title ~~[CAL-336] Route Builder rules should be case insensitive~~ fix: Route Builder rules should be case insensitive May 24, 2023

hariombalhara reviewed May 26, 2023

View reviewed changes

hariombalhara approved these changes May 26, 2023

View reviewed changes

hariombalhara added this pull request to the merge queue May 26, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 26, 2023

PeerRich merged commit 15e50fc into calcom:main May 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Route Builder rules should be case insensitive #9040

fix: Route Builder rules should be case insensitive #9040

jemiluv8 commented May 22, 2023 •

edited by hariombalhara

Loading

vercel bot commented May 22, 2023 •

edited

Loading

vercel bot commented May 22, 2023

github-actions bot commented May 22, 2023 •

edited

Loading

hariombalhara left a comment

jemiluv8 commented May 23, 2023 •

edited

Loading

hariombalhara May 26, 2023

fix: Route Builder rules should be case insensitive #9040

fix: Route Builder rules should be case insensitive #9040

Conversation

jemiluv8 commented May 22, 2023 • edited by hariombalhara Loading

What does this PR do?

Type of change

How should this be tested?

Checklist

vercel bot commented May 22, 2023 • edited Loading

vercel bot commented May 22, 2023

github-actions bot commented May 22, 2023 • edited Loading

📦 Next.js Bundle Analysis for @calcom/web

One Page Changed Size

hariombalhara left a comment

Choose a reason for hiding this comment

jemiluv8 commented May 23, 2023 • edited Loading

hariombalhara May 26, 2023

Choose a reason for hiding this comment

jemiluv8 commented May 22, 2023 •

edited by hariombalhara

Loading

vercel bot commented May 22, 2023 •

edited

Loading

github-actions bot commented May 22, 2023 •

edited

Loading

jemiluv8 commented May 23, 2023 •

edited

Loading