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

43662 - Added error handling hook to StopAreaDetailsEdit #931

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

villepynttari
Copy link
Contributor

@villepynttari villepynttari commented Oct 17, 2024

Testing:

  • Go to: //stop-registry/stop-areas/HSL:GroupOfStopPlaces:2?observationDate=2024-10-16
  • Change label and name values to existing values ( for example these - Go to: //stop-registry/stop-areas/HSL:GroupOfStopPlaces:1?observationDate=2024-10-16)
  • Should print errors with correct texts

This change is Reviewable

@villepynttari villepynttari changed the title Draft: 43662 - Added error handling hook to StopAreaDetailsEdit 43662 - Added error handling hook to StopAreaDetailsEdit Oct 17, 2024
Copy link
Contributor

@Huulivoide Huulivoide left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 6 unresolved discussions (waiting on @villepynttari)


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 7 at r1 (raw file):

import { StopAreaFormState } from '../stopAreaFormSchema';

const ERRORS: Readonly<Map<string, string>> = new Map([

Map luokan sijaan vois käyttää ihan vaan objektia. Ja en oo nyt vielä koko tietostoa lukenut, mutta oletan että tässä on Virhekoodi⋄käännösavain mäppäys → Käännös avain pitäs tyypittää oleen semmonen.
tyyppi TranslationKey ui/src/i18n.ts tiedostosta


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 24 at r1 (raw file):

  const tryHandle = useCallback(
    (error: ApolloError, details?: StopAreaFormState): boolean => {
      const errorNames = Array.from(ERRORS.keys());

Tässä on mun mielestä tarpeettomasti looppausta, kun tän sais tehtyy myös toisinkin päin. Et etitään virheen potentiaalistta errorCodea tunnettujen listasta, sen sijaan että katotaan onko virheessä yhtään tunnettia koodia asetettuna.


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 33 at r1 (raw file):

        const knownErrorKey = ERRORS.get(knownError);
        if (knownErrorKey) {
          if (details) {

Tää on tarpeeton. Koska toi on TFunktiolla optionaalinen argumentti, niin siellä on jo sisällä käsittely tilanteellee että toka argumentti on undefined.


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 45 at r1 (raw file):

    [t],
  );
  return { tryHandle };

Tarpeeton objekti wräppäys. Suoraan vaan funktio ulos.


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 46 at r1 (raw file):

  );
  return { tryHandle };
}

Ite sain muodon:

import { ApolloError } from '@apollo/client';
import get from 'lodash/get';
import { useCallback } from 'react';
import { useTranslation } from 'react-i18next';
import { TranslationKey } from '../../../../i18n';
import { showDangerToast } from '../../../../utils';
import { StopAreaFormState } from '../stopAreaFormSchema';

const ERRORS: Readonly<Record<string, TranslationKey>> = {
  GROUP_OF_STOP_PLACES_UNIQUE_NAME:
    'stopAreaDetails.errors.groupOfStopPlacesUniqueName',

  GROUP_OF_STOP_PLACES_UNIQUE_DESCRIPTION:
    'stopAreaDetails.errors.groupOfStopPlacesUniqueDescription',
};

function mapApolloErrorToTranslationKey(
  error: ApolloError,
): TranslationKey | null {
  const errorCode: unknown = get(error, ['cause', 'extensions', 'errorCode']);
  if (typeof errorCode === 'string' && errorCode in ERRORS) {
    return ERRORS[errorCode];
  }

  return null;
}

export function useStopAreaDetailsApolloErrorHandler() {
  const { t } = useTranslation();

  return useCallback(
    (error: ApolloError, details?: StopAreaFormState): boolean => {
      const translationKey = mapApolloErrorToTranslationKey(error);

      if (translationKey) {
        showDangerToast(t(translationKey, details));
        return true;
      }

      return false;
    },
    [t],
  );
}

ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.spec.ts line 1 at r1 (raw file):

import { ApolloError } from '@apollo/client';

Tälle pitäs olla olemassa oikee E2E testi, ja sen jälkeen tää yksikkö testi jää tarpeettomaks.
Koska Tiamatin pään muutoksetkin voi tän toiminnalisuuden rikkoa, ku sieltä ne virheet loppupeleissä tulee.

@villepynttari
Copy link
Contributor Author

ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 46 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Ite sain muodon:

import { ApolloError } from '@apollo/client';
import get from 'lodash/get';
import { useCallback } from 'react';
import { useTranslation } from 'react-i18next';
import { TranslationKey } from '../../../../i18n';
import { showDangerToast } from '../../../../utils';
import { StopAreaFormState } from '../stopAreaFormSchema';

const ERRORS: Readonly<Record<string, TranslationKey>> = {
  GROUP_OF_STOP_PLACES_UNIQUE_NAME:
    'stopAreaDetails.errors.groupOfStopPlacesUniqueName',

  GROUP_OF_STOP_PLACES_UNIQUE_DESCRIPTION:
    'stopAreaDetails.errors.groupOfStopPlacesUniqueDescription',
};

function mapApolloErrorToTranslationKey(
  error: ApolloError,
): TranslationKey | null {
  const errorCode: unknown = get(error, ['cause', 'extensions', 'errorCode']);
  if (typeof errorCode === 'string' && errorCode in ERRORS) {
    return ERRORS[errorCode];
  }

  return null;
}

export function useStopAreaDetailsApolloErrorHandler() {
  const { t } = useTranslation();

  return useCallback(
    (error: ApolloError, details?: StopAreaFormState): boolean => {
      const translationKey = mapApolloErrorToTranslationKey(error);

      if (translationKey) {
        showDangerToast(t(translationKey, details));
        return true;
      }

      return false;
    },
    [t],
  );
}

Tää varmaan looppaa enemmän pellin alla. Mapin muunto objektiksi ja listoiksi on erittäin tehokasta, mutta eipä tän kokoisessa datamäärässä ole käytännössä mitään väliä. Tämä tekee riippuvuuden lodashiin. Onhan tämä siistimmän näköinen, niin mennään tällä.

@villepynttari
Copy link
Contributor Author

ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.spec.ts line 1 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tälle pitäs olla olemassa oikee E2E testi, ja sen jälkeen tää yksikkö testi jää tarpeettomaks.
Koska Tiamatin pään muutoksetkin voi tän toiminnalisuuden rikkoa, ku sieltä ne virheet loppupeleissä tulee.

Yksikkötestit ei ole koskaan tarpeettomia monestakin syystä, vaikka olisikin integraatiotestejä. Lisään vielä sen e2e testin.

Copy link
Contributor Author

@villepynttari villepynttari left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @Huulivoide)


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 7 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Map luokan sijaan vois käyttää ihan vaan objektia. Ja en oo nyt vielä koko tietostoa lukenut, mutta oletan että tässä on Virhekoodi⋄käännösavain mäppäys → Käännös avain pitäs tyypittää oleen semmonen.
tyyppi TranslationKey ui/src/i18n.ts tiedostosta

Done.


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 24 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tässä on mun mielestä tarpeettomasti looppausta, kun tän sais tehtyy myös toisinkin päin. Et etitään virheen potentiaalistta errorCodea tunnettujen listasta, sen sijaan että katotaan onko virheessä yhtään tunnettia koodia asetettuna.

Done.


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 33 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tää on tarpeeton. Koska toi on TFunktiolla optionaalinen argumentti, niin siellä on jo sisällä käsittely tilanteellee että toka argumentti on undefined.

Done.


ui/src/components/forms/stop-area/util/stopAreaDetailsErrorHandler.ts line 45 at r1 (raw file):

Previously, Huulivoide (Jesse Jaara) wrote…

Tarpeeton objekti wräppäys. Suoraan vaan funktio ulos.

Done.

Copy link
Contributor

@Huulivoide Huulivoide left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @villepynttari)


cypress/e2e/stop-registry/stopAreaDetails.cy.ts line 365 at r2 (raw file):

    it('should handle unique name exception', () => {
      const existingLabel =
        stopAreaData?.at(1)?.stopArea?.name?.value ?? 'noop';

Sen sijaan, että luottaa puhtaasti tohon testidatan insertointi järjestykseen, niin oisko parempi etsiä haluttu testientiteetti mielumiin, jonko ID:n perusteella. name/description kentät.


cypress/e2e/stop-registry/stopAreaDetails.cy.ts line 375 at r2 (raw file):

      inputBasicDetails(newBasicDetails);
      stopAreaDetailsPage.details.edit.getSaveButton().click();
      toast.checkDangerToastHasMessage(

Jokin aika sitten tein success tilanteeseen vähän tarkemman chekin näille paahtoleiville.
toast, toast.expectSuccessToast("viesti").

checkDangerToastHasMessage implementaatio, ja sen muut vastineet on siitä mielestä huonoja, että ne jää pollaamaan DOM:ia ja odottaa timeouttia, kunne ne löytää sieltä "toastin, joka on tyyppiä ja sisältää viestin".

expectSuccessToast taas alkaa pollaan DOM:sta Ensimmäinen toast, joka tulee näkyviin .then() assertoi että se on tyyppiä success ja sitten vielä lopuksi, tarkasta että siinä on annettu viesti sisältönä.

Lyhyesti ongelma vanhassa:
Jos odotetaan onnistumista, mutta tuleekin virhe, tai jos odotetaan virhettä, ja tuleekin onnistuminen, niin: esiin tullut notifikaatio ehtii kadota ruudulta pois, kun testi koodi vielä pollaa kaikkiin hakuehtoihin sopivaa DOM elementtiä. Ja semmosta kun ei lopula löydy, niin Cypress ottaa ruutukaappauksen sivusta, jolla ei ole enää yhtään toastia näkyvissä → Kuvasta ei käy ilmi, miksi testiajo epäonnistui.

Eli: error versio expectSuccessToast:sta

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.

2 participants