EES-5047: Implement character limits

[tomjonesdev]

This PR adds character limit validation and dynamic character counts to various title and summary/description fields. This is to help ensure data is presented in a concise format, and maintain standards and consistency across the service.

Counter to each child ticket's requirement, that each limit should be slightly higher than the highest existing value, it was found that all of the fields had existing production data which is greater than what was deemed acceptable (some by quite a significant margin).

Therefore, as these are limits which would only apply to create and update actions, rather than force any kind of truncation in the database, lower values closer to the overall averages were agreed, which would encourage analysts to refine the wording on these fields as and when needed.

[bennettstuart]

FE looks good to me, have left a nitpick that could be applied to a few instances

[bennettstuart]

Nit pick, but using magic number of 120 might lead to a small bug down the line if we want to update the maxLength. We have "120" appear in 3 places here, we might think about using a const titleMaxLength = 120 and using/interpolating it into the validation/messages

[benoutram]

I think Data set title '{0}' must be 120 characters or less would be more appropriate here. We've been trying to phase out usage of the term 'Subject' for a while now.

[tomjonesdev]

I've made the messages reference the names of the fields on the front end to avoid confusion. I don't think we should alter terminology until a time when it can be changed across the board.

[benoutram]

Ok, sure, I've just seen we have 'Subject title' in the frontend code as well. I hadn't realised that we still have the field labelled as 'Subject title' here in the UI and I'm a bit surprised we haven't suggested renaming it yet! 🙂

[benoutram]

I would remove all of extra .WithMessage configuration that's been added throughout this PR and just rely on the default FluentValidation error messages.

From an API point of view, FluentValidation already does a decent job of describing the error, both in a summary format and a detailed error format which includes additional details like the min length, max length, and the value provided.

The error response is also self describing, containing a path to the field in error state, in this case heading, which is more accurate from an API point of view than Table title. As far as I know these BE errors don't get presented to users so there's no need to provide friendlier names for each of the fields.

The maximum length in the out of the box generated message is also guaranteed to be accurate to the actual maximum length, which is not the case if you define a custom message where the two values could get out of sync.

[tomjonesdev]

Because we don't use MVC data annotations for alternate naming ([DisplayName] I think), Fluent's auto-generated error messages may reference property names which don't match the field name shown on the front end (mentioned on another comment that I opted for field matching to avoid confusion). Also, auto-generated messages don't always space/case values in the preferred way (e.g. SubjectTitle -> "Subject Title" instead of "Subject title").

I've refactored the value/message bits into a single constants class already based on Stu's comment regarding the FE part, which may help alleviate the sync issue you describe (I'll push it up shortly).

I appreciate these messages would only ever be seen if the frontend validation failed, but I'd still prefer to maintain consistent messaging in the presentation layer.

[benoutram]

Ok yeah I see your reasoning and I expect that the mismatch between names in request fields/error paths and the front end names for those fields is quite a widespread issue.

I tried it out to see what the auto generated message for MaximumLength(120) would look like without providing a custom message, and in the API response it will be something like

"message": "Must be 120 characters or fewer (was 123)."

Interestingly when I reduced the max length for the data block title right down to 5 (on the backend only) to deliberately cause a backend validation error this wasn't handled by the frontend. The 400 Bad Request with validation error caused the Sorry... page on the frontend.

I'm guessing you were expecting this to be handled, but this might be due to the mismatching between the names?

[tomjonesdev]

It's not handled properly in every bit of the frontend. In some cases you'll see the error in exactly the same way as you would if it was client-side validation, but in others you will only see the detailed error messages if you check the browser console (i.e. by expanding the Axios error details, or in the network tab). For example, in cases where a list of errors can be returned for a single field (such as the bulk zip upload), only the first error is shown to the client, but the rest can be viewed in the console - this is a limitation of the validation summary component.

FYI I tested the backend validation by disabling the frontend rule, rather than altering it. I wonder if there's an issue on the frontend whereby it fails to update the "valid" state upon receiving an "invalid" response? 🤷

[benoutram]

I chatted with Nick about this as he knows a bit more about how validation is coordinated across the frontend and backend, and we agreed there shouldn't be a need for defining custom error messages on the backend for things like max length validation on a field-by-field basis. We'll just end up defining the same message over and over again, as can be seen in your new ValidationConstants.cs file where you've essentially got the same max length message repeated 7 times. If we accept this change it would also raise the question of where else we've already got length validation on the backend which would now need these type of messages adding in to make those consistent. Presumably there would be many places we'd need to do this, not just those covered by this change.

I think the file uploads may work differently because in those cases you've got no frontend form object or backend request type) and there's a multipart form upload instead, and that's probably being handled differently where an error from the backend may end up being presented to the user.

In the other cases though the frontend should be able to handle the backend errors by using an error mapper to map which fields the server-side validation errors correspond to.

So something like this should work

const errorMappings = [
  mapFieldErrors<FormValues>({
    target: 'heading',
    messages: {
      MaximumLength: 'Title must be 120 characters or fewer',
    },
  }),
];

The errorMappings is then set as a property of FormProvider.

A backend validation error would be as follows:

    "errors": [
        {
            "message": "Must be 120 characters or fewer (was 123).",
            "path": "heading",
            "code": "MaximumLength",
            "detail": {
                "minLength": 0,
                "maxLength": 120,
                "totalLength": 123,
                "value": "My very long title exceeding 120 chars!"
            }
        }
    ]

Using the frontend errorMappings code above, the frontend should then remap the message for MaximumLength to its own message. This gives us the ability for the frontend to own all the messages which the user ends up seeing (Title must be 120 characters or fewer in my example), and we don't really care what the backend validation message is.

From the back end API's point of view the out-of-the-box FluentValidation message is sufficient and we don't get bogged down with the wording of messages. The front end messages can be written in a way that read better for users and are consistent with the GOV.UK style.

In future the frontend error mapping mechanism could be improved to automatically map common backend errors like Required or MaximumLength and extract certain details from the detail element like detail.minLength and detail.maxLength and substitute them into a message. This will mean we'll only have to define the max length message in one place on the frontend and get a consistent message everywhere.

[tomjonesdev]

Suggested changes failed due to FE prioritising server errors in serverValidation.ts (ln. 186) so I've removed the BE messaging.

[benoutram]

I think this could get pretty unwieldy really quickly if we continue with this pattern.

For example, TitleMaxLength and SummaryMaxLength - it's not obvious which title and summary fields these relate to.

There will become a namespacing issue as more and more features want to add their validation constants to this file where we have to come up with a strategy for grouping related constants together. Do we create sub classes, or create groups of related constants under headings in code comments, etc?

Having this class located in Common can also expose constants to more projects than is necessary.

I think it would be better to leave the constants inline with the FluentValidation validator config, where its then obvious what the character limits are, without having to reach out to this file, and we avoid this file growing to a point where it gets unmanageable.

[tomjonesdev]

Inline was my preference, but the ReleasesController methods use query parameters rather than a model. This meant that I couldn't replace the validation with FluentValidation to match the rest of the code, and the values used in the data annotations couldn't be anything other than constants.

I agree this could get unwieldy if extending without consideration, but the separate file allows these values to at least be isolated to a single place rather than dotted around individual classes.

[tomjonesdev]

Removed as per other comment.