[HOLD for payment 2024-01-05] [$500] iOS - Attachment - User able to switch to another attachment in PDF zoomed state

[lanitochka17]

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: 1.4.17.1
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. Open app
2. Open any existing coversation
3. Send an image and PDF attachment in the conversation
4. Open the uploaded PDF attachment (you can access the PDF attachment via arrow navigation between attachments. Open image attachment and tap on left or right arrow to navigate on PDF view)
5. Zoom in the PDF page
6. Tap on the PDF page
7. Try to slide left/right while the PDF is zoomed in

Expected Result:

According step 10 in the TR, user should not be able to switch to another attachment in PDF zoomed state

Actual Result:

User able to switch to another attachment in PDF zoomed state

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

Bug6326452_1703628723874.Rpreplay_Final1703628654.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017d96c87f2683da02
• Upwork Job ID: 1739779120396140544
• Last Price Increase: 2023-12-26
• Automatic offers:
• mollfpr | Reviewer | 28070990
• tienifr | Contributor | 28070991

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017d96c87f2683da02

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (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 - @mollfpr (External)

[abzokhattab]

Proposal

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

User is able to click on previous or next attachement in zoomed state in IOS

What is the root cause of that problem?

while rendering the attachement item we dont disable the previous/next attachements arrows on zooming in here:

App/src/components/Attachments/AttachmentCarousel/index.native.js

Lines 98 to 106 in 9208c95

	({item, isActive}) => (
	<CarouselItem
	item={item}
	isFocused={isActive && activeSource === item.source}
	onPress={() => setShouldShowArrows(!shouldShowArrows)}
	/>
	),
	[activeSource, setShouldShowArrows, shouldShowArrows],
	);

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

introduce a new state onScaleChanged in the component above and then we can check if the zoom is 1 then we can show the buttons else we need to hide them.
Also inside the CarouselItem we need to add the new onScaleChanged prop and it will be used here: as

onScaleChanged={onScaleChanged}

the onScaleChanged prop that we added will be triggered on changing the scale of the attachement.

[mollfpr]

@abzokhattab Wasn't the issue that the user could slide to the prev/next attachment while on Zoom? It does not slide to the/next attachment while zooming in on the picture attachment.

[tienifr]

@mollfpr this proposal fixes the root cause, would you mind taking a look, thank!

Proposal

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

User able to switch to another attachment in PDF zoomed state

What is the root cause of that problem?

We're not passing isUsedInCarousel into AttachmentViewPdf here. So the logic to reset the pinch gesture here does not run, leading to the arrows still show although we don't want it to show if there's a pinch gesture (zoom) as per this logic.

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

Pass isUsedInCarousel into AttachmentViewPdf here (we already do that for AttachmentViewImage here)

isUsedInCarousel={isUsedInCarousel}

What alternative solutions did you explore? (Optional)

Out of scope of this issue but I notice currently it look like for mWeb we're not allowing users to zoom the attachment, if it' s not expected we might want to create another GH to fix it.

[mollfpr]

The proposal from @tienifr looks good! I think this issue was a regression from #31308 when it's removed isUsedInCarousel from the AttachmentViewPdf.

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

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

[melvin-bot]

📣 @mollfpr 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[chrispader]

This PR indeed fixes the problem as described in #31308 (comment)

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

• fix: User able to switch to another attachment in PDF zoomed state #33695

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

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

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

• @mollfpr requires payment automatic offer (Reviewer)
• @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:

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

[bfitzexpensify]

Payment made to @tienifr. @mollfpr, it looks like we're waiting for you to accept the contract in Upwork. Also, a reminder to complete the BZ checklist - thanks!

[bfitzexpensify]

Bump on contract acceptance in Upwork & BZ checklist @mollfpr

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#31308

[@mollfpr] 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:

https://github.com/Expensify/App/pull/31308/files#r1436916636

[@mollfpr] 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:

The regression step should be good.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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.

1. Open the app
2. Open any existing conversation
3. Send an image and PDF attachment in the conversation
4. Open the uploaded PDF attachment
5. Open the image attachment and tap on the left or right arrow to navigate to the PDF view
6. Zoom in the PDF page
7. Tap on the PDF page
8. Try to slide left/right while the PDF is zoomed in
9. Verify that the user is not able to switch to another attachment in the PDF zoomed state

---

@bfitzexpensify Could you give the payment summary for the manual request? Thanks!

[bfitzexpensify]

Payment summary:

Contributor: @tienifr paid $500 via Upwork ✅
C+: @mollfpr to be paid $500 via manual request

[bfitzexpensify]

Added regression tests to https://github.com/Expensify/Expensify/issues/358238, closing this out.

[JmillsExpensify]

$500 payment approved for @mollfpr based on comment above.