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.56k stars 2.9k forks source link

[$250] Onfido terms fonts in the verify identity page are not legit (Get Onfido to use our fonts) #44570

Closed m-natarajan closed 2 weeks ago

m-natarajan commented 4 months 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!


Version Number: 9.0.2.4 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 Expensify/Expensify Issue URL: Issue reported by: @twisterdotcom Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1719491934827009

Action Performed:

  1. Open app in dark mode
  2. Go to workspace
  3. Connect to a bank
  4. Enter the manual credentials
  5. Proceed to verify identity page

    Expected Result:

    The terms page should be easily legible

    Actual Result:

    The fonts and colours are difficult to read

    Workaround:

    unknown

    Platforms:

    Which of our officially supported platforms is this issue occurring on?

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [x] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

SCR-20240627-lswi Snip - (4) New Expensify - Google Chrome (2)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0162040e800bb2d883
  • Upwork Job ID: 1808367008542862770
  • Last Price Increase: 2024-07-10
Issue OwnerCurrent Issue Owner: @twisterdotcom
melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 4 months ago

@JmillsExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

JmillsExpensify commented 4 months ago

Opening up to the community.

melvin-bot[bot] commented 4 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0162040e800bb2d883

melvin-bot[bot] commented 4 months ago

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

sobitneupane commented 4 months ago

Waiting for proposal

MrMuzyk commented 4 months ago

I am Michał from Callstack - expert contributor group. I’d like to work on this job.

Can you provide some more info on expected result? How should it look like? At the moment we're providing set of styles in order to achieve this custom Onfido look so there is room to change some things

https://github.com/Expensify/App/blob/987c96511b7e05fec05cbe4a34dbd2674082f626/src/components/Onfido/BaseOnfidoWeb.tsx#L25-L66

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

sobitneupane commented 4 months ago

I believe we would want it to use fonts like existing pages in RHP. For example: Settings > Save the World > I am a teacher

Screenshot 2024-07-11 at 12 42 45
MrMuzyk commented 4 months ago

Ok, I'm looking for a cause for this weird look. We're passing custom fontStyles so in theory it should be working but for some reason it isn't.

MrMuzyk commented 4 months ago

What I've found so far is that few months ago we've upgraded to v14 of Onfido from v13 and this seems to cause the trouble.

I've reverted back to v13 locally to confirm that and passed font seems to be working fine.

onfidov13

Currently we're on version 14.15.0. I've tried upgrading it to the latest one (14.30.0) but the issue persists.

I'm beginning to think that this is an issue on the sdk side and we can't really do anything about it apart from changing it to one of the fonts they officially support that will resemble our own one (list on the bottom).

I will look into a bit more and try to find a workaround for this.

melvin-bot[bot] commented 4 months ago

@JmillsExpensify @sobitneupane this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

MrMuzyk commented 4 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Fonts are looking suspicious and do not match rest of the app on Onfido screen

What is the root cause of that problem?

There weren't any changes on our end when it comes to fonts used in Onfido iframe so after investigating that everything looks fine on our end I've started looking for the cause on the package side.

When onfido-sdk-ui package was upgraded to v14 the fonts stopped working. I've confirmed that by downgrading to v13 (see screenshot). After lowering version it simply started working again.

onfidov13

I'm suspecting that this is something that is broken on the Onfido package side so definitve root cause isn't known. What happens is that the font we're providing is not being detected so it defaults to some serif font causing this look reported in the issue. Why it is not being picked up - I don't know. There is no Issues tab in onfido-sdk-ui github repository so I can't even tell if this is something that other people are struggling with.

What changes do you think we should make in order to solve the problem?

Custom font provided by us via customUI prop when initializing Onfido is not being picked up by iframe so font defaults to some serif one. We can comment out (and leave a comment why is that) fontFamilyTitle, fontFamilySubtitle and fontFamilyBody styles inside customUI object so it uses default Onfido font which looks much nicer. In the meantime we can report the issue to Onfido and wait for them to fix it (if it's actually on them). Below screenshot how it looks with default Onfido font.

onfido-default-font

What alternative solutions did you explore? (Optional)

I've tried upgrading package from 14.15.0 (current version in app) to latest one (14.30.0) but the issue persists.

There is also an option to change the font to one of the google fonts they support. I've tried one of them and it worked. This is the list of them

sobitneupane commented 4 months ago

we can report the issue to Onfido and wait for them to fix it (if it's actually on them).

@MrMuzyk I believe this is the best approach. First, let's inform them about the issue. If they respond quickly and decide to fix it, we can wait.

MrMuzyk commented 4 months ago

First, let's inform them about the issue.

Should I do this?

Instructions are here on the bottom of the README.md - https://github.com/onfido/onfido-sdk-ui?tab=readme-ov-file#support

Unfortunately there is Issue github tab and it has to be done via email

sobitneupane commented 4 months ago

@MrMuzyk I will request internal engineer to confirm the next step.

This is the proposal from @MrMuzyk . The issue probably lies in onfido-sdk-ui repo. My suggestion is to first inform them about the issue.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 4 months ago

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

roryabraham commented 4 months ago

Agree we should first inform them about the issue, but frankly I haven't had any good experiences working with Onfido in the past. Hopefully a font/css issue is simpler though, particularly since this seems to be a regression.

(https://github.com/onfido/react-native-sdk/pull/115 has been open for more than a year without getting reviewed, despite the fact that we pay for Onfido and opened a support ticket asking for them to review it.)

In the meantime, we should start working on a patch for onfido-sdk-ui to fix the regression in v14, and submit an upstream PR with the fix.

MrMuzyk commented 4 months ago

Onfido-sdk-ui pakcage doesn't expose it's code publicly so I don't think we can fix it in our own.

roryabraham commented 4 months ago

thanks for clarifyin @MrMuzyk. Interesting that they have a public repo but not public source code...

so then I agree in the short-term that the font from this screenshot would be better than the one from the issue description. Let's make that change, then also create an issue in the repo.

MrMuzyk commented 4 months ago

I can change the font but unfortunately I can't create an issue in their repo - they have no Issues tab so it's not possible. The only reporting option that they have is to do it via email or their support - https://github.com/onfido/onfido-sdk-ui?tab=readme-ov-file#support

I believe it would be better if someone from Expensify team would report that via email.

roryabraham commented 4 months ago

ok, I can do that but it might have to be next week.

twisterdotcom commented 4 months ago

I will reach out to Onfido about this.

MrMuzyk commented 4 months ago

Awesome, I will open a PR shortly

twisterdotcom commented 4 months ago
image
roryabraham commented 4 months ago

Thanks @twisterdotcom 🙇🏼

sobitneupane commented 3 months ago

https://github.com/Expensify/App/pull/45587 deployed to production last week.

twisterdotcom commented 3 months ago

5 days ago: https://github.com/Expensify/App/pull/45587#issuecomment-2249280347

twisterdotcom commented 3 months ago

Payment Summary:

No regression test required here.

garrettmknight commented 2 months ago

$250 approved for @sobitneupane based on this summary.

shawnborton commented 2 months ago

Do we have any idea on when they would have a fix for this on their side? Just wondering if we have plans to follow up and undo our hotfix here when the time is right. Thanks!

twisterdotcom commented 2 months ago

Just looking back through my emails and found this actually from about a month ago - they cc'd contributors@ so it hit a filter in my inbox, apologies:

Starting with v14, the control of what fonts to use has moved to our side hence why you can’t use your own custom fonts. Could you please provide us with a list of the fonts that you require so that we can request our engineering team to add them.

I guess I could send them the names and font files and we could see if they can add them?

shawnborton commented 2 months ago

The problem would be that they are our own custom fonts - I'm not sure if they can host on their side or not. Spotnana does this for us for example though. Perhaps you can reach out and ask what the best path forward is for us given that we have custom fonts we want to use?

twisterdotcom commented 2 months ago

I have asked.

twisterdotcom commented 2 months ago

Hi Ted,

Could you please share the fonts so we can share them with our engineering team. We can guarantee that we support Google fonts but for other fonts we will need to confirm how to provide them for you.

Best Regards, Onfido Support

twisterdotcom commented 2 months ago

Just reopening for me to track. @shawnborton can you point me to the font files and I'll share them?

twisterdotcom commented 2 months ago

Not overdue, just reooened.

shawnborton commented 2 months ago

I would just tell them that we use custom .woff files for web and .otf files for the native apps and see what they say? Give that we're open source, you could show them this: https://github.com/Expensify/App/tree/main/assets/fonts

twisterdotcom commented 1 month ago
image

How much do you like our fonts?

twisterdotcom commented 1 month ago

Will leave this until @shawnborton gets back, but my take on this is that we probably don't care about paying $10k, or even negotiating to get our fonts used here when there are likely fonts they have available that are close enough available in google fonts: https://fonts.google.com/.

I'd say we should:

  1. Push back, confirming this was previously supported and then removed without notice
  2. If they still don't come down a lot (say to $500/$1000), we just agree to use another Google font instead of whatever Onfido's default is.
  3. Maybe look at the Onfido API again instead: https://docs.google.com/document/d/1UN_Hei2aWtsUiAh1WLAQOHtV3CWHqmSB1uzNkwHAm2E/edit#heading=h.ns12dr1f35rx (although the downsides were maintenance and the lack of videos).
roryabraham commented 1 month ago

Going on parental leave and gotta hand this off. It's not clear to me whether we'll need internal engineering help so not going to reassign for now

shawnborton commented 1 month ago

@twisterdotcom I think the path you laid out above sounds great to me.

twisterdotcom commented 1 month ago

Okay, waiting for them to come back:

image
twisterdotcom commented 2 weeks ago

Okay, they're gonna allow custom fonts again next year!

Hello Ted,

Hope you’re doing well. Apologies for the late response again and here’s what I got from my engineering and Product team:

Custom Fonts: We will re-introduce the mechanics to allow for URL-based selection and loading of fonts in v14 of our SDK:

We will do it as part of our standard dev process, and we’ll not ask for financial contribution We do not have a specific timeline for this but hope to address it before the end of the year. Anything sooner will require a formal CURE case(internal process to prioritise) as we will need to descope work. We will provide an overview of the mechanics to use the feature when we get closer.

React-Native Fabric: We are not able to commit to revamping our RN SDK as this work doesn’t align with Onfido current priorities. Onfido will be revisiting our React Native and Flutter SDKs later next year once our current core SDKs (iOS and Android) have been rewritten/ rebuilt.

I asked them to just let me know when and we can create an issue then.