Closed kavimuru closed 1 year ago
Triggered auto assignment to @stephanieelliott (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)Hey @kavimuru can you fill out the Platforms
section of the issue body please? Once you do, I can test!
It looks like the back button is from the <Onfido />
element obtained from the Onfido Web SDK.
This happens inside RequestorOnfidoStep.js
.
//...
<ScrollView contentContainerStyle={styles.flex1}>
<Onfido
sdkToken={this.props.onfidoToken}
onUserExit={() => {
BankAccounts.clearOnfidoToken();
BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.REQUESTOR);
}}
onError={() => {
// In case of any unexpected error we log it to the server, show a growl, and return the user back to the requestor step so they can try again.
Growl.error(this.props.translate('onfidoStep.genericError'), 10000);
BankAccounts.clearOnfidoToken();
BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.REQUESTOR);
}}
onSuccess={(onfidoData) => {
this.submit(onfidoData);
}}
/>
</ScrollView>
//...
The Web SDK Flow has a configuration option called forceCrossDevice
that has been set to true
which then shows the above-mentioned <Onfido />
element prompting the user to continue on mobile after selecting a document.
Onfido offers some UI customization options which have already been tweaked to match Expensify's design inside BaseOnfidoWeb.js
- this is also where the config options are set - however, I don't see any option to change the back button's case from their docs.
The customization options for the document screen in question are limited to colors, fonts, and maybe border radii.
The only string
that appears to be customizable is that of the optional welcome screen which I believe Expensify isn't currently using on Web.
I'm not sure if this counts as a proposal per se, but I think changing the back button's case is an option Onfido hasn't provided. If it's accepted as one, then well and good - just thought I'd offer some insight from my findings.
By the way, @kavimuru, did you mean title case instead of upper case for the back button?
back
- lowercaseBack
- title caseBACK
- uppercaseπ£ @Victor-Nyagudi! π£
Hey, it seems we donβt have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Contributor details Your Expensify account email: vicktests4@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0136b5e59ffbb32395
β Contributor details stored successfully. Thank you for contributing to Expensify!
@stephanieelliott updated the platform section.
@stephanieelliott Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Hm, interesting proposal @Victor-Nyagudi! Adding the External
label so we can get more eyes on it.
Job added to Upwork: https://www.upwork.com/jobs/~014397c5d33abcb652
Current assignee @stephanieelliott is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External
)
Back tooltip on personal information is in lowercase
This is coming from Onfido ( en & es )
We can either ignore it or raise an issue on https://github.com/onfido/onfido-sdk-ui.
Triggered auto assignment to @neil-marcellini (External
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
In this issue, we can notice that the tooltip/label for the Onfido back button is set to 'back'. In Expensify, we use Sentence case whenever possible and this causes an inconsistency in the app.
This is default behaviour of Onfido. I doubt that reaching out to the Onfido SDK maintainers would have an effect since this is an inconsistency problem for Expensify and the change of 'back' to 'Back' wouldn't be a priority to them.
In order to solve this issue, we can use the localisation customisation ability to Onfido. This will let us declare custom phrases that can be used across the whole Onfido flow and we can customize everything we want here. I think this can be a major undertaking involving the Copy team as well for both English and Spanish and doing a textual personalisation of the Onfido flow.
In order to customise locales, different techniques need to be applied for Web, ios and Android. The links to the respective documentation are:
Each of these implementation is a bit complicated but I'm pretty sure I can do this. As for the phrases, we will create our own versions of them for both, spanish and english and depending on the user's chosen language, pass it to the Onfido SDK here.
For this specific issue, we will need to change the Onfido phrase - generic.back
I was able to put together a quick POC for the Web, which is the issue here:
Reviewing now...
Thank you guys for the proposals!
As @Victor-Nyagudi and @aman-atg point out, that is a default from the Onfido SDK, and asking the Onfido maintainer to change the copy will not be a priority for them.
@Prince-Mendiratta The documentation links for iOS and Android you attach is pointing to the web SDK. Do you mean we need to do this https://github.com/onfido/react-native-sdk#android https://github.com/onfido/react-native-sdk#ios for the native to add custom localization?
@mollfpr that's weird, I swear I think I added the right links π
here it is:
@Prince-Mendiratta So, we will update the iOS and Android with the Onfido RN SDK, right? As the documentation is pretty clear about custom the localization on native platform, it will do for you to work on it?
@mollfpr Actually we need not do anything for iOS since the "back" tooltip is not shown there at all.
We will need to do for Android though since I can see the tooltip on long press on the back button. We need to add strings for the following:
I'm unable to get the tooltips on mWeb no neither device.
Thanks @Prince-Mendiratta, that make sense to me!
Let's go with @Prince-Mendiratta proposal where they assure it can be done within our app.
For @Victor-Nyagudi and @aman-atg thank your proposal and insight about this issue. I'll be happy to review your proposals on other issues. π
π π π C+ reviewed!
I also like @Prince-Mendiratta's proposal. Since the issue is only on Web and Desktop, we only need to implement the fix there. Let's keep it simple for now and only fix the capitalization of the back button. If you like, you could make a feature request for us to localize the entire Onfido flow, but I'm not sure it's a priority.
π£ @Prince-Mendiratta You have been assigned to this job by @neil-marcellini! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
PR is on staging
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.3-2 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 2023-04-28. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
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:
PR of this issue caused https://github.com/Expensify/App/issues/17789 regression.
@Prince-Mendiratta would you please submit a PR to fix the regression?
https://github.com/Expensify/App/pull/17943#issue-1682183369 is still undergoing QA, "waiting for payment" period will restart once it is merged.
PR on staging
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.12-0 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 2023-05-16. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
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:
Sharing the timeline of this issue to help with the eventual analysis, calculated with this tool.
π Issue created at: Mon, 10 Apr 2023 21:24:56 GMT π§― Help Wanted at: Fri, 14 Apr 2023 19:37:50 GMT π΅π» Contributor assigned at: Tue, 18 Apr 2023 16:50:24 GMT πΈ PR created at: Tue, 18 Apr 2023 17:07:37 GMT π― PR merged at: Thu, 20 Apr 2023 20:39:34 GMT π± Regression PR merged at: Fri, 28 Apr 2023 19:30:11 GMT π° Timeline Bonus: No bonus applicable.
[@mollfpr] The PR that introduced the bug has been identified. Link to the PR: [@mollfpr] 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: [@mollfpr] 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:
Another improvement to Onfido languages, so no offending PR.
[@mollfpr] Determine if we should create a regression test for this bug.
Maybe we can add the check to Workspace - Add bank account - Plaid back account selector screen?
Propose update for Workspace - Add bank account - Plaid back account selector screen TC added a step 14-18
Hey @Prince-Mendiratta @mollfpr @ayazhussain79 can you please apply for the job in Upwork so that we can pay this out? Thank you!
Hiya @stephanieelliott! Already applied here when I put in the PR, I'm not able to reapply as it says job closed.
Jumping in on behalf of Steph.
New job post - https://www.upwork.com/jobs/~012bae1e57414f2d48
I sent offers to all 3 just now.
Checklist complete and QA test posted in E/E repository https://github.com/Expensify/Expensify/issues/286071
Are you still looking for someone to fix this? If yes, I can land a PR in 24 hours.
π£ @MrJithil! π£ Hey, it seems we donβt have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
@michaelhaxhiu Offer accepted
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:
Expected Result:
Back tooltip on personal information should be in uppercase
Actual Result:
Back tooltip on personal information is in lowercase
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.98-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: 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/43996225/231001912-441937ef-3ed5-40fb-aa2c-37ed5a2a7a6a.mp4
Expensify/Expensify Issue URL: Issue reported by: @ayazhussain79 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681146014939409
View all open jobs on GitHub
Upwork Automation - Do Not Edit