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: Add custom attribute for OptionValue and OptionLabel and allow empty string #14035

Conversation

standeren
Copy link
Contributor

@standeren standeren commented Nov 11, 2024

Description

Add new custom [NotNullable] attribute for Value and Label fields in Option class in order to avoid [Required] attribute that denies empty strings including null. According to the json schemas from Apps that defines Options, empty strings are allowed, so our backend should also allow this.

Additional improvements:

  • Add OptionsFormatExceptionsFilter in order to be able to pass informative exceptions when value is assigned an invalid type
  • Adapt Value and Label fields in Option class to use required property that is evaluated during deserialization, which allowed us to remove some custom null-validation code
  • Add tests for upload options endpoint
  • Add validation when fetching options from repo
  • Add test checking for 400 when fetching invalid options

Related Issue(s)

  • #{issue number}

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)

@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 labels Nov 11, 2024
@standeren standeren changed the title Add custom attribute for OptionValue and OptionLabel and allow empty … fix: Add custom attribute for OptionValue and OptionLabel and allow empty string Nov 11, 2024
@standeren standeren force-pushed the fix/allow-empty-strings-in-value-and-label-in-options-controller branch 4 times, most recently from 93399f2 to fa36271 Compare November 12, 2024 13:00
@standeren standeren force-pushed the fix/allow-empty-strings-in-value-and-label-in-options-controller branch from fa36271 to 4c50a5c Compare November 12, 2024 13:09
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.32%. Comparing base (7840b40) to head (907f422).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14035   +/-   ##
=======================================
  Coverage   95.32%   95.32%           
=======================================
  Files        1775     1775           
  Lines       23128    23128           
  Branches     2685     2685           
=======================================
  Hits        22047    22047           
  Misses        833      833           
  Partials      248      248           

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


🚨 Try these New Features:

@standeren standeren force-pushed the fix/allow-empty-strings-in-value-and-label-in-options-controller branch from eb296b5 to ab76bf5 Compare November 12, 2024 14:20
@standeren standeren added team/studio-domain1 skip-releasenotes Issues that do not make sense to list in our release notes skip-documentation Issues where updating documentation is not relevant kind/bug Used when there is a defect / something is not working as it should. labels Nov 12, 2024
@standeren standeren force-pushed the fix/allow-empty-strings-in-value-and-label-in-options-controller branch 2 times, most recently from ea1429b to 4400b7c Compare November 13, 2024 13:12
@standeren standeren marked this pull request as ready for review November 13, 2024 13:12
@standeren standeren force-pushed the fix/allow-empty-strings-in-value-and-label-in-options-controller branch from 4400b7c to 971097d Compare November 13, 2024 13:28
@ErlingHauan ErlingHauan self-assigned this Nov 14, 2024
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.

Nice work! Great that you also spotted the missing tests and took action. It's a reminder for me to get familiar with code coverage in my editor. But it would be nice if we had code coverage in the pipeline for backend as well 😬

@ErlingHauan ErlingHauan assigned standeren and unassigned ErlingHauan Nov 14, 2024
@standeren standeren removed their assignment Nov 14, 2024
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.

Nice work 🚀

@Konrad-Simso Konrad-Simso removed their assignment Nov 21, 2024
@Konrad-Simso Konrad-Simso merged commit eb8f613 into main Nov 21, 2024
23 checks passed
@Konrad-Simso Konrad-Simso deleted the fix/allow-empty-strings-in-value-and-label-in-options-controller branch November 21, 2024 07:56
nkylstad pushed a commit that referenced this pull request Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend frontend kind/bug Used when there is a defect / something is not working as it should. 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.

3 participants