ronantakizawa/codereview-bench huggingface dataset:
Repository: freeCodeCamp/freeCodeCamp
File: e2e/email-sign-up.spec.ts (TypeScript)

Code before:
import { test, expect, type Page } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

let page: Page;

test.beforeAll(async ({ browser }) => {
  page = await browser.newPage();
  await page.goto('/email-sign-up');
});

test.afterAll(async () => {
  await page.close();
});

test.describe('Email sign-up page', () => {

  test('The page renders with correct title', async () => {
    await expect(page).toHaveTitle(
        `Email Sign Up | freeCodeCamp.org`
      );
  });

  test('The page renders the correct content', async () => {
      await expect(
        page.locator(
          "text=freeCodeCamp is a proven path to your first software developer job."
        )
      ).toBeVisible();
});

  test('The sign up button renders and functions', async() => {
    const button = page.getByTestId('email-sign-up-button')

    await expect(button).toBeVisible()

    await button.click()

    await expect(page).toHaveURL(/.*\/learn\/?$/);
  })


});


Review comment (suggestion):
```suggestion
  test('The page renders the correct content', async ({ page }) => {
```

Code after:
import { test, expect } from '@playwright/test';

import translations from '../client/i18n/locales/english/translations.json';

const apiLocation = process.env.API_LOCATION || 'http://localhost:3000';

