search

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.86k stars 2.94k forks source link

[HOLD #50349] Chat - Opened offline image preview switches to previously uploaded image online #50296

Closed lanitochka17 closed 2 days ago

lanitochka17 commented 2 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.45-2 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/49832

Action Performed:

  1. Open a chat
  2. Go offline
  3. Upload an image
  4. Open the image
  5. Go online
  6. Verify that image preview modal not dismissed

Expected Result:

Image preview modal is not dismissed after going online, preview of the uploaded offline image is opened

Actual Result:

Opened preview of the uploaded offline image switches to the preview of previously uploaded image after going online

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/abcb7911-5a58-4b0f-b770-963ac424beac

View all open jobs on GitHub

github-actions[bot] commented 2 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @AndrewGable (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

lanitochka17 commented 2 months ago

Production:

https://github.com/user-attachments/assets/fe59703d-ff39-438c-a5d3-0a3310e85759

thienlnam commented 2 months ago

This was the note on the linked PR

There is a known issue in the native platform where the attachment index incorrectly switches even after setting it correctly https://github.com/Expensify/App/pull/49832#issuecomment-2384722144. This is a known upstream issue that should be addressed separately https://github.com/Expensify/App/pull/49832#issuecomment-2385872261.

thienlnam commented 2 months ago

cc @s77rt @wildan-m @rlinoz If I'm reading that comment correctly - are we saying that this current bug was broken elsewhere and needs to fixed upstream? Because this is not reproducible on production so it seems like somewhere along the line it was changed

AndrewGable commented 2 months ago

Thanks for noticing that @thienlnam - Do we have a follow up issue we can link here or can we just use this as the issue?

rlinoz commented 2 months ago

Yeah, it is not reproducible in prod right now because when going online again in prod the modal is dismissed (that is what the PR is fixing)

This are the related upstream issues we found: https://github.com/callstack/react-native-pager-view/issues/791 https://github.com/callstack/react-native-pager-view/issues/597

I don't think we have an issue to track that on our side yet

s77rt commented 2 months ago

@thienlnam In production I believe the behaviour was that after you get online the modal will be dismissed which is already considered a bug. Now the modal is not dismissed but the attachment changes. Since this was not working at all it is not a regression. The new bug on native is upstream and we should fix it as well however this does not need to be a deploy blocker.

thienlnam commented 2 months ago

Ahh okay I see, thanks for the clarification - yeah let's create an issue to track that on our side and then we can solve it seperately

thienlnam commented 2 months ago

Created the issue here: https://github.com/Expensify/App/issues/50349

AndrewGable commented 2 months ago

Do we need to keep this one open @thienlnam ?

thienlnam commented 2 months ago

I imagine this one might get re-reported so was thinking it could be good to leave it open and on hold for the other issue - but feel free to close if you want

mallenexpensify commented 1 month ago

We're holding on creating a regression test to this linked issue, so we'll likely want one here.

AndrewGable commented 1 month ago

I'm a bit confused on the next steps here, seems like we are waiting on making a regression test, but we are waiting on this issue to create the regression test?

s77rt commented 1 month ago

@AndrewGable Making a regression test now won't make much sense because there is still an ongoing bug and the QA team won't be able to complete the test. Once https://github.com/Expensify/App/issues/50349 is fixed we can proceed

AndrewGable commented 1 month ago

Still on hold

AndrewGable commented 1 month ago

Seems like the issue on hold is getting close

AndrewGable commented 3 weeks ago

Deployed to staging, let's wait a little bit longer

AndrewGable commented 5 days ago

@s77rt @thienlnam - What's next steps here?

s77rt commented 5 days ago

@AndrewGable Let's add the following regression test and close the issue:

  1. Open any chat
  2. Go offline
  3. Upload an image
  4. Open the image
  5. Go online
  6. Verify the image is still displayed and that the modal is not dismissed
mallenexpensify commented 2 days ago

Thanks @s77rt test case created, gonna close