search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.33k stars 2.76k forks source link

Android - Room - Unexpected quantity of letters when inputting room name in capital letters #13463

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Open Android app and login
  2. Create a room
  3. Got to room setting to change it's name
  4. Select "Capital letter only" mode and input a new name

Expected Result:

Name should be correctly input

Actual Result:

Unexpected quantity of letters when inputting room name in capital letters

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.37.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/206597527-3f4a03fb-c5ef-4840-ae7c-eff64e45693b.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

s77rt commented 1 year ago

The issue is on RN. https://github.com/facebook/react-native/issues/27449

There is a posted workaround and a possible PR that may fix it but it's still a draft.

Given that room feature is still in beta, perhaps we should do nothing for now.

sketchydroide commented 1 year ago

I think s77rt might be right, also this is an old bug on RN (2019), not sure why they have not tried to fix it yet

sketchydroide commented 1 year ago

I wonder if someone with an android test what happens in Android Whatsapp?

sketchydroide commented 1 year ago

Asked @Julesssss for some help to test here, and in Whatsapp this does not happen, therefor I think we should keep this open, and try to see if there is a fix for it.

syedsaroshfarrukhdot commented 1 year ago

Proposal

As it is known bug in React Native for android and there is no actual fix for it at the moment. We can use a workaround to fix the problem by keyboardType={"visible-password"} and by turning autoCorrect={none} in index.android.js.

           <TextInput
            ref={this.props.forwardedRef}
            disabled={this.props.disabled}
            label={this.props.translate('newRoomPage.roomName')}
            prefixCharacter={CONST.POLICY.ROOM_PREFIX}
            placeholder={this.props.translate('newRoomPage.social')}
            onChange={this.setModifiedRoomName}
            value={this.props.value.substring(1)} // Since the room name always starts with a prefix, we omit the first character to avoid displaying it twice.
            errorText={this.props.errorText}
            autoCapitalize="none"
            maxLength={CONST.REPORT.MAX_ROOM_NAME_LENGTH}
            autoCorrect={none}
            keyboardType={"visible-password"}
        />

After Fix

https://user-images.githubusercontent.com/81307212/207496591-319cda56-8b53-4e54-ae3e-04aa2d1bb265.mp4

@sketchydroide Is there any reason we are restricting user from using capital letters in Room Name ? As on WhatsApp for instance for Group Name they don't restrict user from entering capital letters that's why this doesn't happen there we are forcing room name to be lowercase.

Whatsapp for reference :-

https://user-images.githubusercontent.com/81307212/207497924-12105c71-13a1-43a1-a53e-68744dde601e.mp4

s77rt commented 1 year ago

@syedsaroshfarrukhdot I don't think we want a workaround for this, besides you posted the workaround I linked.

sketchydroide commented 1 year ago

I think the idea was to do something like slack does, and to not allow capital letter in the room/channel name.

sketchydroide commented 1 year ago

I think I understand the underlying issue here, because there is a feedback loop using the onChange to reset the text as lowercase. I think this creates an issue where it's trying to set the new text on top of the old one and creates copies. No idea if we can fix this localy like it was mentioned here before. But I think we should at least remove the autoCapitalize="none", as it's not doing anything we are doing the calitalization with code anyway.

s77rt commented 1 year ago

@sketchydroide What about the use of style textTransform: 'uppercase'? Won't this also recreate the bug? (not tested) I think we should find out why using keyboardType="visible-password" works

sketchydroide commented 1 year ago

@s77rt and @syedsaroshfarrukhdot while I appreciate the feedback, and your link helped understand the problem @s77rt, please remember that we will only pay for solutions that are presented in External labeled GHs (this one is not, and probably will not be) We are currently reviewing this process, as more internal engineers are tackling Bugs as part of the WAQ effort. I say this for your best interest so you don't loose to much effort on a GH that you will most likely not be payed for.

This said yes I think at this stage I will test the removing autoCapitalize="none" and/or check the soltuion in the RN link keyboardType="visible-password"

s77rt commented 1 year ago

@sketchydroide I'm aware of that and I appreciate your concern

sketchydroide commented 1 year ago

keyboardType="visible-password" seems to work for this case, so building a PR for this, I'm also removing autoCapitalize="none" as I feel it's confusing, as it does nothing, the onChange does.

sketchydroide commented 1 year ago

G has been Merged, we can start the payment for the C+ @johncschuster 🙇🏼

sketchydroide commented 1 year ago

no update, waiting on payment still I think

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.41-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-12-27. :confetti_ball:

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

sketchydroide commented 1 year ago

This fix is now in prod, the only thing missing is payments. @johncschuster not sure if you are arounf to help with that :)

sketchydroide commented 1 year ago

bump @johncschuster seems OOO will probably wait for next week

sketchydroide commented 1 year ago

John still OOO today, maybe tomorrow

sketchydroide commented 1 year ago

@johncschuster bump

sketchydroide commented 1 year ago

change the title since it's awayting payment

johncschuster commented 1 year ago

@sketchydroide thanks for the bumps! I'm finally getting caught up from being OOO. I'll get the payment issued in just a moment.

johncschuster commented 1 year ago

@thesahindia can you apply for the Upwork job when you get the chance?

https://www.upwork.com/jobs/~012a221d0f9589cf03

JmillsExpensify commented 1 year ago

Added the reviewing label so that this issue isn't our WAQ-focused list.

johncschuster commented 1 year ago

@thesahindia, I've accepted your proposal in Upwork. Can you accept the offer? Once you've done that, I'll get the payment issued.

thesahindia commented 1 year ago

@johncschuster accepted, thanks!

johncschuster commented 1 year ago

Paid! 🎉

sketchydroide commented 1 year ago

I think there is nothing left to do here, closing