Fix interactive dialogs not showing #8427

larkox · 2024-12-18T16:41:18Z

Summary

We were forcing having a value for the dialog elements. This PR removes that limitation, and fixes the types around it.

Ticket Link

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

Release Note

Fix issue with interactive dialog not showing all the elements

cpoile

Non-blocking comments/questions. Thanks @larkox!

cpoile · 2024-12-18T17:02:44Z

app/components/settings/text_setting.tsx

@@ -55,7 +55,7 @@ type Props = {
    maxLength?: number;
    optional: boolean;
    onChange: (value: string) => void;
-    value: string;
+    value: string | undefined;


nit: any reason why it's not

Suggested change

value: string | undefined;

value?: string;

to match the ones above?

First one I did, I forgot to update 😅

cpoile · 2024-12-18T17:06:30Z

app/screens/interactive_dialog/dialog_element.tsx

@@ -59,7 +74,7 @@ function DialogElement({
            return;
        }
        onChange(name, newValue);
-    }, [onChange, type, subtype]);
+    }, [type, subtype, onChange, name]);


for the parseInt above, does newValue as string work if newValue is a string[]?

The idea is that string[] is only getting into this function if we were to accept multiselect selectors (we accept that for Apps but not for Interactive Dialogs). But then the type and subtype won't be text and number.

rahimrahman

I only have questions.

I would have loved to see a test to cover the changes, specifically the "fix" that would cause the dialog element to appear because the value is no longer blank?

app/screens/interactive_dialog/dialog_element.tsx

rahimrahman · 2024-12-18T17:28:02Z

app/screens/interactive_dialog/dialog_element.tsx

@@ -22,6 +22,21 @@ function selectKeyboardType(type: InteractiveDialogElementType, subtype?: Intera
    return selectKB(subtype);
 }

+function getStringValue(value: string | number | boolean | string[] | undefined): string | undefined {


Any value (heh, pun intended) in converting false to string and gets "false"?

I was thinking of option "true" / "false", but I have a feeling this gets converted somewhere already.

if (typeof value === 'number' || typeof value === 'boolean') { return value.toString(); }

Maybe if I understand correctly, the initValues function would have converted that.

Suggested change

function getStringValue(value: string | number | boolean | string[] | undefined): string | undefined {

function getStringValue(value?: string | number | boolean | string[]): string | undefined {

True/false values should only be used on Boolean settings, and those will use the getBooleanValue. If a boolean value gets into this point, probably is because something is "wrong", and undefined is the safe approach.

But I don't have a strong opinion on this. Nevertheless, it "should never happen".

Regarding the change to getStringValue, I would argue the argument is not optional. You must provide an argument, just that that argument can be undefined. 0/5 on that too, so if you have a strong opinion, happy to change it.

I was thinking of consistency. Since if people can pass undefined, it's almost like not
passing anything. But it is silly to see getStringValue(). I rather see people send getStringValue(undefined) instead of getStringValue().

Long to short: don't change. :P

larkox · 2024-12-18T18:08:16Z

@rahimrahman Re: Tests https://mattermost.atlassian.net/browse/MM-62306

DHaussermann

Confirmed Demo Plugin contains dialogue with several fields and field type some of witch were affected by the issue
Confirmed all fields are now showing including those affected by this issue
All dialogues I tested can be open and submitted (or canceled)
As a precaution, I used an ngrok tunnel to see the payload in the submission all fields including those affected are submitting data accurately
Regression testes using fields and dialogues of various types
Testing done on both Android and iOS
No issues found

LGTM!

jwilander · 2024-12-18T21:01:24Z

/cherry-pick release-2.23

mattermost-build · 2024-12-18T21:01:27Z

Cherry pick is scheduled.

* Fix interactive dialogs not showing * Use conditionals (cherry picked from commit d75cbcd)

jwilander · 2024-12-18T21:01:49Z

/cherry-pick release-2.24

mattermost-build · 2024-12-18T21:01:51Z

Cherry pick is scheduled.

* Fix interactive dialogs not showing * Use conditionals (cherry picked from commit d75cbcd)

* Fix interactive dialogs not showing * Use conditionals (cherry picked from commit d75cbcd) Co-authored-by: Daniel Espino García <[email protected]>

Fix interactive dialogs not showing

559e482

larkox added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Dec 18, 2024

larkox requested review from a team and hmhealey and removed request for a team December 18, 2024 16:41

mm-cloud-bot added the release-note label Dec 18, 2024

jwilander requested review from cpoile, rahimrahman and DHaussermann and removed request for hmhealey December 18, 2024 17:07

cpoile approved these changes Dec 18, 2024

View reviewed changes

rahimrahman approved these changes Dec 18, 2024

View reviewed changes

jwilander added this to the v2.23.0 milestone Dec 18, 2024

Use conditionals

de69aa9

DHaussermann added Build App for iOS Build the mobile app for iOS Build Apps for PR Build the mobile app for iOS and Android to test and removed Build App for iOS Build the mobile app for iOS labels Dec 18, 2024

DHaussermann approved these changes Dec 18, 2024

View reviewed changes

DHaussermann added QA Review Done and removed Build Apps for PR Build the mobile app for iOS and Android to test 3: QA Review Requires review by a QA tester labels Dec 18, 2024

jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Dec 18, 2024

jwilander merged commit d75cbcd into main Dec 18, 2024
30 checks passed

jwilander deleted the fixDialogs branch December 18, 2024 20:54

jwilander added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Dec 18, 2024

mattermost-build pushed a commit that referenced this pull request Dec 18, 2024

Fix interactive dialogs not showing (#8427)

e94a0e1

* Fix interactive dialogs not showing * Use conditionals (cherry picked from commit d75cbcd)

mattermost-build mentioned this pull request Dec 18, 2024

Automated cherry pick of #8427 #8428

Merged

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 18, 2024

mattermost-build pushed a commit that referenced this pull request Dec 18, 2024

Fix interactive dialogs not showing (#8427)

f06fcac

* Fix interactive dialogs not showing * Use conditionals (cherry picked from commit d75cbcd)

mattermost-build mentioned this pull request Dec 18, 2024

Automated cherry pick of #8427 #8429

Merged

jwilander pushed a commit that referenced this pull request Dec 18, 2024

Fix interactive dialogs not showing (#8427) (#8429)

b7d4242

* Fix interactive dialogs not showing * Use conditionals (cherry picked from commit d75cbcd) Co-authored-by: Daniel Espino García <[email protected]>

jwilander pushed a commit that referenced this pull request Dec 18, 2024

Fix interactive dialogs not showing (#8427) (#8428)

de05f43

* Fix interactive dialogs not showing * Use conditionals (cherry picked from commit d75cbcd) Co-authored-by: Daniel Espino García <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix interactive dialogs not showing #8427

Fix interactive dialogs not showing #8427

larkox commented Dec 18, 2024

cpoile left a comment

cpoile Dec 18, 2024

larkox Dec 18, 2024

cpoile Dec 18, 2024

larkox Dec 18, 2024

rahimrahman left a comment •

edited

Loading

rahimrahman Dec 18, 2024 •

edited

Loading

rahimrahman Dec 18, 2024

rahimrahman Dec 18, 2024

larkox Dec 18, 2024

rahimrahman Dec 18, 2024

larkox commented Dec 18, 2024

DHaussermann left a comment

jwilander commented Dec 18, 2024

mattermost-build commented Dec 18, 2024

jwilander commented Dec 18, 2024

mattermost-build commented Dec 18, 2024

	function getStringValue(value: string \| number \| boolean \| string[] \| undefined): string \| undefined {
	function getStringValue(value?: string \| number \| boolean \| string[]): string \| undefined {

Fix interactive dialogs not showing #8427

Fix interactive dialogs not showing #8427

Conversation

larkox commented Dec 18, 2024

Summary

Ticket Link

Release Note

cpoile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahimrahman left a comment • edited Loading

Choose a reason for hiding this comment

rahimrahman Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larkox commented Dec 18, 2024

DHaussermann left a comment

Choose a reason for hiding this comment

jwilander commented Dec 18, 2024

mattermost-build commented Dec 18, 2024

jwilander commented Dec 18, 2024

mattermost-build commented Dec 18, 2024

rahimrahman left a comment •

edited

Loading

rahimrahman Dec 18, 2024 •

edited

Loading