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

Surface room name errors and warn users about reserved policy room name error #7227

Merged
merged 18 commits into from
Jan 14, 2022

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Jan 14, 2022

CC: @jasperhuangg @luacmartins

Details

Warns users not to use reserved names for rooms. Also adds an inline error so that users can see what the error they're seeing is.

Fixed Issues

Follow up to: https://github.com/Expensify/Auth/pull/6291

Tests/ QA Steps

  1. Go to the green plus on the bottom left and click on New Room
  2. Verify like in the screenshot below, if you try to use #admins or #announce for the room name, it won't let you.
  3. Also verify that it shows you the right error message (see screenshot).
  4. Verify that you also see an error if you try to use the same name as an existing policy room in that same workspace.
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image
image

@TomatoToaster TomatoToaster self-assigned this Jan 14, 2022
@TomatoToaster TomatoToaster changed the title Warn users about reserved policy room name error Surface room name errors and warn users about reserved policy room name error Jan 14, 2022
@TomatoToaster TomatoToaster marked this pull request as ready for review January 14, 2022 16:59
@TomatoToaster TomatoToaster requested a review from a team as a code owner January 14, 2022 16:59
@MelvinBot MelvinBot requested review from danieldoglas and removed request for a team January 14, 2022 17:00
luacmartins
luacmartins previously approved these changes Jan 14, 2022
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Left a NAB comment

containerStyles={[styles.mb5]}
onChangeText={roomName => this.checkAndModifyRoomName(roomName)}
value={this.state.roomName.substring(1)}
errorText={this.state.error}
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB since the input functionality is unrelated to these changes. It's really weird that we pass errorText here, but then omit passing it on to TextInput and have to use InlineErrorText to display the error. Not sure why this is the case @jasperhuangg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OH good catch! Honestly maybe it's better to just pass it in? Maybe there was a weird styling reason why we didn't do it. Let me try modifying that omit and seeing if that makes it better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given how TextInputWithPrefix currently works, we will have to pass it here so the outline border changes to red. I'm not sure why we omit it though, but I assume it's intentional/necessary since it was added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm removing the omit doesn't work - no error text shows up. Maybe it has to do with us using the base ReactNative TextInput there in TextInputWithPrefix/index.js and it doesn't seem to use the errorText correctly. I think I'll still move around the error to TextInputWithPrefix and out of TextInputWithLabel (I don't think this is used anymore)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok I know why we don't use the BaseTextInput we created. It's a less configurable input. I think it still makes sense to move the error around to the TextInputWithPrefix though, so I'll try to do that.
image

danieldoglas
danieldoglas previously approved these changes Jan 14, 2022
Copy link
Contributor

@danieldoglas danieldoglas left a comment

Choose a reason for hiding this comment

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

LGTM

@TomatoToaster TomatoToaster changed the title Surface room name errors and warn users about reserved policy room name error [HOLD] Surface room name errors and warn users about reserved policy room name error Jan 14, 2022
@TomatoToaster TomatoToaster changed the title [HOLD] Surface room name errors and warn users about reserved policy room name error Surface room name errors and warn users about reserved policy room name error Jan 14, 2022
@TomatoToaster
Copy link
Contributor Author

@danieldoglas @luacmartins ready for another review!

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Left a small comment. Otherwise changes LGTM

src/components/TextInputWithPrefix/index.android.js Outdated Show resolved Hide resolved
@TomatoToaster TomatoToaster merged commit 8e11f8a into main Jan 14, 2022
@TomatoToaster TomatoToaster deleted the amal-ucpr-reserved-room-names branch January 14, 2022 23:14
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @TomatoToaster in version: 1.1.29-10 🚀

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

@mvtglobally
Copy link

@TomatoToaster @luacmartins @danieldoglas Should we QA this on Web only?

@luacmartins
Copy link
Contributor

@mvtglobally we should test on all platforms.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.30-3 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants