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

Redundant ValidationOption object properties for short text, long text and number fields #408

Closed
liangyuanruo opened this issue Oct 4, 2020 · 2 comments · Fixed by #2040
Assignees
Labels
P2 planned for next 1-2 months tech debt

Comments

@liangyuanruo
Copy link
Contributor

liangyuanruo commented Oct 4, 2020

The ValidationOption object defines the required validation rules for the short text and long text fields. An example for Short Text is shown below:

enum ShortTextSelectedValidation {
  Max = 'Maximum',
  Min = 'Minimum',
  Exact = 'Exact',
  Range = 'Range',
}

{
  customMax: number | null
  customMin: number | null
  customVal: number | null
  selectedValidation: ShortTextSelectedValidation | null
}

The type model is potentially inconsistent for the following reasons:

  • Short text and long text fields do not offer range validation, so the type system is not reflective of application functionality today.
  • The key selectedValidation defines the validation rule being applied, while the customVal property defines the number of characters to validate against. This makes the customMin and customMax keys redundant and runs the risk of inconsistent application state.

TODOs:

  1. Verify that the database is internally consistent today for all short text and long text fields:
    a. customVal === customMin when selectedValidation === 'Minimum'
    b. customVal === customMax when selectedValidation === 'Maximum'
    c. customVal === customMin === customMax when selectedValidation === 'Exact'
    d. selectedValidation === 'Range' should not exist
  2. Simplify the codebase by removing references to customMin, customMax and Range
  3. Write a database migration script and/or mongoose hooks to fix inconsistencies discovered
@liangyuanruo liangyuanruo added P2 planned for next 1-2 months tech debt labels Oct 4, 2020
@tshuli tshuli self-assigned this Feb 16, 2021
@r00dgirl r00dgirl added P3 and removed P2 planned for next 1-2 months labels Mar 10, 2021
@r00dgirl r00dgirl added P2 planned for next 1-2 months and removed P3 labels Apr 15, 2021
@tshuli tshuli self-assigned this May 17, 2021
@tshuli
Copy link
Contributor

tshuli commented May 31, 2021

@liangyuanruo should we extend this to include number field? The same considerations apply as (i) number field does not offer range validation, and (ii) customMin and customMax are redundant

@liangyuanruo
Copy link
Contributor Author

Yes, let's do that!

@liangyuanruo liangyuanruo changed the title Redundant ValidationOption object properties for short text and long text fields Redundant ValidationOption object properties for short text, long text and number fields May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 planned for next 1-2 months tech debt
Projects
None yet
3 participants