test.describe('Email sign-up page when user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test.beforeEach(async ({ page }) => {
    // Intercept the endpoint to prevent `acceptedPrivacyTerms` from being set
    await page.route('*/**/update-privacy-terms', async route => {
      const json = [{ message: 'flash.privacy-updated', type: 'success' }];
      await route.fulfill({ json });
    });

    await page.goto('/email-sign-up');
  });

  test("should not enable Quincy's weekly newsletter when the user clicks the sign up button", async ({
    page,
    browserName
  }) => {
    test.skip(
      browserName === 'webkit',
      'user appears to not signed in on Webkit'
    );

    await expect(page).toHaveTitle('Email Sign Up | freeCodeCamp.org');
    await expect(
      page.getByText(
        'freeCodeCamp is a proven path to your first software developer job.'
      )
    ).toBeVisible();

    const signupLink = page.getByRole('link', {
      name: translations.buttons['sign-up-email-list']
    });

    await expect(signupLink).toBeVisible();
    await expect(signupLink).toHaveAttribute('href', `${apiLocation}/signin`);
    await signupLink.click();

    // The user is signed in and automatically redirected to /learn after clicking the button.
    // We wait for this navigation to complete before moving onto the next.
    await page.waitForURL('**/learn');
    await expect(
      page.getByRole('heading', { name: 'Welcome back, Full Stack User' })
Repository: freeCodeCamp/freeCodeCamp
File: e2e/email-sign-up.spec.ts (TypeScript)

Code before:
test.beforeAll(async ({ browser }) => {
  page = await browser.newPage();
  await page.goto('/email-sign-up');
});

test.afterAll(async () => {
  await page.close();
});

test.describe('Email sign-up page', () => {

  test('The page renders with correct title', async () => {
    await expect(page).toHaveTitle(
        `Email Sign Up | freeCodeCamp.org`
      );
  });

  test('The page renders the correct content', async () => {
      await expect(
        page.locator(
          "text=freeCodeCamp is a proven path to your first software developer job."
        )
      ).toBeVisible();
});

  test('The sign up button renders and functions', async() => {
    const button = page.getByTestId('email-sign-up-button')

    await expect(button).toBeVisible()

    await button.click()

    await expect(page).toHaveURL(/.*\/learn\/?$/);
  })


});


Review comment (suggestion):
```suggestion
  test('The sign up button renders and functions', async({ page }) => {
```

Code after:

test.describe('Email sign-up page when user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test.beforeEach(async ({ page }) => {
    // Intercept the endpoint to prevent `acceptedPrivacyTerms` from being set
    await page.route('*/**/update-privacy-terms', async route => {
      const json = [{ message: 'flash.privacy-updated', type: 'success' }];
      await route.fulfill({ json });
    });

    await page.goto('/email-sign-up');
  });

  test("should not enable Quincy's weekly newsletter when the user clicks the sign up button", async ({
    page,
    browserName
  }) => {
    test.skip(
      browserName === 'webkit',
      'user appears to not signed in on Webkit'
    );

    await expect(page).toHaveTitle('Email Sign Up | freeCodeCamp.org');
    await expect(
      page.getByText(
        'freeCodeCamp is a proven path to your first software developer job.'
      )
    ).toBeVisible();

    const signupLink = page.getByRole('link', {
      name: translations.buttons['sign-up-email-list']
    });

    await expect(signupLink).toBeVisible();
    await expect(signupLink).toHaveAttribute('href', `${apiLocation}/signin`);
    await signupLink.click();

    // The user is signed in and automatically redirected to /learn after clicking the button.
    // We wait for this navigation to complete before moving onto the next.
    await page.waitForURL('**/learn');
    await expect(
      page.getByRole('heading', { name: 'Welcome back, Full Stack User' })
    ).toBeVisible();

    await page.goto('/settings');
    await expect(
      page.getByRole('group', { name: translations.settings.email.weekly })
    ).toBeVisible();
    await expect(
      page.getByRole('button', { name: translations.buttons['yes-please'] })
Repository: freeCodeCamp/freeCodeCamp
File: e2e/email-sign-up.spec.ts (TypeScript)

Code before:
import translations from '../client/i18n/locales/english/translations.json';

let page: Page;

test.beforeAll(async ({ browser }) => {
  page = await browser.newPage();
  await page.goto('/email-sign-up');
});

test.afterAll(async () => {
  await page.close();
});

test.describe('Email sign-up page', () => {

  test('The page renders with correct title', async () => {
    await expect(page).toHaveTitle(
        `Email Sign Up | freeCodeCamp.org`
      );
  });

  test('The page renders the correct content', async () => {
      await expect(
        page.locator(
          "text=freeCodeCamp is a proven path to your first software developer job."
        )
      ).toBeVisible();
});

  test('The sign up button renders and functions', async() => {
    const button = page.getByTestId('email-sign-up-button')

    await expect(button).toBeVisible()

    await button.click()

    await expect(page).toHaveURL(/.*\/learn\/?$/);
  })


});


Review comment (suggestion):
```suggestion
        page.getByText(
          'freeCodeCamp is a proven path to your first software developer job.
        )
```

We aren't using a custom locator here, so we should use the `getByText` method instead.

Code after:

import translations from '../client/i18n/locales/english/translations.json';

const apiLocation = process.env.API_LOCATION || 'http://localhost:3000';

test.describe('Email sign-up page when user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test.beforeEach(async ({ page }) => {
    // Intercept the endpoint to prevent `acceptedPrivacyTerms` from being set
    await page.route('*/**/update-privacy-terms', async route => {
      const json = [{ message: 'flash.privacy-updated', type: 'success' }];
      await route.fulfill({ json });
    });

    await page.goto('/email-sign-up');
  });

  test("should not enable Quincy's weekly newsletter when the user clicks the sign up button", async ({
    page,
    browserName
  }) => {
    test.skip(
      browserName === 'webkit',
      'user appears to not signed in on Webkit'
    );

    await expect(page).toHaveTitle('Email Sign Up | freeCodeCamp.org');
    await expect(
      page.getByText(
        'freeCodeCamp is a proven path to your first software developer job.'
      )
    ).toBeVisible();

    const signupLink = page.getByRole('link', {
      name: translations.buttons['sign-up-email-list']
    });

    await expect(signupLink).toBeVisible();
    await expect(signupLink).toHaveAttribute('href', `${apiLocation}/signin`);
    await signupLink.click();

    // The user is signed in and automatically redirected to /learn after clicking the button.
    // We wait for this navigation to complete before moving onto the next.
    await page.waitForURL('**/learn');
    await expect(
      page.getByRole('heading', { name: 'Welcome back, Full Stack User' })
    ).toBeVisible();

    await page.goto('/settings');
    await expect(
Repository: freeCodeCamp/freeCodeCamp
File: client/src/pages/email-sign-up.tsx (TypeScript)

Code before:
      completedChallengeCount
    }: { acceptedPrivacyTerms: boolean; completedChallengeCount: number },
    isSignedIn: boolean,
    showLoading: boolean
  ) => ({
    acceptedPrivacyTerms,
    isSignedIn,
    showLoading,
    completedChallengeCount
  })
);
const mapDispatchToProps = (dispatch: Dispatch) =>
  bindActionCreators({ acceptTerms }, dispatch);
const RedirectToLearn = createRedirect('/learn');

function EmailListOptIn({
  isSignedIn,
  acceptTerms
}: {
  isSignedIn: boolean;
  acceptTerms: (accepted: boolean) => void;
}) {
  const { t } = useTranslation();
  if (isSignedIn) {
    return (
      <Row data-playwright-test-label="user-email-buttons">
        <Col md={4} mdOffset={2} sm={5} smOffset={1} xs={12}>
          <Button
            block={true}
            bsSize='lg'
            bsStyle='primary'
            className='big-cta-btn'
            onClick={() => acceptTerms(true)}
          >
            {t('buttons.yes-please')}
          </Button>
          <Spacer size='small' />
        </Col>
        <Col md={4} sm={5} xs={12}>
          <Button
            block={true}
            bsSize='lg'
            bsStyle='primary'
            className='big-cta-btn'
            onClick={() => acceptTerms(false)}
          >
            {t('buttons.no-thanks')}
          </Button>
          <Spacer size='small' />
        </Col>
      </Row>

Review comment (suggestion):
```suggestion
      <Row data-playwright-test-label='user-email-buttons'>
```

Code after:
      completedChallengeCount
    }: { acceptedPrivacyTerms: boolean; completedChallengeCount: number },
    isSignedIn: boolean,
    showLoading: boolean
  ) => ({
    acceptedPrivacyTerms,
    isSignedIn,
    showLoading,
    completedChallengeCount
  })
);
const mapDispatchToProps = (dispatch: Dispatch) =>
  bindActionCreators({ acceptTerms }, dispatch);
const RedirectToLearn = createRedirect('/learn');

function EmailListOptIn({
  isSignedIn,
  acceptTerms
}: {
  isSignedIn: boolean;
  acceptTerms: (accepted: boolean) => void;
}) {
  const { t } = useTranslation();
  if (isSignedIn) {
    return (
      <Container>
        <Row className='email-list-opt'>
          <Col md={4} mdOffset={2} sm={5} smOffset={1} xs={12}>
            <Button
              block={true}
              bsSize='lg'
              bsStyle='primary'
              className='big-cta-btn'
              onClick={() => acceptTerms(true)}
            >
              {t('buttons.yes-please')}
            </Button>
            <Spacer size='small' />
          </Col>
          <Col md={4} sm={5} xs={12}>
            <Button
              block={true}
              bsSize='lg'
              bsStyle='primary'
              className='big-cta-btn'
              onClick={() => acceptTerms(false)}
            >
              {t('buttons.no-thanks')}
            </Button>
            <Spacer size='small' />
          </Col>
Repository: freeCodeCamp/freeCodeCamp
File: client/src/pages/email-sign-up.tsx (TypeScript)

Code before:
        </Col>
        <Col md={4} sm={5} xs={12}>
          <Button
            block={true}
            bsSize='lg'
            bsStyle='primary'
            className='big-cta-btn'
            onClick={() => acceptTerms(false)}
          >
            {t('buttons.no-thanks')}
          </Button>
          <Spacer size='small' />
        </Col>
      </Row>
    );
  } else {
    return (
      <Col md={8} mdOffset={2} sm={10} smOffset={1} xs={12}>
        <Spacer size='small' />
        <Button
          block={true}
          bsSize='lg'
          bsStyle='primary'
          className='big-cta-btn'
          href={`${apiLocation}/signin`}
          data-playwright-test-label="email-sign-up-button"
        >
          {t('buttons.sign-up-email-list')}
        </Button>
        <Spacer size='small' />
      </Col>
    );
  }
}

function AcceptPrivacyTerms({
  acceptTerms,
  acceptedPrivacyTerms,
  isSignedIn,
  showLoading,
  completedChallengeCount = 0
}: AcceptPrivacyTermsProps) {
  const { t } = useTranslation();
  const acceptedPrivacyRef = useRef(acceptedPrivacyTerms);
  const acceptTermsRef = useRef(acceptTerms);

  useEffect(() => {
    acceptedPrivacyRef.current = acceptedPrivacyTerms;
    acceptTermsRef.current = acceptTerms;
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, []);

Review comment (suggestion):
```suggestion
          data-playwright-test-label='email-sign-up-button'
```

Code after:
            <Spacer size='small' />
          </Col>
          <Col md={4} sm={5} xs={12}>
            <Button
              block={true}
              bsSize='lg'
              bsStyle='primary'
              className='big-cta-btn'
              onClick={() => acceptTerms(false)}
            >
              {t('buttons.no-thanks')}
            </Button>
            <Spacer size='small' />
          </Col>
        </Row>
      </Container>
    );
  } else {
    return (
      <Col md={8} mdOffset={2} sm={10} smOffset={1} xs={12}>
        <Spacer size='small' />
        <Button
          block={true}
          bsSize='lg'
          bsStyle='primary'
          className='big-cta-btn'
          href={`${apiLocation}/signin`}
        >
          {t('buttons.sign-up-email-list')}
        </Button>
        <Spacer size='small' />
      </Col>
    );
  }
}

function AcceptPrivacyTerms({
  acceptTerms,
  acceptedPrivacyTerms,
  isSignedIn,
  showLoading,
  completedChallengeCount = 0
}: AcceptPrivacyTermsProps) {
  const { t } = useTranslation();
  const acceptedPrivacyRef = useRef(acceptedPrivacyTerms);
  const acceptTermsRef = useRef(acceptTerms);

  useEffect(() => {
    acceptedPrivacyRef.current = acceptedPrivacyTerms;
    acceptTermsRef.current = acceptTerms;
    // eslint-disable-next-line react-hooks/exhaustive-deps
Repository: freeCodeCamp/freeCodeCamp
File: client/src/pages/email-sign-up.tsx (TypeScript)

Code before:
      completedChallengeCount
    }: { acceptedPrivacyTerms: boolean; completedChallengeCount: number },
    isSignedIn: boolean,
    showLoading: boolean
  ) => ({
    acceptedPrivacyTerms,
    isSignedIn,
    showLoading,
    completedChallengeCount
  })
);
const mapDispatchToProps = (dispatch: Dispatch) =>
  bindActionCreators({ acceptTerms }, dispatch);
const RedirectToLearn = createRedirect('/learn');

function EmailListOptIn({
  isSignedIn,
  acceptTerms
}: {
  isSignedIn: boolean;
  acceptTerms: (accepted: boolean) => void;
}) {
  const { t } = useTranslation();
  if (isSignedIn) {
    return (
      <Row data-playwright-test-label='user-email-buttons'>
        <Col md={4} mdOffset={2} sm={5} smOffset={1} xs={12}>
          <Button
            block={true}
            bsSize='lg'
            bsStyle='primary'
            className='big-cta-btn'
            onClick={() => acceptTerms(true)}
          >
            {t('buttons.yes-please')}
          </Button>
          <Spacer size='small' />
        </Col>
        <Col md={4} sm={5} xs={12}>
          <Button
            block={true}
            bsSize='lg'
            bsStyle='primary'
            className='big-cta-btn'
            onClick={() => acceptTerms(false)}
          >
            {t('buttons.no-thanks')}
          </Button>
          <Spacer size='small' />
        </Col>
      </Row>

Review comment (suggestion):
We aren't using this label in the test so it shouldn't be added.

```suggestion
      <Row>
```

Code after:
      completedChallengeCount
    }: { acceptedPrivacyTerms: boolean; completedChallengeCount: number },
    isSignedIn: boolean,
    showLoading: boolean
  ) => ({
    acceptedPrivacyTerms,
    isSignedIn,
    showLoading,
    completedChallengeCount
  })
);
const mapDispatchToProps = (dispatch: Dispatch) =>
  bindActionCreators({ acceptTerms }, dispatch);
const RedirectToLearn = createRedirect('/learn');

function EmailListOptIn({
  isSignedIn,
  acceptTerms
}: {
  isSignedIn: boolean;
  acceptTerms: (accepted: boolean) => void;
}) {
  const { t } = useTranslation();
  if (isSignedIn) {
    return (
      <Container>
        <Row className='email-list-opt'>
          <Col md={4} mdOffset={2} sm={5} smOffset={1} xs={12}>
            <Button
              block={true}
              bsSize='lg'
              bsStyle='primary'
              className='big-cta-btn'
              onClick={() => acceptTerms(true)}
            >
              {t('buttons.yes-please')}
            </Button>
            <Spacer size='small' />
          </Col>
          <Col md={4} sm={5} xs={12}>
            <Button
              block={true}
              bsSize='lg'
              bsStyle='primary'
              className='big-cta-btn'
              onClick={() => acceptTerms(false)}
            >
              {t('buttons.no-thanks')}
            </Button>
            <Spacer size='small' />
          </Col>
Repository: freeCodeCamp/freeCodeCamp
File: e2e/email-sign-up.spec.ts (TypeScript)

Code before:
import { test, expect } from '@playwright/test';

test.beforeEach(async ({ page }) => {
  await page.goto('/email-sign-up');
});

test.describe('Email sign-up page', () => {
  test('should render correctly', async ({ page }) => {
    await expect(page).toHaveTitle(`Email Sign Up | freeCodeCamp.org`);
    await expect(
      page.getByText(
        'freeCodeCamp is a proven path to your first software developer job.'
      )
    ).toBeVisible();
    const button = page.getByTestId('email-sign-up-button');
    await expect(button).toBeVisible();
    await button.click();
    await expect(page).toHaveURL(/.*\/learn\/?$/);
  });
});


Review comment (suggestion):
Instead of querying the element by ID, we should use the `getByRole` so that we can ensure this is a link and not just a div that happens to have the test ID

```ts
import translations from '../client/i18n/locales/english/translations.json';

const signupLink = page.getByRole('link', { name: translations.buttons['sign-up-email-list'] });
await expect(signupLink).toBeVisible();
```

Code after:
import { test, expect } from '@playwright/test';

import translations from '../client/i18n/locales/english/translations.json';

const apiLocation = process.env.API_LOCATION || 'http://localhost:3000';

test.describe('Email sign-up page when user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test.beforeEach(async ({ page }) => {
    // Intercept the endpoint to prevent `acceptedPrivacyTerms` from being set
    await page.route('*/**/update-privacy-terms', async route => {
      const json = [{ message: 'flash.privacy-updated', type: 'success' }];
      await route.fulfill({ json });
    });

    await page.goto('/email-sign-up');
  });

  test("should not enable Quincy's weekly newsletter when the user clicks the sign up button", async ({
    page,
    browserName
  }) => {
    test.skip(
      browserName === 'webkit',
      'user appears to not signed in on Webkit'
    );

    await expect(page).toHaveTitle('Email Sign Up | freeCodeCamp.org');
    await expect(
      page.getByText(
        'freeCodeCamp is a proven path to your first software developer job.'
      )
    ).toBeVisible();

    const signupLink = page.getByRole('link', {
      name: translations.buttons['sign-up-email-list']
    });

    await expect(signupLink).toBeVisible();
Repository: freeCodeCamp/freeCodeCamp
File: e2e/link-ms-user.spec.ts (TypeScript)

Code before:
import { test, expect, type Page } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

let page: Page;

test.beforeEach(async ({ browser }) => {
  page = await browser.newPage();
  await page.goto(
    '/learn/foundational-c-sharp-with-microsoft/write-your-first-code-using-c-sharp/trophy-write-your-first-code-using-c-sharp'
  );
});

test.afterEach(async () => {
  await page.close();
});

test.describe('Link MS user component (unlinked signedOut user)', () => {
  test('Component has proper main heading and relevant sections', async () => {
    const mainHeading = page.getByRole('heading', {
      name: translations.learn.ms['link-header']
    });
    await expect(mainHeading).toBeVisible();

    const linkSignInText = page.getByTestId('link-signin-text');
    await expect(linkSignInText).toBeVisible();
  });
});

test.describe('Link MS user component (unlinked signedIn user)', () => {
  test.use({ storageState: 'playwright/.auth/certified-user.json' });

  test('Component has proper main heading and relevant sections', async () => {
    const mainHeading = page.getByRole('heading', {
      name: translations.learn.ms['link-header']
    });
    await expect(mainHeading).toBeVisible();

    const linkSignInText = page.getByTestId('unlinked-text');
    await expect(linkSignInText).toBeVisible();
  });

Review comment (suggestion):
```suggestion
test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/foundational-c-sharp-with-microsoft/write-your-first-code-using-c-sharp/trophy-write-your-first-code-using-c-sharp'
  );
});

test.afterEach(async ({ page }) => {
  await page.close();
});
```

Code after:
import { test, expect } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/foundational-c-sharp-with-microsoft/write-your-first-code-using-c-sharp/trophy-write-your-first-code-using-c-sharp'
  );
});

test.afterEach(async ({ page }) => {
  await page.close();
});

test.describe('Link MS user component (unlinked signedOut user)', () => {
  test('Component has proper main heading and relevant sections', async ({
    page
  }) => {
    const mainHeading = page.getByRole('heading', {
      name: translations.learn.ms['link-header']
    });
    await expect(mainHeading).toBeVisible();

    const linkSignInText = page.getByTestId('link-signin-text');
    await expect(linkSignInText).toBeVisible();
  });
});

test.describe('Link MS user component (unlinked signedIn user)', () => {
  test.use({ storageState: 'playwright/.auth/certified-user.json' });

  test('Component has proper main heading and relevant sections', async ({
    page
  }) => {
    const mainHeading = page.getByRole('heading', {
      name: translations.learn.ms['link-header']
    });
    await expect(mainHeading).toBeVisible();

    const linkSignInText = page.getByTestId('unlinked-text');
    await expect(linkSignInText).toBeVisible();