Skip to content

Server side validation for property editors (integer, decimal and slider)#18428

Merged
nikolajlauridsen merged 9 commits intov15/devfrom
v15/feature/server-side-property-validation
Feb 25, 2025
Merged

Server side validation for property editors (integer, decimal and slider)#18428
nikolajlauridsen merged 9 commits intov15/devfrom
v15/feature/server-side-property-validation

Conversation

@AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Feb 24, 2025

I've implemented the server side property editor validation for the integer property editor. Looks to work as expected and I've added unit tests to verify, but wanted to check a couple of things:

  • The configuration object I've got here is a dictionary, so there's a little bit of "magic string" going on where I retrieve the min, max and step values. At very least I could move these into constants. But do you think we need more typing here? I.e. should I be converting some strongly typed configuration model and using that (and if so, is there an example of how you go from the dictionary to this typed representation?).
  • I'm repeating the conversion from the value as object? to int in both the min/max and step validator. The code is reused and it's a cheap operation, so I wasn't sure it was valuable to create a combined validator like I saw you were working on for the more complex JSON values where we need to deserialize. But maybe you think that would be valuable or more consistent.

Copy link
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a few minor nitpicky things.

Not too worried about doing the conversion multiple times, especially not when it's so nicely wrapped up.

In regards to the dictionary, some of the new property editors comes with typed configurations which is optimal, the dictionary is legacy, but I don't think it's necessary to convert these to strongly typed in the validators, however constants might be nice 😄 I Love the pattern in the base class though 🤩

@AndyButland AndyButland changed the title WIP: Server side validation for property editor validation Server side validation for property editors (integer, decimal and slider) Feb 24, 2025
@AndyButland
Copy link
Contributor Author

This is updated now to reflect the earlier comments and to now handle the server-side validation for the integer, decimal and slider property editors. So it's ready for another look over.

To test I've been using Swagger, using the PUT endpoint for updating a document and amending the provided value such that it triggers the expected validation, and reviewing the output. Plus there are unit tests for all the validations being made.

Oh, and if you could help me out with the Danish translations, that would be lovely. Thanks!

@AndyButland AndyButland marked this pull request as ready for review February 24, 2025 16:30
Copy link
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 I've added danish translations

# Conflicts:
#	src/Umbraco.Core/EmbeddedResources/Lang/da.xml
#	src/Umbraco.Core/EmbeddedResources/Lang/en.xml
@nikolajlauridsen nikolajlauridsen merged commit ebc38f4 into v15/dev Feb 25, 2025
3 of 4 checks passed
@nikolajlauridsen nikolajlauridsen deleted the v15/feature/server-side-property-validation branch February 25, 2025 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants