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

Fix interactive dialog number handling #8431

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Fix interactive dialog number handling #8431

merged 1 commit into from
Dec 19, 2024

Conversation

larkox
Copy link
Contributor

@larkox larkox commented Dec 19, 2024

Summary

The handling of the number field was not done right for a long time. We were passing a number value to the text input value property, which was completely ignoring it (since it was not a string).

The changes we did to be more coherent with the types surfaced the issue, by showing NaN in the input when we removed completely the number.

This PR solves that, and makes sure we only send on the submission proper numbers, and not the empty string for number values.

Ticket Link

Fix https://mattermost.atlassian.net/browse/MM-62334

Release Note

Improve number field validation on Interactive Dialogs

@larkox larkox added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Dec 19, 2024
@larkox larkox requested a review from enahum December 19, 2024 05:15
@enahum enahum added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Dec 19, 2024
@enahum enahum added this to the v2.23.0 milestone Dec 19, 2024
@DHaussermann DHaussermann added the Build Apps for PR Build the mobile app for iOS and Android to test label Dec 19, 2024
@DHaussermann
Copy link

I have done a round of testing on this for the iOS build.

  • Confirmed issue is resolved and number field can be emptied
  • Confirmed validation around number field
  • Confirmed for demo plugin form if I try and paste in text or a decimal they do not get added
  • As a precaution I checked other field types and ensured there's no problem with emptying them after they've had a valur

Tested only on iOS for now. There is an external service failing preventing Android builds at the moment.

@DHaussermann DHaussermann self-requested a review December 19, 2024 07:42
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed based on QA Testing above.

Android will be tested post merge so we can process with creating a release while we wait for for Android builds to be functional again.

@DHaussermann DHaussermann added QA Review Done and removed 3: QA Review Requires review by a QA tester Build Apps for PR Build the mobile app for iOS and Android to test labels Dec 19, 2024
@larkox larkox merged commit 32453b3 into main Dec 19, 2024
53 of 54 checks passed
@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

@larkox larkox deleted the fixDialogNumbers branch December 19, 2024 08:45
mattermost-build pushed a commit that referenced this pull request Dec 19, 2024
@mattermost-build mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Dec 19, 2024
@larkox
Copy link
Contributor Author

larkox commented Dec 19, 2024

/cherry-pick release-2.24

@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit that referenced this pull request Dec 19, 2024
larkox added a commit that referenced this pull request Dec 19, 2024
(cherry picked from commit 32453b3)

Co-authored-by: Daniel Espino García <[email protected]>
larkox added a commit that referenced this pull request Dec 19, 2024
(cherry picked from commit 32453b3)

Co-authored-by: Daniel Espino García <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone QA Review Done release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants