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: unit tests for survey service #1813

Merged

Conversation

ShubhamPalriwala
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala commented Dec 21, 2023

What does this PR do?

Unit tests for all survey services

PS: Looks like my linter messed up :/ but I just ran pnpm run lint and nothing changed, weird

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
  • First PR at Formbricks? Please sign the CLA! Without it we wont be able to merge it 🙏

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

Copy link

vercel bot commented Dec 21, 2023

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

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
formbricks-cloud ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 5:30am
formbricks-com ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 5:30am

Copy link
Contributor

github-actions bot commented Dec 21, 2023

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

Copy link
Contributor

apps/formbricks-com/components/dummyUI/AddNoCodeEventModalDummy.tsx

Reordering CSS classes in a consistent manner can improve code readability and maintainability. It's a good practice to follow a specific order, for example, placing responsive classes (like sm:, md:, lg:, etc.) at the end of the class list.
Create Issue
See the diff
Checkout the fix

    className="hidden items-center space-x-2 rounded-lg bg-slate-50 p-3 dark:bg-slate-600 md:flex"
git fetch origin && git checkout -b ReviewBot/Impro-hps9r3o origin/ReviewBot/Impro-hps9r3o

apps/formbricks-com/components/home/Steps.tsx

There are several repeated blocks of code in the Steps component. These blocks can be extracted into a separate component to improve the readability of the code and make it more maintainable.
Create Issue
See the diff
Checkout the fix

    // New Step component
    const Step = ({ stepNumber, title, description, children }) => (
      <div className="mx-auto mb-12 mt-8 max-w-lg md:mb-0 md:mt-32  md:max-w-none">
        <div className="px-4 sm:max-w-4xl sm:px-6 lg:max-w-7xl lg:px-8">
          <div className="grid md:grid-cols-2 md:items-center md:gap-16">
            <div className="pb-8 sm:pl-10 md:pb-0">
              <h4 className="text-brand-dark font-bold">Step {stepNumber}</h4>
              <h2 className="xs:text-3xl text-2xl font-bold tracking-tight text-slate-800 dark:text-slate-200 sm:text-3xl">
                {title}
              </h2>
              <p className="text-md mt-6 max-w-lg leading-7 text-slate-500 dark:text-slate-400">
                {description}
              </p>
            </div>
            {children}
          </div>
        </div>
      </div>
    );

    // Usage in Steps component
    <Step stepNumber={1} title="Copy + Paste" description="Simply copy a &lt;script&gt; tag to your HTML head - that’s about it. Or use NPM to install Formbricks for React, Vue, Svelte, etc.">
      <div className="rounded-lg bg-slate-100 dark:bg-slate-800">
        <SetupTabs />
      </div>
    </Step>
git fetch origin && git checkout -b ReviewBot/Impro-2j47hex origin/ReviewBot/Impro-2j47hex

apps/formbricks-com/components/shared/PricingCalculator.tsx

There is a repeated block of code in the LinkSurveySlider, InAppSlider, and UserSegmentationSlider components. This block of code can be extracted into a separate component to improve the readability of the code and adhere to the DRY (Don't Repeat Yourself) principle.
Create Issue
See the diff
Checkout the fix

    const SliderComponent = ({ label, usersCount, price }) => (
      <div className="mb-2 flex items-center gap-x-2 md:gap-x-4">
        <div className="md:text-md w-3/6 text-left text-sm font-medium text-slate-700 dark:text-slate-200">
          {label}
        </div>
        <div className="md:text-md w-2/6 text-center text-sm font-medium text-slate-700 dark:text-slate-200">
          {Math.round(usersCount).toLocaleString()} Submissions
        </div>
        <div className="md:text-md flex w-1/6 items-center justify-end text-center text-sm font-medium text-slate-700 dark:text-slate-200 md:justify-center">
          <span>${price.toFixed(2)}</span>
        </div>
      </div>
    );
git fetch origin && git checkout -b ReviewBot/Impro-7lz3y4d origin/ReviewBot/Impro-7lz3y4d

The calculation of usersCountForInProductSlider and productSurveysPrice can be memoized using the useMemo hook to avoid unnecessary calculations on each render.
Create Issue
See the diff
Checkout the fix

    const usersCountForInProductSlider = useMemo(() => transformToLog(inProductSlider), [inProductSlider]);
    const productSurveysPrice = useMemo(() => calculatePrice(usersCountForInProductSlider), [usersCountForInProductSlider]);
git fetch origin && git checkout -b ReviewBot/Impro-6pq4qsd origin/ReviewBot/Impro-6pq4qsd

The code block that maps over the array [3, 4, 5, 6] and returns a string based on the value of the mark can be simplified by using a switch statement. This will make the code more readable and easier to maintain.
Create Issue
See the diff
Checkout the fix

    {[3, 4, 5, 6].map((mark) => {
      let label;
      switch(mark) {
        case 3:
          label = "1K";
          break;
        case 4:
          label = "10K";
          break;
        case 5:
          label = "100K";
          break;
        case 6:
          label = "1M";
          break;
        default:
          label = "";
      }
      return (
        <span key={mark} className="text-slate-600 dark:text-slate-300">
          {label}
        </span>
      );
    })}
git fetch origin && git checkout -b ReviewBot/Impro-etsmr2i origin/ReviewBot/Impro-etsmr2i

apps/formbricks-com/components/shared/Header.tsx

Consider using a throttling function to limit the number of times the scroll event listener is fired. This can significantly improve performance, especially on devices with slower processors.
Create Issue
See the diff
Checkout the fix

    useEffect(() => {
      const handleScroll = throttle(() => {
        if (window.scrollY > 250) {
          setStickyNav(true);
        } else {
          setStickyNav(false);
        }
      }, 200);
      window.addEventListener("scroll", handleScroll);
      return () => {
        window.removeEventListener("scroll", handleScroll);
      };
    }, []);
git fetch origin && git checkout -b ReviewBot/The-u-1j6yxr4 origin/ReviewBot/The-u-1j6yxr4

Consider using an object as an argument to the clsx function instead of a template string. This will make the code more readable and easier to maintain.
Create Issue
See the diff
Checkout the fix

    className={clsx({
      'text-slate-900 dark:text-slate-100': brick.status,
      'text-slate-400': !brick.status,
      'font-semibold': true
    })}
git fetch origin && git checkout -b ReviewBot/The-c-0i3hq7e origin/ReviewBot/The-c-0i3hq7e

@@ -55,7 +55,7 @@ export const AddNoCodeEventModalDummy: React.FC<EventDetailModalProps> = ({ open
Inner Text
</Label>
</div>
<div className="hidden items-center space-x-2 rounded-lg bg-slate-50 p-3 md:flex dark:bg-slate-600">
<div className="hidden items-center space-x-2 rounded-lg bg-slate-50 p-3 dark:bg-slate-600 md:flex">
Copy link
Contributor

Choose a reason for hiding this comment

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

Reordered the CSS classes to improve code readability and maintainability. The responsive class 'md:flex' is now placed at the end of the class list.

Suggested change
<div className="hidden items-center space-x-2 rounded-lg bg-slate-50 p-3 dark:bg-slate-600 md:flex">
className="hidden items-center space-x-2 rounded-lg bg-slate-50 p-3 dark:bg-slate-600 md:flex"

packages/lib/survey/service.ts Outdated Show resolved Hide resolved
@ShubhamPalriwala ShubhamPalriwala changed the title feat: unit tests for response service feat: unit tests for survey service Jan 9, 2024
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.

@ShubhamPalriwala thanks a lot for making the changes :-)

@mattinannt mattinannt added this pull request to the merge queue Jan 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 15, 2024
@ShubhamPalriwala ShubhamPalriwala added this pull request to the merge queue Jan 16, 2024
Merged via the queue into main with commit f4a31ad Jan 16, 2024
12 checks passed
@ShubhamPalriwala ShubhamPalriwala deleted the shubham/for-1565-write-unit-tests-for-survey-services branch January 16, 2024 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants