Allow Welcome Message on Room Creation #27836

dangrous · 2023-09-20T07:51:44Z

Details

You'll get the "too many auth calls" alert for this, due to the welcome message being added as an NVP. @jasperhuangg let me know how you want to handle this, whether we just add it to the list of exceptions, or if we need to somehow roll it into Auth. For local testing, please add the command to the list of exceptions otherwise you'll get an error when you try to create the room and won't be able to test. Staging server should be fine.

This also covers a bit of a refactor for the room creation page, using push-to-page selectors. Since we didn't really have a generic one of these before for use in forms, I created one. It takes a list of items (with values and labels) and will make them into one of these pop up selection modals. Handles display of the selected item, and offers an option for an initial placeholder (though you can't get back to it afterwards, which isn't an issue here since selections are required for all).

I'm not 100% confident on the name of the new component, so any suggestions are welcome.

Lastly, I also had to update the NewChatSelectorPage to include withOnyx because otherwise it wasn't actually able to access the betas prop, but now it works.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/316189
PROPOSAL: N/A

Tests

Same as QA

Verify that no errors appear in the JS console

Offline tests

Same as QA (and confirm that the room is correctly created with no errors once you return online)

QA Steps

Log into an account that has at least two workspaces (or create them)
Open the FAB
Click "Send message"
Toggle over to "Room"
Enter a room name
Ensure that an error is shown if you do NOT enter a room name
Enter a welcome message
Ensure that the form shows an error if you do not select a workspace (and try to create room)
Select a workspace - ensure that all possible workspaces are in the list and selectable
Ensure that your selection persists in the main form
If you are an admin of a workspace, ensure that the "Who can post" selector appears once you have chosen that workspace.
Test that all options for "Who can post" are selectable and persist to the main form
Test that all options for "Visibility" are selectable and persist to the main form
Submit the form and ensure that the room is created as expected with the correct qualities.
Log in to another account and navigate to that room (assuming it is public).
Ensure you are sent the welcome message you entered in step 6.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Uploading web.mp4…

Mobile Web - Chrome

I can't get this to login, on main either, so I'm forgoing this. Some combo of Bali + ngrok + cloudflare + etc. etc.. If you could double test this it would be appreciated!

Mobile Web - Safari

mWebSafari.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

This is coming! Taking forever to build so I wanted to get a head start.

shawnborton · 2023-09-21T01:48:17Z

Just a note that I think we want mb5 for our form spacing for this one, cc @Expensify/design

melvin-bot · 2023-09-21T08:24:37Z

@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot · 2023-09-24T02:55:42Z

@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

eVoloshchak · 2023-09-24T09:45:49Z

src/pages/workspace/WorkspaceNewRoomPage.js

+                                    accessibilityLabel={translate('welcomeMessagePage.welcomeMessage')}
+                                    accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
+                                    autoGrowHeight
+                                    placeholder={translate('welcomeMessagePage.welcomeMessage')}


I think this is redundant

We aren't using a placeholder on other pages (i.e. task creation modal, personal details)

whoops, not sure where that came from

eVoloshchak · 2023-09-24T09:53:20Z

src/pages/workspace/WorkspaceNewRoomPage.js

+                            <View style={[styles.mb5]}>
+                                <TextInput
+                                    inputID="welcomeMessage"
+                                    label={translate('welcomeMessagePage.welcomeMessage')}


It isn't clear that welcomeMessage is optional, we could change it to Welcome message (optional) similar to how it's done for task description

yeah makes sense to me

eVoloshchak · 2023-09-24T10:03:57Z

Besides the two small comments above everything else looks good. I can't test the actual functionality of this until BE change is deployed, but all the FE changes work as expected

dangrous · 2023-09-25T03:11:45Z

@eVoloshchak BE change is on staging now so you should be able to test!

Will review your feedback in a couple mins but wanted to call that out.

dangrous · 2023-09-25T03:48:53Z

Updated!

eVoloshchak · 2023-09-25T08:41:36Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-09-29.at.16.46.43.mov

Mobile Web - Chrome

screen-20230929-165144.mp4

Mobile Web - Safari

RPReplay_Final1695999686.MP4

Desktop

Screen.Recording.2023-09-29.at.17.02.13.mov

iOS

RPReplay_Final1696000840.MP4

Android

screen-20230929-165848.mp4

dangrous · 2023-09-29T05:21:08Z

Okay so I think I'm almost there. If you remove shouldEnableMaxHeight and shouldEnableKeyboardAvoidingView={false} from the ScreenWrapper in NewChatSelectorPage, and then add back scrollContextEnabled into the Form of WorkspaceNewRoomPage then it seems to work as expected except there's a bunch of empty space underneath the submit button of the room tab, and under the chat list of the Chat tab.

2023-09-29_13-19-02.mp4

dangrous · 2023-09-29T05:23:46Z

Okay good/bad - there's actually already an existing bug with this weird space. If you have a long list of contacts on the Chat tab, the page is already cut off too high - with no changes from main:

So I don't like it BUT I think we might be able to merge without fixing the padding since I'm guessing it's the same source. And then we can handle that separately, in the interest of closing this one out.

What do you think @eVoloshchak cc @MitchExpensify

dangrous · 2023-09-29T08:29:14Z

Okay confirmed in Slack - we're going to move forward and handle that padding bug separately (reported here). Let us know if you can do a final review with that in mind, @eVoloshchak - thank you!

eVoloshchak · 2023-09-29T09:07:19Z

Found another bug, this is the last one:

Click FAB -> Send Message -> Room
Enter a room name
Select a workspace you're the admin of
Click on "Who can post" and select "Admins only"
Then select another workspace you are not the admin of
Press "Create room", you're redirected to a newly created room
Notice that it still says "Only admins can send messages in this room."

Screen.Recording.2023-09-29.at.11.05.20.mov

cc: @dangrous

dangrous · 2023-09-29T12:17:23Z

Can you double check that that doesn't happen on staging?

I'm not 100% sure it's a bug - like, it's silly to do so, but I guess there's no reason you can't create a room you can't post in - but maybe we should revert the writeCapability back to the default if isPolicyAdmin is false for the selected policy?

dangrous · 2023-09-29T12:28:12Z

Ah, it doesn't. Will fix!

dangrous · 2023-09-29T12:41:27Z

Updated - should work now! Grabs the value from state, and updates it if !isPolicyAdmin. I also fixed a console error that came from merging main (all ScreenWrappers need a testID)

eVoloshchak

Went through the checklist once more, LGTM

The re-rendering problem on iOS came back, but at this point I'm not sure if this is a legitimate bug or a problem with my ENV, since it's not reproducible by others and is not 100% reproducible by me, so approving.
We could fix this in a follow-up PR if QA will experience the same

melvin-bot · 2023-09-29T15:44:46Z

@marcochavezf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot · 2023-09-29T15:44:49Z

🎯 @eVoloshchak, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #28503.

marcochavezf

The code LGTM, waiting for @jasperhuangg if has some opinion about the "too many auth calls" alert. Or create a follow-up PR to add it to the whitelist and feel free to merge.

dangrous · 2023-10-04T08:07:48Z

I'm gonna merge and create a quick followup issue for the Auth calls thing

OSBotify · 2023-10-04T08:13:18Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-10-05T00:39:21Z

🚀 Deployed to staging by https://github.com/dangrous in version: 1.3.78-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

…messages" This reverts commit 447d87e, reversing changes made to 7f063c7.

src/pages/workspace/WorkspaceNewRoomPage.js

[CP Staging] Revert "Merge pull request #27836 from Expensify/dangrous-roomwelcomemessages"

[CP Staging] Revert "Merge pull request #27836 from Expensify/dangrous-roomwelcomemessages" (cherry picked from commit d420b0a)

OSBotify · 2023-10-06T07:54:18Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.78-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

add and send welcome message

02b3918

dangrous self-assigned this Sep 20, 2023

dangrous changed the title ~~Allow Welcome Message on Room Creation~~ [WIP] Allow Welcome Message on Room Creation Sep 20, 2023

add initial ValuePicker component

34531ee

dangrous mentioned this pull request Sep 21, 2023

[$500] Dev: Web - Inconsistence space between input fields in #Rooms #27604

Closed

6 tasks

dangrous added 6 commits September 21, 2023 12:54

handle selected items

4e2f808

clean up

79fe7b7

padding and margins

36fb0e0

update bottom margins

bb5f174

let NewChatSelectorPage actually access the betas

4df151e

merge conflicts

ec64d40

dangrous changed the title ~~[WIP] Allow Welcome Message on Room Creation~~ Allow Welcome Message on Room Creation Sep 21, 2023

dangrous marked this pull request as ready for review September 21, 2023 08:24

dangrous requested a review from a team as a code owner September 21, 2023 08:24

melvin-bot bot requested review from abdulrahuman5196 and removed request for a team September 21, 2023 08:24

MitchExpensify requested review from a team and abdulrahuman5196 and removed request for abdulrahuman5196 and a team September 24, 2023 02:54

melvin-bot bot requested a review from eVoloshchak September 24, 2023 02:55

eVoloshchak suggested changes Sep 24, 2023

View reviewed changes

address feedback - add optional to welcome message

bcbd156

Merge branch 'main' into dangrous-roomwelcomemessages

29370d7

update value based on policy

03f63bf

eVoloshchak approved these changes Sep 29, 2023

View reviewed changes

melvin-bot bot requested a review from marcochavezf September 29, 2023 15:44

melvin-bot bot mentioned this pull request Sep 29, 2023

[Pay on Oct 23] Payment issue for E/App PR#27836 #28503

Closed

kbecciv mentioned this pull request Oct 2, 2023

[$500] DEV: mWeb - Bottom Spacing Issues on NewChatSelectorPage #28609

Closed

6 tasks

marcochavezf approved these changes Oct 3, 2023

View reviewed changes

dangrous merged commit 447d87e into main Oct 4, 2023

dangrous deleted the dangrous-roomwelcomemessages branch October 4, 2023 08:07

OSBotify mentioned this pull request Oct 5, 2023

Deploy Checklist: New Expensify 2023-10-05 #28883

Closed

57 tasks

lanitochka17 mentioned this pull request Oct 5, 2023

[HOLD for payment 2023-10-23] [$500] iOS - Screen flashes after selecting inputs while creating a Room #28950

Closed

6 tasks

mountiny added a commit that referenced this pull request Oct 5, 2023

Revert "Merge pull request #27836 from Expensify/dangrous-roomwelcome…

edd499a

…messages" This reverts commit 447d87e, reversing changes made to 7f063c7.

mountiny reviewed Oct 5, 2023

View reviewed changes

src/pages/workspace/WorkspaceNewRoomPage.js Show resolved Hide resolved

mountiny mentioned this pull request Oct 5, 2023

[CP Staging] Revert "Merge pull request #27836 from Expensify/dangrous-roomwelcomemessages" #28955

Merged

59 tasks

mountiny added a commit that referenced this pull request Oct 5, 2023

Merge pull request #28955 from Expensify/vit-revert27836

d420b0a

[CP Staging] Revert "Merge pull request #27836 from Expensify/dangrous-roomwelcomemessages"

OSBotify pushed a commit that referenced this pull request Oct 5, 2023

Merge pull request #28955 from Expensify/vit-revert27836

9716fba

[CP Staging] Revert "Merge pull request #27836 from Expensify/dangrous-roomwelcomemessages" (cherry picked from commit d420b0a)

mountiny mentioned this pull request Oct 13, 2023

Allow Welcome Message on Room Creation #29541

Merged

59 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Welcome Message on Room Creation #27836

Allow Welcome Message on Room Creation #27836

dangrous commented Sep 20, 2023 •

edited

Loading

shawnborton commented Sep 21, 2023

melvin-bot bot commented Sep 21, 2023

melvin-bot bot commented Sep 24, 2023

eVoloshchak Sep 24, 2023

dangrous Sep 25, 2023

eVoloshchak Sep 24, 2023

dangrous Sep 25, 2023

eVoloshchak commented Sep 24, 2023

dangrous commented Sep 25, 2023

dangrous commented Sep 25, 2023

eVoloshchak commented Sep 25, 2023 •

edited

Loading

dangrous commented Sep 29, 2023

dangrous commented Sep 29, 2023 •

edited

Loading

dangrous commented Sep 29, 2023

eVoloshchak commented Sep 29, 2023 •

edited

Loading

dangrous commented Sep 29, 2023

dangrous commented Sep 29, 2023

dangrous commented Sep 29, 2023 •

edited

Loading

eVoloshchak left a comment

melvin-bot bot commented Sep 29, 2023

melvin-bot bot commented Sep 29, 2023

marcochavezf left a comment

dangrous commented Oct 4, 2023

OSBotify commented Oct 4, 2023

OSBotify commented Oct 5, 2023

OSBotify commented Oct 6, 2023

Allow Welcome Message on Room Creation #27836

Allow Welcome Message on Room Creation #27836

Conversation

dangrous commented Sep 20, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

shawnborton commented Sep 21, 2023

melvin-bot bot commented Sep 21, 2023

melvin-bot bot commented Sep 24, 2023

eVoloshchak Sep 24, 2023

Choose a reason for hiding this comment

dangrous Sep 25, 2023

Choose a reason for hiding this comment

eVoloshchak Sep 24, 2023

Choose a reason for hiding this comment

dangrous Sep 25, 2023

Choose a reason for hiding this comment

eVoloshchak commented Sep 24, 2023

dangrous commented Sep 25, 2023

dangrous commented Sep 25, 2023

eVoloshchak commented Sep 25, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

dangrous commented Sep 29, 2023

dangrous commented Sep 29, 2023 • edited Loading

dangrous commented Sep 29, 2023

eVoloshchak commented Sep 29, 2023 • edited Loading

dangrous commented Sep 29, 2023

dangrous commented Sep 29, 2023

dangrous commented Sep 29, 2023 • edited Loading

eVoloshchak left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Sep 29, 2023

melvin-bot bot commented Sep 29, 2023

marcochavezf left a comment

Choose a reason for hiding this comment

dangrous commented Oct 4, 2023

OSBotify commented Oct 4, 2023

OSBotify commented Oct 5, 2023

OSBotify commented Oct 6, 2023

dangrous commented Sep 20, 2023 •

edited

Loading

eVoloshchak commented Sep 25, 2023 •

edited

Loading

dangrous commented Sep 29, 2023 •

edited

Loading

eVoloshchak commented Sep 29, 2023 •

edited

Loading

dangrous commented Sep 29, 2023 •

edited

Loading