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

Add GenericFormDataValidator2 that doesn't take DataType in constructor #547

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ivarne
Copy link
Member

@ivarne ivarne commented Apr 3, 2024

GenericFormDataValidator<TModel> is a (currently undocumented) convenience abstract class for implementing IFormDataValidator. The benefit is that HasRelevantChanges and ValidateFormData has a signature of TModel instead of object and the helper method CreateValidationIssue that takes an expression (aka linq) instead of providing the field the issue relates to as a string. In addition ValidateFormData does not return a list of issues, but instead you register them with AddValidationIssue and CreateValidationIssue.

The previous version of this abstract class defined the DataType property and to ensure that users actually set this property, it was a required parameter in the constructor. Today I realised that it was hard to read and understand how that magic string ended up in the base() call, and that an abstract class can declare an abstract property so that inheriting classes is required to implement the property. I think this results in much prettier code.

// Define constructor in order to set `DataType` to a hard coded value
public TestValidator() : base("MyType"){}

The new way is almost the same as when you implement the IFormDataValidator interface directly (needs the override keyword because of the abstract class)

// Just override the property (as required by the compiler) and set the value directly.
public override string DataType => "MyType";

Breaking change, alternatives?

This is a breaking change which will cause those apps that has started to use GenericFormDataValidator<TModel>. As it is not documented, I think that would be very few cases.

Add obsolete message, to better explain the change

We can improve the error message by adding the following constructors.

    /// <summary>
    /// Default constructor
    /// </summary>
    public GenericFormDataValidator() { }

    [Obsolete("Override DataType instead of setting in constructor `public override string DataType => \"MyType\";`", error: true)]
    public GenericFormDataValidator(string dataType) { }

Create a new abstract class

As this is just a utility class, we could potentially just add a new one with a new name and the slightly changed semantics. This will ensure that no direct changes will be needed until the old (obsolete) version is removed in v9. Good suggestions for names would be very welcome. See suggestion here

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)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@ivarne ivarne added the breaking-change Label Pull requests with breaking changes. Used when generating releasenotes label Apr 3, 2024
@ivarne ivarne self-assigned this Apr 3, 2024
@ivarne ivarne added this to the 9.0.0 milestone Apr 24, 2024
Copy link

sonarcloud bot commented Apr 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

…taType.

Keep the old GenericFormDataValidator for backwards compatibility.
@ivarne ivarne force-pushed the ivarne/GenericFormDataValidator-fix branch from 7e0e6c8 to 6477b1f Compare June 1, 2024 12:19
@ivarne ivarne removed the breaking-change Label Pull requests with breaking changes. Used when generating releasenotes label Jun 1, 2024
@ivarne ivarne removed this from the 9.0.0 milestone Jun 1, 2024
@ivarne ivarne changed the title Don't use Base constructor to set DataType in GenericFormDataValidator Add GenericFormDataValidator2 that doesn't take DataType in constructor Jun 1, 2024
Copy link

sonarcloud bot commented Jun 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
51.7% Coverage on New Code (required ≥ 65%)
46.8% Duplication on New Code (required ≤ 3%)
30.0% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarCloud

@ivarne ivarne added this to the 9.0.0 milestone Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

1 participant