[HOLD for payment 2024-11-01] [$250] Editing comment with image attachment fails to load the image when cache expires

[m-natarajan]

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:
Reproducible in staging?:
Reproducible in production?:
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: @kidroca
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1715767781452439

Action Performed:

1. Type text and add an image attachment.
2. After sending the comment, inspect the tag for data-expensify attributes.
3. Edit the comment and make a trivial change to the text before the attachment.
4. Observe that the tag no longer has data-expensify attributes.
5. Disable cache in dev tools, or clear cache and reload the page.
6. Observe that the edited comment no longer loads the image.
   LINK: https://staging.new.expensify.com/r/5708510475809890

Expected Result:

Editing a comment, clearing cache, and reloading the page should display the comment and the image attachment.

Actual Result:

Clearing cache and reloading shows the comment and a placeholder box instead of the image attachment.
More details here: #41952 (comment)

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

330751938-58ed8062-5520-41f2-a971-dc291af9956f.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018082d3734c0fe3cf
• Upwork Job ID: 1792320613124546560
• Last Price Increase: 2024-06-10

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

Triggered auto assignment to @jliexpensify (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.

[m-natarajan]

@mallen tagging you as per this comment

[jliexpensify]

@m-natarajan you've tagged the wrong person. cc @mallenexpensify

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018082d3734c0fe3cf

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rushatgabhane (Internal)

[mallenexpensify]

Labeled internally and added to #vip-vsb cuz it doesn't have to do with money. All yours JLi

[melvin-bot]

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

[jliexpensify]

Not overdue, waiting on an Engineer to pick this up.

[melvin-bot]

@rushatgabhane @jliexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[jliexpensify]

Not overdue

[melvin-bot]

@rushatgabhane, @jliexpensify Huh... This is 4 days overdue. Who can take care of this?

[rushatgabhane]

internal. not overdue

[rushatgabhane]

🎀 👀 🎀 assign internal

[melvin-bot]

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

[amyevans]

Sorry, I don't have bandwidth to take this, unassigning!

[mallenexpensify]

I just did a quick-test in the latest staging build on Desktop and, after editing a comment, the image still shows! Think we need to to still leave this open for a retest?

[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 9.0.53-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Bump expensify-common to 2.0.100 #50990

If no regressions arise, payment will be issued on 2024-11-01. 🎊

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

• @rushatgabhane requires payment through NewDot Manual Requests
• @kidroca does not require payment (Contractor)

[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:

• [@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
• [@rushatgabhane] 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:
• [@rushatgabhane] 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:
• [@rushatgabhane] Determine if we should create a regression test for this bug.
• [@rushatgabhane] 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.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jliexpensify]

Payment Summary

• C+: @rushatgabhane $250 (owed via ND payments)

[rushatgabhane]

@jliexpensify that must be a mistake right? amount is 250

[jliexpensify]

Ha, yeah I fat fingered that one - thanks @rushatgabhane!

[truph01]

Hey @mountiny @rushatgabhane @jliexpensify The solution that ends up being implemented here is the same as the Solution 2 of my proposal (that was posted when there's still Help Wanted label). Should I be eligible for partial compensation here?

[garrettmknight]

$250 approved for @rushatgabhane

[mallenexpensify]

@truph01 can you provide more details plz? Ideally a bullet point list with links and time stamps? It'll make it easier to review for potential compensation. Thx

[truph01]

@mallenexpensify Sure, here's the timeline:

• The issue was made external on June 10
• I posted my proposal right after that on June 10, which has the Solution 2 of

Store the data-expensify- attributes in an object like a cache, with the image source as key, then after editing, apply the attributes again to the image html data. This way we'll still preserve the important metadata of data-expensify- in the image tags.

And some draft implementation details

• On June 14, There was a suggestion to fix in back-end by @kidroca

• @mountiny seems to agree with it and the issue is considered to be Internal and back-end fix was attempted from this point.

• On Sep 23, @mountiny assigned @mjasikowski to work on the issue

• On Sep 26, the PR was raised by @mjasikowski, which uses same front-end approach as my proposal. Besides that there's also this back-end fix

This PR adds caching for additional attributes to the tags when converted to and back from Markdown.

As such, I think a compensation of 50% to me makes sense, because the issue was made Internal for attempted back-end fix (I don't have access to the back-end repo so there's nothing I could do/help about it) but the front-end solution that ends up being used is same as suggested in my proposal. And this is a quite complex issue and it took me quite some time to work on the front-end solution in my proposal.

[mountiny]

No back-end fix is done.

backend fix was done, you just do not have access to it https://github.com/Expensify/Web-Expensify/pull/43729

[truph01]

@mountiny Was it needed to fix this issue? If yes I'd appreciate short explanation on why, as I don't have access to the back-end repo so can't see what the PR does.

From my testing with my initial proposal, only the front-end fix is enough.

[mountiny]

It was required as we were stripping the necessary tags in PHP layer too so not only the FE changes would be sufficient for all cases. @mjasikowski will be able to provide better explanation

[mjasikowski]

The tags were being stripped in the PHP layer. Caching attributes was already implemented in App for videos and that needed to be adjusted to apply to images as well, but the was only a small portion of the changes needed.

[truph01]

Thanks for your input, I've updated the timeline summary highlighting there's a back-end fix.

Looking forward to the final decision from @mountiny @mallenexpensify

Thanks in advance!

[mountiny]

Sorry @truph01, I don't think we are going to issue a reward for this one as there were many pieces required to fix this, and it was handled by @mjasikowski regardless of your proposal.

We appreciate your contributions and hope you will continue fixing issues in our repo ahead.