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

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

Closed
kbecciv opened this issue Dec 9, 2022 · 29 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Dec 9, 2022

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?

  • Android

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

Bug5855634_Screenrecorder-2022-12-08-20-53-30-25_1___1_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 9, 2022

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

@sketchydroide sketchydroide self-assigned this Dec 9, 2022
@s77rt
Copy link
Contributor

s77rt commented Dec 10, 2022

The issue is on RN.
facebook/react-native#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.

@melvin-bot melvin-bot bot added the Overdue label Dec 12, 2022
@sketchydroide
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2022
@sketchydroide
Copy link
Contributor

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

@sketchydroide
Copy link
Contributor

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
Copy link
Contributor

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

WhatsApp.Video.2022-12-14.at.8.13.07.AM.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 :-

WhatsApp.Video.2022-12-14.at.8.24.09.AM.mp4

@s77rt
Copy link
Contributor

s77rt commented Dec 14, 2022

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

@sketchydroide
Copy link
Contributor

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

@sketchydroide
Copy link
Contributor

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
Copy link
Contributor

s77rt commented Dec 14, 2022

@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
Copy link
Contributor

@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
Copy link
Contributor

s77rt commented Dec 14, 2022

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

@sketchydroide
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Dec 15, 2022
@sketchydroide
Copy link
Contributor

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

@sketchydroide
Copy link
Contributor

no update, waiting on payment still I think

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 20, 2022
@melvin-bot melvin-bot bot changed the title Android - Room - Unexpected quantity of letters when inputting room name in capital letters [HOLD for payment 2022-12-27] Android - Room - Unexpected quantity of letters when inputting room name in capital letters Dec 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 20, 2022

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

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

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

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

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

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Dec 20, 2022

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] The PR that introduced the bug has been identified. Link to the PR:
  • [@sketchydroide] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@sketchydroide] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@johncschuster] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 26, 2022
@sketchydroide
Copy link
Contributor

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

@sketchydroide
Copy link
Contributor

bump @johncschuster seems OOO will probably wait for next week

@sketchydroide
Copy link
Contributor

John still OOO today, maybe tomorrow

@sketchydroide
Copy link
Contributor

@johncschuster bump

@sketchydroide sketchydroide changed the title [HOLD for payment 2022-12-27] Android - Room - Unexpected quantity of letters when inputting room name in capital letters Android - Room - Unexpected quantity of letters when inputting room name in capital letters Jan 5, 2023
@sketchydroide sketchydroide removed the Reviewing Has a PR in review label Jan 5, 2023
@sketchydroide
Copy link
Contributor

change the title since it's awayting payment

@johncschuster
Copy link
Contributor

@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
Copy link
Contributor

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

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

@JmillsExpensify JmillsExpensify added the Reviewing Has a PR in review label Jan 6, 2023
@JmillsExpensify
Copy link

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

@johncschuster
Copy link
Contributor

@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
Copy link
Member

@johncschuster accepted, thanks!

@johncschuster
Copy link
Contributor

Paid! 🎉

@sketchydroide
Copy link
Contributor

I think there is nothing left to do here, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants