[HOLD for payment 2023-07-17] [HOLD for payment 2023-07-12] [$1000] Attachment Carousel closes on its own when a specific image is deleted

[kavimuru]

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:

1. Have a chat with 2 users (User A, User B)
2. Have a few attachments posted by User B (Attachment 1, 2, 3)
3. Make User A press and open the latest attachment (Attachment 3)
4. Make User B delete Attachment 3

Expected result:

Nothing disruptive should happen to User A’s experience. They continue to see that attachment until they close the window

Actual result:

The whole attachment modal/carousel abruptly closes (even if they scrolled to another image)

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.10
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/236339219-067cdc95-1b40-4b29-9d3b-4da0cbbf4650.mov

Recording.492.mp4

Expensify/Expensify Issue URL:
Issue reported by: @kidroca
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1683212386358629

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01afedd756887cbd8d
• Upwork Job ID: 1655921750675152896
• Last Price Increase: 2023-05-09

[melvin-bot]

Triggered auto assignment to @flaviadefaria (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]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01afedd756887cbd8d

[melvin-bot]

Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[flaviadefaria]

Added the external label!

[dukenv0307]

Proposal

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

The whole attachment modal/carousel abruptly closes (even if they scrolled to another image) when user A is viewing an image and user B deletes the message that contains the image.

What is the root cause of that problem?

Here

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

Line 61 in c281e7d

<AttachmentModal

the AttachmentModal is contained inside ImageRenderer so it will be deleted along side the report item when the report item is deleted.

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

We need to move AttachmentModal modal outside of the report item, for example to put it outside ReportActionsList here

App/src/pages/home/report/ReportActionsView.js

Line 351 in c281e7d

<ReportActionsList

, and then we can provide the show method along with a setter function for the props of AttachmentModal (reportID, source, isAuthTokenRequired, originalFileName) to the ImageRenderer via a context.

Inside ImageRenderer we'll retrieve the methods from the context and:

• Set the props of the AttachmentModal to refer to the correct attachment
• Call show on the AttachmentModal to display the attachment

This approach will make sure that the AttachmentModal is independent from the report item and won't be removed when the message is deleted.

What alternative solutions did you explore? (Optional)

NA

[NikkiWines]

@rushatgabhane friendly bump here to review the above proposal ^^

[rushatgabhane]

woah this is kind of a big change. reviewing today

[rushatgabhane]

@dukenv0307 that logically makes sense but again it's a big change. We'll have to really careful while testing.
@NikkiWines I think we can give their proposal a try.

🎀 👀 🎀 C+ reviewed

[NikkiWines]

Hmmm, for a change like this, I think it might be better to get some more thoughts on the behavior we want. I just tested this with @alex-mechler on Whatsapp and saw the following after I sent two attachments.

1. User is not viewing the image that was deleted

No change in behavior, user still sees the image they were on

2. User is viewing the image that was deleted

User is scrolled to the previous attachment.

I think we should probably follow that model and not continue to show the attachment as the "Expected Result" section of the OP shows. Gonna bring this to #expensify-open-source in slack for more discussion (discussion here)

[kidroca]

I agree that AttachmentModal should be removed from the ImageRenderer, though I would prefer an even more elegant approach than trying to use Context

An approach that would allow deep linking to an attachment (I don't think App is capable of this basic feature right now)

The idea is to create a new path under the report route for attachments, something like: /r/{reportID}/attachments/{attachmentID}

This way thumbnails rendered by the ImageRenderer would not render the modal, but a link (activated on press) to the appropriate path, which would open the Carousel with the particular attachment in place

[flaviadefaria]

I'm OoO for 8 days so re-adding the BUG label, but keeping myself assigned. I'll be back at work on May 30th.

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

• Report Attachments Modal Route #20167

If no regressions arise, payment will be issued on 2023-07-12. 🎊

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.

• 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

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

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

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @kidroca got assigned: 2023-06-03 00:18:28 Z
• when the PR got merged: 2023-07-07 08:52:17 UTC
• days elapsed: 24

On to the next one 🚀

[melvin-bot]

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

• Improve Attachment Carousel State Management in AttachmentModal and ReportAttachments #22179

If no regressions arise, payment will be issued on 2023-07-17. 🎊

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.

• 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

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

• [@chiragsalian / @NikkiWines] The PR that introduced the bug has been identified. Link to the PR:
  • In this case the AttachmentModal has always lived within the ImageRenderer.js since we added it here - there isn't really anything we could've done to prevent this as it's been the case since this functionality was added.
• [@chiragsalian / @NikkiWines] 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: N/A see above
• [@chiragsalian / @NikkiWines] 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: N/A see above
• [@kidroca] Determine if we should create a regression test for this bug.
• [@kidroca] 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.
• [@flaviadefaria] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[flaviadefaria]

@kidroca please fill out the checklist above so that i can process the payment - thanks!

[kidroca]

Thanks for the bump @flaviadefaria

I don't think we need a regression test, because the component structure and usage changed in a way that prevents this kind of bug from happening again, furthermore it's rare, it's not super easy to test and it does not affect important functionality

[flaviadefaria]

Ok this is what I think needs to be issued as payment:

Merged PR more than 9 business days after assignment - 50% penalty

@kidroca = $1000 - 50% penalty = $500
@allroundexperts = $1000 - 50% penalty = $500

It seems fair to me to remove the penalty due to the initial C+ getting sick so will issue payment for the full $1000 for each of you.

@allroundexperts do you request payment via Expensify or should I pay you via UpWork?

[flaviadefaria]

@kidroca I just sent you an offer. I also just realized you reported this bug so will issue a bonus of $250 to account for that once you accept the offer.

[allroundexperts]

@allroundexperts do you request payment via Expensify or should I pay you via UpWork?

Expensify works for me. Thanks @flaviadefaria!

[anmurali]

@allroundexperts is paid

[flaviadefaria]

@kidroca you've also been paid. Closing this as complete.