[Pay on 3/8][$750] New room can be created with blank name - Reported by Abhishek Raj Pandey

[mvtglobally]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open app
2. Create Workspace
3. Select "+" > New room
4. Enter any name and delete
5. Hit Create room

Expected Result:

User should not be able to save a room without a name

Actual Result:

Room created without name

Context: (More details in Slack tread)
As per code in line 166, if shouldDisableSubmit is true then the create room button disables and when it becomes false create room button becomes enabled. The problem here is when we put the room's name on "Room Name" field the function called checkAndModifyRoomName() in line 105 will put hash symbol i.e "#" in front of room name in line 111. we don't see "#" on field because the value on room name field is given without "#" symbol using substr this.state.roomName.substr(1) in line 151.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.24-0
Reproducible in staging?: Y
Reproducible in production?: N (new feature)
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Bug.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ABee-Tech
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1640874300265900

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @nkuoch (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[ABee-Tech]

I am Abhishek Raj Pandey. Here is my GH Handle: @ABee-Tech

[MelvinBot]

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[mananjadhav]

This is because we're storing # at the beginning of the room name.

Two approaches:

1. Now in the whole WorkspaceNewRoomPage, we're using 'substr(1)' but in the shouldDisableSubmit flag we're using only this.state.roomName. Just updating !this.state.roomName ==> !this.state.roomName.substring(1) will solve this.

2. The second approach might be some cleanup to always keep this.state.roomName without the # at the beginning and do the reverse of adding it wherever required.

I'll go with the first approach as it'll be a small fix and not require changes across the page.

[ABee-Tech]

Here is my proposal I have submitted into slack thread just after a minute I found the bug.
https://expensify.slack.com/archives/C01GTK53T8Q/p1640874569266400?thread_ts=1640874300.265900&cid=C01GTK53T8Q

Proposal:

In the line: 131 of file pages/workspace/WorkspaceNewRoomPage.js
As per code in line 166, if shouldDisableSubmit is true then the create room button disables and when it becomes false create room button becomes enabled.
The problem here is when we put the room's name on "Room Name" field the function called checkAndModifyRoomName() in line 105 will put hash symbol i.e "#" in front of room name in line 111. we don't see "#" on field because the value on room name field is given without "#" symbol using substr this.state.roomName.substr(1) in line 151.
So if we want "#" to be there in the state then my proposed solution will be changes in two places.

1. set state roomName: "#" as default on component's constructor.
2. adding substr on condition in line 166 changing code from:

const shouldDisableSubmit = Boolean(
      !this.state.roomName || !this.state.policyID || this.state.error,
    );

to:

const shouldDisableSubmit = Boolean(
      !this.state.roomName.substr(1) || !this.state.policyID || this.state.error,
    );

[mananjadhav]

set state roomName: "#" as default on component's constructor.

@ABee-Tech Why would you want to set the default value as '#' in the constructor?

[parasharrajat]

Thanks for the proposal's guys.

@ABee-Tech I will have the same question as @mananjadhav. Apart from it point 2 looks good.

I have also, thinking why don't we always keep the roomName without # in the state. And only append # when sending the data to the backend. This will require a small refactor of checkAndModifyRoomName.

we also need a backend solution to throw an error when the room name is empty. There should be a backend validation as well.

[mananjadhav]

I have also, thinking why don't we always keep the roomName without # in the state

Exactly what I said in the second approach here. It is much cleaner than having .substr at all places.

This will require a small refactor of checkAndModifyRoomName.

And removing substr call at all places.

[ABee-Tech]

Point 2 looks great. In that way, the code will not get messy using substr(1) everywhere.

[parasharrajat]

Could you please post a proposal based on point 2?

[ABee-Tech]

Proposal for the solution to avoid using too many substr(1):

The prefixCharacter prop on TextInputWithLabel cannot be used to just to show # in front of text input without affecting state. We can declare another prop to allow prefix character in state as allowPrefixCharacterInState or addPrefixCharacterOnChangeText which will add condition on file components/TextInputWithPrefix in line 47 like:

onChangeText={text => props.onChangeText(`${props.allowPrefixCharacterInState ? props.prefixCharacter : null}${text}`)}

This solution will allow us to pass just this.state.roomName on value of TextInput on file pages/workspace/WorkspaceNewRoomPage.js in line 151. And allow to remove substr(1) in line 106.

And, the last thing we should do to sent room name with # to the API is by updating onSubmit() function on the line 91 from:

    onSubmit() {
        Report.createPolicyRoom(this.state.policyID, this.state.roomName, this.state.visibility);
    }

to:

    onSubmit() {
        Report.createPolicyRoom(this.state.policyID, '#'+this.state.roomName, this.state.visibility);
    }

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

[MelvinBot]

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[mallenexpensify]

Job posted https://www.upwork.com/jobs/~0167335f0a64da17cb

[marcaaron]

This is still in review. Also dropping a note here that we identified an improvement and @ABee-Tech has agreed to do a small refactor and there will be a fee increase.

[mallenexpensify]

👍, in the future @marcaaron will you mention the amount and tag the CM in the comment? I updated the amount in the title to $500, I'm assuming that's the new total price, let me know if now. Since I've already hired @ABee-Tech , I don't think upping the job price in Upwork will make a difference.

[MelvinBot]

Triggered auto assignment to @dylanexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[mallenexpensify]

@dylanexpensify I'm out next week so auto-added you to keep 👀 on this til I'm back, then I'll take back over.

[parasharrajat]

Requested changes on the PR... Close to finish line.

[ABee-Tech]

@mallenexpensify @marcaaron So, do doubling the fee means doubling the total amount [$500] which I get for finding the issue [$250] and proposal [$250]? Or, only for proposal one only. And, also if you don't mind would you reevaluate the compensation? As it has been more than a month working on the PR. Applied more reusable codes. Can it be increased? It's okay if you reject the request for reevaluation.

[mallenexpensify]

@ABee-Tech , the doubling is specific to fixing the issue. So, currently for this, you're set to receive $500 for the fix plus $250 for reporting, $750 in total. As for increasing the compensation for the fix, the main reasons for that would be if there was increased scope, where additional work was needed outside of the specific bug reported in the OP. Do you think there was? If not, outside of how long the process has taken, can you provide other specific reasoning why you think an increase would be warranted?

[ABee-Tech]

Here is the link to the proposal: #6967 (comment)

I think I have done more work than needed due to some of the merge conflicts on the way and extra changes like change in form behavior and work on other pages than I told to work on. I will explain the things I have worked on within (2022/01/09 to 2022/02/20) 43 days:

1. as on proposal
   file worked on: WorkspaceNewRoomPage.js
   fixes:
   i. fixed room name bug with validation added on the input component.
   ii. added validation and changed behavior of entire form
   iii. added translations for error statements:

• Please enter a room name.
• Please select a workspace.
• Please select a visibility.

2. after solving merge conflicts new component for room name input named RoomNameInput added to file: WorkspaceNewRoomPage.js
   fixes:
   i. every validations related to room name input were removed from parent component WorkspaceNewRoomPage.js and used validations of RoomNameInput.js
   ii. transferred room name related error to RoomNameInput.js component via props to control as the behavior of input was different from ours.
3. Found another use of RoomNameInput component on room settings page i.e. RoomSettingsPage.js
   fixes:
   i. have to revert all the changes that was done in point 2.
   ii. Marc requested to fix the other usage of RoomNameInput with doubled fee.
   iii. removed the validation from the input i.e. RoomNameInput and transferred to parent components i.e. WorkspaceNewRoomPage.js and RoomSettingsPage.js
   iv. changed the form behavior of RoomNameInput component.
   v. moved individual validations as utility functions to ValidationUtils.js for reusability and implemented on WorkspaceNewRoomPage.js and RoomSettingsPage.js. as explained on this comment

[ABee-Tech]

@mallenexpensify Thank you for asking. I have mentioned above how I think increase would be warranted.

[mallenexpensify]

I spoke with @marcaaron about the compensation, we've agreed to increase the amount to $750 because combining the initial fix with the increase in scope created additional work. Thanks @ABee-Tech for the outline above.

Per https://github.com/Expensify/App/blob/main/CONTRIBUTING.md payment will be made 7 days after the PR is deployed to production, assuming there are no regressions.

[ABee-Tech]

Thank you @mallenexpensify for acknowledging my request to re-evaluate the amount. And, thank you @marcaaron @parasharrajat for all your due efforts and support.

[MelvinBot]

@mallenexpensify, @parasharrajat, @marcaaron, @ABee-Tech Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[parasharrajat]

PR is merged to PROD. But why, the title is not updated? 🤔

[mallenexpensify]

Weird, thanks for the ping @parasharrajat .
#7098 (comment)
Will pay on 3/8 if there on no regressions

[mallenexpensify]

D'oh, I paid @ABee-Tech $250 for the reporting the issue on the Upwork job that was closed.
@ABee-Tech and @parasharrajat can you apply for the job here plz. https://www.upwork.com/jobs/~01e968fc3149a1006a and comment on this issue once you have? Thx

[ABee-Tech]

@mallenexpensify That's fine. I have again applied for the job from that link you have provided.

[mallenexpensify]

Hired both @ABee-Tech and @parasharrajat , let me know when accepted and I'll pay

[ABee-Tech]

@mallenexpensify accepted

[mallenexpensify]

@ABee-Tech and @parasharrajat paid $750/each