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

fix: use same regex for layoutsets in FE and BE #14327

Merged

Conversation

standeren
Copy link
Contributor

@standeren standeren commented Dec 23, 2024

Description

  • Adjust reg ex in backend to have same restrictions as in frontend.
  • Use same length restrictions in frontend as in backend.
  • Change error type passed from backend when name does not match regex. Also adapt text in frontend.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

@standeren standeren linked an issue Dec 23, 2024 that may be closed by this pull request
@github-actions github-actions bot added solution/studio/designer Issues related to the Altinn Studio Designer solution. quality/testing Tests that are missing, needs to be created or could be improved. backend frontend labels Dec 23, 2024
@standeren standeren added skip-releasenotes Issues that do not make sense to list in our release notes team/studio-domain1 skip-documentation Issues where updating documentation is not relevant labels Dec 23, 2024
@standeren standeren force-pushed the 13689-submitting-a-valid-layoutset-name-results-in-an-error branch from 4bcf361 to ed9d076 Compare December 23, 2024 08:10
Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.57%. Comparing base (3c55616) to head (d291ca7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14327   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files        1864     1864           
  Lines       24151    24151           
  Branches     2786     2786           
=======================================
  Hits        23082    23082           
  Misses        812      812           
  Partials      257      257           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Konrad-Simso Konrad-Simso self-assigned this Jan 2, 2025
Copy link
Contributor

@Konrad-Simso Konrad-Simso left a comment

Choose a reason for hiding this comment

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

Great work!

I have 1 suggestion, but i'm not sure it's a better alternative, the error message is already long, but it does not mention the limit on length.

frontend/language/src/nb.json Outdated Show resolved Hide resolved
@standeren standeren assigned Ildest and unassigned standeren Jan 3, 2025
Copy link
Contributor

@Ildest Ildest left a comment

Choose a reason for hiding this comment

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

Små justeringer

Copy link
Contributor

@ErlingHauan ErlingHauan left a comment

Choose a reason for hiding this comment

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

Test

Har endringen blitt gjort på feil tekstressurs?
Funksjonen getLayoutSetIdValidationErrorKey bruker tekstressursen validation_errors.file_name_invalid og ikke api_errors.AD_03 som har blitt endret i denne PR-en. Får også førstnevnte tekstressurs i Studio når jeg tester.


Jeg har også sett litt på ting som ikke angår sidegrupper direkte.

Layout-navn
Tekstressurs: "validation_errors.file_name_invalid": "Filnavnet er ugyldig. Du kan bruke tall, understrek, bindestrek, og store/små bokstaver fra det engelske alfabetet."

  • fra brukerens side er ikke dette et filnavn, så det er litt misvisende
  • funksjonen getPageNameErrorKey har ikke blitt oppdatert, så det kommer en gammel feilmelding ved 31 tegn

Tekstressurser som er veldig like
"api_errors.AD_03": "Navnet på sidegruppen er ugyldig. Det må ha mellom 2 og 28 tegn og kan inneholde tall, understrek, bindestrek, og store/små bokstaver fra det engelske alfabetet."

"validation_errors.file_name_invalid": "Filnavnet er ugyldig. Du kan bruke tall, understrek, bindestrek, og store/små bokstaver fra det engelske alfabetet.",

Kunne vi hatt en felles tekstressurs i stedet, som ser slik ut, siden de tilsynelatende bruker samme regex?
"validation_errors.name_invalid": "{{nameEntity}} er ugyldig. Det må ha mellom 2 og 28 tegn og kan inneholde tall, understrek, bindestrek, og store/små bokstaver fra det engelske alfabetet."

Da slipper man å holde dem i synk manuelt, hvis regex skulle forandre seg i fremtiden.


Mulig at bolken over bør være et eget issue 😊

@ErlingHauan ErlingHauan assigned standeren and unassigned ErlingHauan Jan 7, 2025
@standeren
Copy link
Contributor Author

Har endringen blitt gjort på feil tekstressurs?
Funksjonen getLayoutSetIdValidationErrorKey bruker tekstressursen validation_errors.file_name_invalid og ikke api_errors.AD_03 som har blitt endret i denne PR-en. Får også førstnevnte tekstressurs i Studio når jeg tester.

Får du teksten validation_errors.file_name_invalid i toast eller i grensesnittet? Teksten api_errors.AD_03 gjelder kun toast, nemlig. Og med endringene som er gjort så skal det jo helst ikke skje at man klarer å sende et ugyldig sidegruppenavn til backend, ettersom regexen de nå bruker er lik.

Hva angår de andre kommentarene dine, er jeg veldig enig. Vi burde unngå å bruke filnavn dersom det ikke er tydelig at brukeren opererer på en fil, feks når de laster opp filer. Ettersom det ikke har noe med regex for layoutset å gjøre, som denne saken angår, så er det sikkert best å opprette en egen sak/PR på det, ja! 😄 Bra sett!

Og godt mulig tekst-ressursene kan generaliseres litt med bruk av variabler, men samtidig tror jeg vi har gått for filosofien at det ikke skader å ha duplikate eller like tekster med ulike tekstnøkler 🫣

@standeren standeren removed their assignment Jan 7, 2025
standeren and others added 3 commits January 7, 2025 16:02
Bytt ut "Den" med "Det", siden det referer til "navnet".

Co-authored-by: Konrad-Simso <[email protected]>
@standeren standeren force-pushed the 13689-submitting-a-valid-layoutset-name-results-in-an-error branch from c62cb4c to 6394823 Compare January 7, 2025 15:03
@ErlingHauan
Copy link
Contributor

Ah, da har jeg misforstått issuet. Jeg skal teste en gang til og opprette issue på de andre tingene 👍

Og godt mulig tekst-ressursene kan generaliseres litt med bruk av variabler, men samtidig tror jeg vi har gått for filosofien at det ikke skader å ha duplikate eller like tekster med ulike tekstnøkler 🫣

Den er grei 😊

Copy link
Contributor

@ErlingHauan ErlingHauan left a comment

Choose a reason for hiding this comment

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

Test OK ☺️

@ErlingHauan ErlingHauan merged commit 465fc97 into main Jan 8, 2025
16 checks passed
@ErlingHauan ErlingHauan deleted the 13689-submitting-a-valid-layoutset-name-results-in-an-error branch January 8, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend frontend quality/testing Tests that are missing, needs to be created or could be improved. skip-documentation Issues where updating documentation is not relevant skip-releasenotes Issues that do not make sense to list in our release notes solution/studio/designer Issues related to the Altinn Studio Designer solution. team/studio-domain1
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Submitting a valid layoutset name results in an error
4 participants