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

[HOLD for payment 2023-01-30] [$1000] New Room - Capital letters is converted to lowercase when all caps is enable #13676

Closed
kbecciv opened this issue Dec 16, 2022 · 86 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Dec 16, 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!


Issue found when executing PR #13621

Action Performed:

  1. Launch the app
  2. Log in with any account
  3. Press the Green + button and select New Room
  4. On the name field double tap on the shift ⬆️ so the all caps is enabled
  5. Tap in different keys on the actual device keyboard

Expected Result:

Capital letters is visible until an area outside of the field is tapped.
Capital letters is not visible and transaction is smooth when all caps is enabled

Actual Result:

Capital letter changes to lowercase.
Capital letters is visible and transaction isn't smooth when all caps is enable

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.2.41.1

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

Bug5866671_Screen_Recording_20221216-122119_New_Expensify__1_.mp4
Bug5866671_RPReplay_Final1671212576.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01773a5a2201587c50
  • Upwork Job ID: 1612436167049437184
  • Last Price Increase: 2023-01-09
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 16, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 16, 2022

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Dec 16, 2022
@Christinadobrzyn
Copy link
Contributor

This might be related - #13463 @sketchydroide can I get your thoughts if this is the same issue (I see the PR merged so maybe it's not)?

It might also be related to this - #13042 - maybe capital letters are considered a special character right now? @madmax330 can I get your thoughts?

@sketchydroide
Copy link
Contributor

It's not related, but I'm not sure if we can do something about it, this is mobile iOS and Android, the other one was Android only.
Yes it's related to the other one.
The issue is that we take the input of the user, and change it on the fly, so what happens is that the user still sees the text before we change it, giving it this weird jagged behaviour.
@madmax330 if you make changes on the other one it should in princible fix this one, but let us know if I'm wrong.

Either way @Christinadobrzyn I think this GH will be closed.

@Christinadobrzyn
Copy link
Contributor

Thanks @sketchydroide! We'll wait to hear from @madmax330 to see if he might have more info on this issue being resolved with #13042

@Christinadobrzyn
Copy link
Contributor

I'm ooo so going to reassign

@Christinadobrzyn Christinadobrzyn removed the Bug Something is broken. Auto assigns a BugZero manager. label Dec 22, 2022
@Christinadobrzyn Christinadobrzyn removed their assignment Dec 22, 2022
@Christinadobrzyn Christinadobrzyn added the Bug Something is broken. Auto assigns a BugZero manager. label Dec 22, 2022
@mallenexpensify
Copy link
Contributor

Also waiting for feedback from @madmax330

@melvin-bot melvin-bot bot added the Overdue label Dec 26, 2022
@mallenexpensify
Copy link
Contributor

@madmax330 should be back from OOO today

@melvin-bot melvin-bot bot removed the Overdue label Dec 28, 2022
@madmax330
Copy link
Contributor

I don't think my fix for the other issue will fix this.
I think for this issue we should just convert the value to lowercase before we display it instead of displaying it then changing it.

@sketchydroide
Copy link
Contributor

is there a method for that? Atm we are using onChange that seems to be actinc after the change not before. I don't know enough about react

@madmax330
Copy link
Contributor

I think onChange should be fine. It's just that we should make sure the correct value is set before rendering it

@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2022
@mallenexpensify
Copy link
Contributor

@madmax330 @sketchydroide , what's best next step here?

@mallenexpensify
Copy link
Contributor

Assigning @madmax330 and @sketchydroide cuz otherwise they might not see. Can you review the above and provide guidance? plzzzz

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 6, 2023
@jatinsonijs
Copy link
Contributor

Thanks @sketchydroide, applied on Upwork.

@thesahindia Can you plz let me know from where I can confirm translation ?

@jatinsonijs
Copy link
Contributor

I'm using

Spanish: Los nombres de las habitaciones solo pueden incluir letras minúsculas, números y guiones
English: Room names can only include lowercase letters, numbers and hyphens

Need confirmation

@sketchydroide
Copy link
Contributor

sketchydroide commented Jan 18, 2023

Can you plz let me know from where I can confirm translation ?

I'll get that done for you, we normally ask in an internal slack channel where the native Spanish speakers can chime in.

@sketchydroide
Copy link
Contributor

I would say Los nombres de las salas solo pueden contener letras minúsculas, números y guiones is the correct one, but will check, habitaciones is really about where people live in.

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Jan 18, 2023

Ok @sketchydroide waiting for your confirmation, I have converted English to Spanish with google translate may be its incorrect. Plz also confirm English string I have picked it from #13676 (comment)

Room names can only include lowercase letters, numbers and hyphens

@sketchydroide
Copy link
Contributor

No worries, yeah, the translation from Google is not completly incorrect, it's one of those context changes translations things

@sketchydroide
Copy link
Contributor

ok got a confirmation to Los nombres de las salas solo pueden contener minúsculas, números y guiones for the Spanish

@jatinsonijs
Copy link
Contributor

Ok @sketchydroide English string suggested here #13676 (comment)

I'm going to add these. will submit PR within 1 hour

@jatinsonijs

This comment was marked as outdated.

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Jan 18, 2023
@jatinsonijs
Copy link
Contributor

PR is ready for review

cc @thesahindia

@mallenexpensify
Copy link
Contributor

Hired @jatinsonijs, thanks for the quick PR.
@thesahindia can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01773a5a2201587c50

@thesahindia
Copy link
Member

Accepted!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jan 23, 2023
@melvin-bot melvin-bot bot changed the title [$1000] New Room - Capital letters is converted to lowercase when all caps is enable [HOLD for payment 2023-01-30] [$1000] New Room - Capital letters is converted to lowercase when all caps is enable Jan 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 23, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.57-3 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 2023-01-30. 🎊

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

Sorry, something went wrong.

@melvin-bot
Copy link

melvin-bot bot commented Jan 23, 2023

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:

Sorry, something went wrong.

@thesahindia
Copy link
Member

We can leave the first 3 checkboxes since it wasn't a regression. It was implemented this way.

@thesahindia
Copy link
Member

@mallenexpensify, this is ready for payment.

@mallenexpensify
Copy link
Contributor

@jatinsonijs assigned on Jan 18
PR merged on Jan 19th

Paid @jatinsonijs and @thesahindia $1500 each which includes the timeliness bonus.

Posted buddy check for regression test steps
Recommendation: Update Room - Create Room Conversation (Online/Offline) in Testrail to add
6. Enter capital letters in the room name and hit return
7 Verify you're allowed to enter capital letters and that after return is tapped this error message shows "Room names can only include lowercase letters, numbers and hyphens"

@thesahindia
Copy link
Member

No action is needed from me so unassigning.

P.S. The steps looks good to me

@thesahindia thesahindia removed their assignment Feb 2, 2023
@mallenexpensify
Copy link
Contributor

Testrail GH created https://github.com/Expensify/Expensify/issues/260050

@thesahindia @sketchydroide @madmax330 can you review and take action on the top 3 in the checklist plz, then we can close #13676 (comment)

@thesahindia
Copy link
Member

As mentioned in #13676 (comment). I think we can leave those because it wasn't a regression.

@mallenexpensify
Copy link
Contributor

K, I checked them off then. @thesahindia are C+ able to check off boxes? Can't remember

@thesahindia
Copy link
Member

Nope, we can't edit.

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests