[HOLD for payment 2024-02-09] [$500] Android - Workspace - WS image is displayed without background

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.18-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Tap on a Workspace ( with single letter name like eg:"Z")
3. Tap Workspace icon in header
4. Tap Workspace icon in Details page
5. Note Workspace image is displayed with background in mweb.
6. Launch app
7. Repeat step 2 - step 4
8. Note Workspace image is displayed without background in Android

Expected Result:

Workspace image must be displayed with background

Actual Result:

Workspace image is displayed without background in Android but with background in mweb

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6327135_1703716298402.az_recorder_20231228_001317.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010b6d3a83f995ac38
• Upwork Job ID: 1740172374538711040
• Last Price Increase: 2024-01-04
• Automatic offers:
• tienifr | Contributor | 28091060

Issue Owner

Current Issue Owner: @MitchExpensify

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010b6d3a83f995ac38

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[tienifr]

Proposal

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

Workspace image is displayed without background in Android but with background in mweb

What is the root cause of that problem?

Since the SVG migration to using expo-image, it no longer match this condition, because SVG is no longer React components, it's now statically imported

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

Check if source is number here instead, because the statically imported SVG will have source as number, it's native iOS & Android's static asset representation.

if (typeof source === 'number' && !transactionID) {

The transactionID check is so that we exclude receipt case, since generic receipt is an image and it should never display as Icon. An alternative of this is that we can move all the checks related to receipt like here and here to above the source number check. But I prefer the transactionID check since it's more robust.

What alternative solutions did you explore? (Optional)

Since source number could be images as well, there could be risk of the image showing as icon if that happens (although currently I didn't find any such case in the app except for the generic receipt above). If we want to avoid that risk entirely, we can add another prop like isImage to indicate the file type (or add type to the file), and the condition of source above will take into account the file type as well, so only if the source is number, and it is CSV (or it's not image), we'll use the <Icon>.

[shubham1206agra]

@NicMendonca @sobitneupane Can we combine this GH with #33616?

As #33616 (comment) seems to solve both issues.

[tienifr]

@shubham1206agra they have 2 different root causes, in #33616 the solution that's accepted won't fix this issue.

But we can hold this issue on that one though.

cc @NicMendonca @sobitneupane

[shubham1206agra]

Nope both have the same root cause

And the accepted solution is partially wrong

[shubham1206agra]

Proposal

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

Android - Workspace - WS image is displayed without background

What is the root cause of that problem?

This is stemming from #30905, as we changed the ImageRenderer Engine from FastImage to Expo Image, and so the properties https://github.com/Expensify/App/pull/30905/files?short_path=b5e8f8f#r1436919622

If we use default workspace avatar, the background color will be missing in this case. This is happening due to we are using AttachmentViewImage component to render this instead of Icon component.

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

There are different solutions depending on the ideal behaviour

1. If we want to double tap zoom / pinch behaviour on native devices (which is doubtful given this is not present on web/mWeb), then we need to pass fill color for workspace icons to the AttachmentViewImage, and then to Image component also.

2. If we do not want to double tap zoom / pinch behaviour on native devices (which is likely behaviour), then we can pass a prop through AttachmentModal to know the image is avatar and render that through Icon component instead of AttachmentViewImage for the default avatars.

Test branch - https://github.com/shubham1206agra/App/tree/test-avatar-d

What alternative solutions did you explore? (Optional)

[shubham1206agra]

I am just reposting the proposal here from #33616 (comment) as other proposal might be accepted there.

@NicMendonca Since this GH did not exist when I posted my proposal on the other GH pointing out this problem, can you see about the order on which proposal to review in this case?

[melvin-bot]

@NicMendonca, @sobitneupane Eep! 4 days overdue now. Issues have feelings too...

[NicMendonca]

can you see about the order on which proposal to review in this case?

Sorry! I don't understand. Any proposals should be reviewed by @sobitneupane

[sobitneupane]

This issue won't be resolved by the PR in associated issue. I will review the proposals by tomorrow EOD (NPT).

[melvin-bot]

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

[sobitneupane]

Thanks for the proposal @tienifr and @shubham1206agra .

Alternative proposal from @tienifr looks good which is similar to the one proposed by @shubham1206agra.

Proposal from @tienifr comes before @shubham1206agra in this issue. But @shubham1206agra had proposed the solution earlier in another issue which is linked here.

I am not sure if we have any provision of referencing proposal from another issue. So, I will need help from CME to decide which proposal to go with.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[tienifr]

I believe the main GH issue should be the only reference point according to the guidelines. But I think the most fair here would be to share the reward since both of us happen to come up with same solution, just in 2 parallel issues. @shubham1206agra what do you think? 😁

[melvin-bot]

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

[tienifr]

PR ready for review #34486.

[melvin-bot]

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

[NicMendonca]

Starting leave so re-assigning!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Fix: SVG icons do not show on native #34486

If no regressions arise, payment will be issued on 2024-02-09. 🎊

For reference, here are some details about the assignees on this issue:

• @sobitneupane requires payment through NewDot Manual Requests
• @tienifr requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane] 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:
• [@sobitneupane] 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:
• [@sobitneupane] Determine if we should create a regression test for this bug.
• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MitchExpensify]

Reminder set to pay

[MitchExpensify]

Payment summary:

$500 @sobitneupane requires payment through NewDot Manual Requests
$500 @tienifr requires payment automatic offer (Contributor) PAID

[JmillsExpensify]

$500 approved for @sobitneupane based on summary.