cal.com icon indicating copy to clipboard operation
cal.com copied to clipboard

feat: prevent automatic query parameters when opening booking page

Open sanchitttt opened this issue 1 year ago • 2 comments

  • Resolve the user hindrance where the event's booking page should not have search parameters such as date and month in the initial page load

What does this PR do?

  • Fixes #18076 (GitHub issue number)
  • Fixes #18097 since it is needed

Loom link : https://www.loom.com/share/0f2d0cc02fd84868b72780ae3840dbc2

Mandatory Tasks (DO NOT REMOVE)

  • [ ] I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • [ ] I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • [ ] I confirm automated tests are in place that prove my fix is effective or that my feature works.

Checklist

  • I haven't checked if my changes generate no new warnings

sanchitttt avatar Dec 12 '24 08:12 sanchitttt

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

A member of the Team first needs to authorize it.

vercel[bot] avatar Dec 12 '24 08:12 vercel[bot]

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (12/12/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (12/12/24)

1 label was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (02/13/25)

1 label was added to this PR based on Keith Williams's automation.

graphite-app[bot] avatar Dec 12 '24 08:12 graphite-app[bot]

We also need test case for this feature, @sanchitttt can you please add 🙏

Hi @Praashh

Could you help me understand what specific test cases I should write? Since my changes are primarily in the store.ts file (specifically for setSelectedDate) and the Days component (particularly for useHandleInitialDateSelection), I assume I should focus on testing these areas only. Is that okay?

For store.ts , I've done something like:

describe('useBookerStore - setSelectedDate',() => {
  beforeEach(() => {
    // Set the initial state or reset URL before each test
    window.history.pushState({},'','/');
    useBookerStore.setState(initialStoreState,true); // The second argument `true` tells Zustand to merge and not overwrite the state completely
  });

  afterEach(() => {
    cleanup();
  });

  it('given empty search params when setSelectedDate is invoked with omitUpdatingParams as true then url search params should remain unchanged (Custom Behavior)',() => {
    const TestComponent = () => {
      const { setSelectedDate,selectedDate } = useBookerStore((state) => ({
        setSelectedDate: state.setSelectedDate,
        selectedDate: state.selectedDate, // Access the selectedDate from the store
      }));

      return (
        <button onClick={() => setSelectedDate('2024-12-14',true)}>
          Set Date
        </button>
      );
    };

    render(<TestComponent />);

    // Simulate clicking the button
    const button = screen.getByText('Set Date');
    act(() => {
      button.click();
    });

    const urlSearchParams = new URLSearchParams(window.location.search);
    const dateParam = urlSearchParams.get("date");
    const monthParam = urlSearchParams.get("month");

    // Assert that the date and month query params remain null
    expect(dateParam).toBeNull();
    expect(monthParam).toBeNull();
  });

  it('given empty search params when setSelectedDate is invoked with omitUpdatingParams as false then url search params should remain change (Default Behavior)',() => {
    const TestComponent = () => {
      const { setSelectedDate,selectedDate } = useBookerStore((state) => ({
        setSelectedDate: state.setSelectedDate,
        selectedDate: state.selectedDate, // Access the selectedDate from the store
      }));


      return (
        <button onClick={() => setSelectedDate('2024-12-15',false)}>
          Set Date
        </button>
      );
    };

    render(<TestComponent />);

    // Simulate clicking the button
    const button = screen.getByText('Set Date');
    act(() => {
      button.click();
    });

    const urlSearchParams = new URLSearchParams(window.location.search);
    const dateParam = urlSearchParams.get("date");
    const monthParam = urlSearchParams.get("month");

    // Assert that the date and month query params are updated correctly
    expect(dateParam).equal("2024-12-15");
    expect(monthParam).equal("2024-12");
  });
});

Am I heading in the right direction ?

For the useHandleInitialDateSelection(), I was thinking of extracting it into a separate file as a hook so it can be tested easily. Will that be okay ?

sanchitttt avatar Dec 14 '24 07:12 sanchitttt

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Dec 29 '24 00:12 github-actions[bot]

We also need test case for this feature, @sanchitttt can you please add 🙏

Hi @Praashh

Could you help me understand what specific test cases I should write? Since my changes are primarily in the store.ts file (specifically for setSelectedDate) and the Days component (particularly for useHandleInitialDateSelection), I assume I should focus on testing these areas only. Is that okay?

For store.ts , I've done something like:

describe('useBookerStore - setSelectedDate',() => {
  beforeEach(() => {
    // Set the initial state or reset URL before each test
    window.history.pushState({},'','/');
    useBookerStore.setState(initialStoreState,true); // The second argument `true` tells Zustand to merge and not overwrite the state completely
  });

  afterEach(() => {
    cleanup();
  });

  it('given empty search params when setSelectedDate is invoked with omitUpdatingParams as true then url search params should remain unchanged (Custom Behavior)',() => {
    const TestComponent = () => {
      const { setSelectedDate,selectedDate } = useBookerStore((state) => ({
        setSelectedDate: state.setSelectedDate,
        selectedDate: state.selectedDate, // Access the selectedDate from the store
      }));

      return (
        <button onClick={() => setSelectedDate('2024-12-14',true)}>
          Set Date
        </button>
      );
    };

    render(<TestComponent />);

    // Simulate clicking the button
    const button = screen.getByText('Set Date');
    act(() => {
      button.click();
    });

    const urlSearchParams = new URLSearchParams(window.location.search);
    const dateParam = urlSearchParams.get("date");
    const monthParam = urlSearchParams.get("month");

    // Assert that the date and month query params remain null
    expect(dateParam).toBeNull();
    expect(monthParam).toBeNull();
  });

  it('given empty search params when setSelectedDate is invoked with omitUpdatingParams as false then url search params should remain change (Default Behavior)',() => {
    const TestComponent = () => {
      const { setSelectedDate,selectedDate } = useBookerStore((state) => ({
        setSelectedDate: state.setSelectedDate,
        selectedDate: state.selectedDate, // Access the selectedDate from the store
      }));


      return (
        <button onClick={() => setSelectedDate('2024-12-15',false)}>
          Set Date
        </button>
      );
    };

    render(<TestComponent />);

    // Simulate clicking the button
    const button = screen.getByText('Set Date');
    act(() => {
      button.click();
    });

    const urlSearchParams = new URLSearchParams(window.location.search);
    const dateParam = urlSearchParams.get("date");
    const monthParam = urlSearchParams.get("month");

    // Assert that the date and month query params are updated correctly
    expect(dateParam).equal("2024-12-15");
    expect(monthParam).equal("2024-12");
  });
});

Am I heading in the right direction ?

For the useHandleInitialDateSelection(), I was thinking of extracting it into a separate file as a hook so it can be tested easily. Will that be okay ?

yeah something like that, it should check the query params

Praashh avatar Dec 29 '24 04:12 Praashh

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Jan 26 '25 00:01 github-actions[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 13 '25 05:02 CLAassistant

Tests are not required for this (confirmed in the team as well)

TusharBhatt1 avatar Feb 13 '25 05:02 TusharBhatt1

E2E results are ready!

github-actions[bot] avatar Feb 13 '25 12:02 github-actions[bot]