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

[23.0] Fix for Numeric form field to allow Null min/max values in validation methods #18585

Merged

Conversation

hujambo-dunia
Copy link
Contributor

@hujambo-dunia hujambo-dunia commented Jul 22, 2024

Addresses: #16335 and #18161

demo_v1_Fix_Allows_Null

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@hujambo-dunia hujambo-dunia marked this pull request as ready for review July 22, 2024 16:19
@github-actions github-actions bot added this to the 24.2 milestone Jul 22, 2024
@hujambo-dunia
Copy link
Contributor Author

gmx_trjQAFixNumericValidation.XML.txt

@bernt-matthias Could you confirm the demo video above is as expected? (Demo XML file for video attached.)

@davelopez I removed the !isNaN() method since it outputs True when input was Null which is partly what caused the error. I believe the FormNumber.vue is expecting Null to not be a number in order that the slider visualization does not appear when there is no maximum range listed. This was the only use of isNan() I could find in our *.vue files which is good.

@mvdbeek Unsure why 1 check is failing ("Selenium tests / Test (3.8, 0) (pull_request)"), if this is an issue related to my code that needs to be resolved prior to merge, please let me know.

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Video looks good to me. Can't say much about the code changes.

Copy link
Contributor

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Thank you @hujambo-dunia
You may need to target an older branch (probably 23.0), as this is a bug fix.

@hujambo-dunia hujambo-dunia requested a review from dannon July 25, 2024 14:33
@hujambo-dunia hujambo-dunia changed the base branch from dev to release_23.0 July 25, 2024 22:02
Copy link
Contributor

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Thank you!

Test failures are unrelated.

At some point, when this component gets refactored to composition API + Typescript it will be easier to catch and handle all these type mismatches 👍

@mvdbeek mvdbeek changed the title Fix for Numeric form field to allow Null min/max values in validation methods [23.0] Fix for Numeric form field to allow Null min/max values in validation methods Jul 31, 2024
@davelopez davelopez merged commit 2384c89 into galaxyproject:release_23.0 Jul 31, 2024
30 of 32 checks passed
@galaxyproject galaxyproject deleted a comment from github-actions bot Jul 31, 2024
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.

3 participants