Add validation to Multiple field settings

Created on 7 June 2023, over 1 year ago
Updated 21 June 2023, over 1 year ago

Problem/Motivation

Follow-up from πŸ› Entering a non-numeric value for a start row value in 'Multiple field settings' for a views field leads to a fatal error Fixed /

Steps to reproduce
* Install Umami
* Create a View showing content in fields
* Add the 'Tag' field
* In 'Multiple field settings' set "Display N value(s)" to 'All'
* Apply the field settings
* Watch the spinner spin forever

Following the previous issue, there's no fatal error now, but we'll be casting 'All' to 0. Instead, we should validate the field as numeric.

We should still leave the cast in because there could continue to be invalid views around, these will eventually get fixed once they're resaved from the UI and the validation error kicks in, but seems like it would be very challenging to write an upgrade path for.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
ViewsΒ  β†’

Last updated about 3 hours ago

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @catch
  • First commit to issue fork.
  • last update over 1 year ago
    29,505 pass
  • @lendude opened merge request.
  • Status changed to Needs review over 1 year ago
  • πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

    This turns the two Multiple values settings, that we assume to be numbers, into numbers fields.

    Tested this:

    these will eventually get fixed once they're resaved from the UI and the validation error kicks in

    Turns out this isn't what happens if you just make these number fields. If you start with a string value and then apply this fix and reload the View it will show an empty value. So no validation error is given if we solve it like this.
    We could validate this with actual form validation rules and in that case it would throw the error. But that feels a little overkill to me, also since we already hardend the ode against people sneaking in strings

    Opinions welcome!

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    I agree that it makes sense change to a number form.

    Maybe a follow up could be open to add an "all" option

    Form could maybe be

    All - new
    Custom - current setup of using Display N value(s)

    Question though will an update hook be needed to transform current values from string to int or is the form API smart enough to know that.

Production build 0.71.5 2024