[CP Staging] Fix same name policy rooms and policy room title and mobile app crashing when creating policy room by mountiny · Pull Request #6724 · Expensify/App

mountiny · 2021-12-12T20:20:54Z

cc @TomatoToaster asking you for a review as you have worked on the original PR.

Adding CP Staging label as it fixes two deploy blockers.

Details

For the new Create room page, we are fixing parsing the reportName. Currently, we have been turning the reportName into an empty string, therefore we have not recognized that the value is same (if nothing is typed, the value is #).

At the same time, on mobile as the reportName is an empty string, this line is false and nothing is shown in the View component, which causes the app to fail. By making sure the reportName wont be an empty string, this should not happen.

This also fixes the policy room title not showing.

Fixed Issues

$ #6709
$ #6712
$ #6731

Tests

Click "New Room" in the global create menu. Verify it takes you to <baseURL>/workspace/new-room.
Verify the Room Name input functions correctly:
- Type spaces, verify that they appear as underscores.
- Try to type non-alphanumeric characters (besides underscores) into the room name input, verify nothing shows up.
- Turn your caps-lock and type in the room name input, verify everything appears as lowercase.
Click the dropdown. Verify that all the workspaces (policies) shown match up to the "Free Plan" policies shown for the same account in OldDot (Settings > Policies)
Type test_room_name into the room name input and select a workspace. Click "Create Room". Verify that the room is created and that you navigate to the room (<baseURL>/r/<reportID>).
- NOTE: The room name in the header, the room's entry in the LHN, and the default text won't show up correctly yet. Just verify that you are navigated to a new report. I will open up a separate PR for this, since it'll make this one too cluttered.
Navigate back to <baseURL>/workspace/new-room. Select the same workspace you used in [4]. Then, type test_room_name into the Room Name input. Verify that the A room with this name already exists error message displays (in real-time as you finish typing test_room_name).
Change the workspace in the workspace dropdown. Verify the error disappears. Reselect the same workspace, and verify the error appears again.
Delete the last e off of the room name. Verify the error disappears. Add the last e back, and verify the error appears again.

QA Steps

Same as the above tests with account which has appropriate betas accessible.

Tested On

Screenshots

web.mp4

With the policy room title:

github-actions · 2021-12-12T20:21:08Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

mountiny

Thank you for your review.

mountiny · 2021-12-13T09:26:41Z

+    // For a basic policy room, return its original name
+    if (ReportUtils.isPolicyRoom({chatType})) {
+        return fullReport.reportName;
+    }


The isDefaultPolicy does not check for basic policy room, so we need to add this check here too.

mountiny · 2021-12-13T09:27:30Z

+    const isPolicyRoom = ReportUtils.isPolicyRoom(props.report);
    const isDefaultChatRoom = ReportUtils.isDefaultRoom(props.report);
-    const title = isDefaultChatRoom
+    const title = isDefaultChatRoom || isPolicyRoom


We need to show similar style of title for both, default and policy rooms. I need to keep them separate though as the default dooms have subtitle and avatar, which is a different behaviour and design than policy room.

TomatoToaster

Aha! I should've known there would be consequences for the UI being incomplete on mobile. Good work @mountiny. Good thing this is behind a beta.

TomatoToaster · 2021-12-13T15:46:24Z

Since there is fixing a deploy blocker, I'm going to merge this rn.

botify · 2021-12-13T15:46:33Z

@TomatoToaster looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

[CP Staging] Fix same name policy rooms and policy room title and mobile app crashing when creating policy room (cherry picked from commit ab6b887)

TomatoToaster · 2021-12-13T16:03:01Z

Hmmm weird note because it definitely passed the checks on commit: 9bbd555 - Use isPolicyRoom. I guess except the Explain Cherry-Pick label via OSBotify comment didn't but I'm not sure what more is needed for that. Both @mountiny and I commented on this PR.

OSBotify · 2021-12-16T01:26:44Z

🚀 Deployed to staging by @TomatoToaster in version: 1.1.20-3 🚀

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

OSBotify · 2021-12-20T12:09:04Z

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

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

mountiny added 2 commits December 12, 2021 20:15

Make sure to return report name for the policy room

07b9ae9

Update the comment to reflect change

8831bac

mountiny added the CP Staging label Dec 12, 2021

mountiny requested a review from TomatoToaster December 12, 2021 20:20

mountiny self-assigned this Dec 12, 2021

mountiny requested a review from a team as a code owner December 12, 2021 20:20

MelvinBot requested review from aldo-expensify and removed request for a team December 12, 2021 20:21

mountiny added 2 commits December 12, 2021 20:45

Fix lint

783496a

Update a comment

23943f3

mountiny mentioned this pull request Dec 12, 2021

Add the NewRoomPage #6314

Merged

5 tasks

mountiny added 2 commits December 13, 2021 09:15

Make sure to display room name

4d74742

Use isPolicyRoom

9bbd555

mountiny changed the title ~~[CP Staging] Fix being able to create rooms with the same name~~ [CP Staging] Fix same name policy rooms and policy room title and mobile app crashing when creating policy room Dec 13, 2021

mountiny commented Dec 13, 2021

View reviewed changes

TomatoToaster approved these changes Dec 13, 2021

View reviewed changes

TomatoToaster merged commit ab6b887 into main Dec 13, 2021

TomatoToaster deleted the vit-fixRoomName branch December 13, 2021 15:46

botify added the Emergency label Dec 13, 2021

OSBotify pushed a commit that referenced this pull request Dec 13, 2021

Merge pull request #6724 from Expensify/vit-fixRoomName

74f7631

[CP Staging] Fix same name policy rooms and policy room title and mobile app crashing when creating policy room (cherry picked from commit ab6b887)

OSBotify mentioned this pull request Dec 13, 2021

🍒 Cherry pick PR #6724 to staging 🍒 #6736

Merged

TomatoToaster removed the Emergency label Dec 13, 2021

kavimuru mentioned this pull request Dec 13, 2021

iOS -Chat - App crashes tapping the room name in the chat #6740

Closed

isagoico mentioned this pull request Dec 13, 2021

Deploy Checklist: New Expensify 2021-12-10 #6703

Closed

38 tasks

OSBotify mentioned this pull request Dec 15, 2021

Deploy Checklist: New Expensify 2021-12-15 #6778

Closed

37 tasks

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CP Staging] Fix same name policy rooms and policy room title and mobile app crashing when creating policy room#6724

[CP Staging] Fix same name policy rooms and policy room title and mobile app crashing when creating policy room#6724
TomatoToaster merged 6 commits into
mainfrom
vit-fixRoomName

mountiny commented Dec 12, 2021 •

edited

Loading

Uh oh!

github-actions Bot commented Dec 12, 2021

Uh oh!

mountiny left a comment

Uh oh!

mountiny Dec 13, 2021

Uh oh!

mountiny Dec 13, 2021

Uh oh!

TomatoToaster left a comment

Uh oh!

TomatoToaster commented Dec 13, 2021

Uh oh!

botify commented Dec 13, 2021

Uh oh!

TomatoToaster commented Dec 13, 2021

Uh oh!

OSBotify commented Dec 16, 2021

Uh oh!

OSBotify commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mountiny commented Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Uh oh!

github-actions Bot commented Dec 12, 2021

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

mountiny Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

TomatoToaster left a comment

Choose a reason for hiding this comment

Uh oh!

TomatoToaster commented Dec 13, 2021

Uh oh!

botify commented Dec 13, 2021

Uh oh!

TomatoToaster commented Dec 13, 2021

Uh oh!

OSBotify commented Dec 16, 2021

Uh oh!

OSBotify commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mountiny commented Dec 12, 2021 •

edited

Loading