Report Attachments Modal Route #20167

kidroca · 2023-06-05T13:38:28Z

Details

In this PR, I have made several enhancements to the existing code to improve the handling of report attachments. Key changes include the addition of a new modal screen src/pages/home/report/ReportAttachments.js, modifying AttachmentModal to make the passing of children optional, and the addition of a defaultOpen prop to AttachmentModal. A new route, REPORT_ATTACHMENT, has also been introduced.

Changes:

src/ROUTES.js:
A new route named REPORT_ATTACHMENT has been added. It is structured as r/:reportID/attachment?source=... and has an associated getter function for generating this URL with the required parameters.
src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js:
The AttachmentModal component usage has been replaced with navigation to the new REPORT_ATTACHMENT route.
src/libs/Navigation/AppNavigator/AuthScreens.js:
The new Report_Attachments screen has been registered in the application's authenticated screens.
src/libs/Navigation/linkingConfig.js:
The Report_Attachments screen has been added to the linking configuration, enabling it to be opened via URL schemes or deep linking.
src/components/AttachmentCarousel/index.js:
Use onNavigate to update the parent modal's state with additional information about the selected attachment - file name and auth requirements
src/components/AttachmentModal.js:
The children prop is now optional, and a new defaultOpen prop has been added, which determines whether the modal should be open by default.
src/pages/home/report/ReportAttachments.js:
This new file creates the ReportAttachments component, which returns an AttachmentModal that's set to defaultOpen. It takes a reportID and source as parameters from the route, and on modal hide, it dismisses the modal.

Fixed Issues

$ #18451
PROPOSAL: #18451 (comment)

Tests

Test 1: Verify Existing Functionality is Unaltered

Objective: To ensure that the implemented changes haven't affected the existing functionality.

Navigate to a chat with multiple attachments.
Open different types of attachments (e.g., images, documents), and verify that they are correctly displayed in the carousel.
Download a variety of attachments and check that the downloading functionality remains intact.

Expected Outcomes:

Each attachment should open and display correctly when selected from the chat.
Navigation between attachments in the carousel should be smooth, with attachments appearing in the correct order.
All downloads should proceed as before, with no new issues introduced.

Note: This PR does not address existing known issues such as the carousel occasionally displaying the wrong image (as detailed in #18150 (comment)).

Test 2: Verify the issue with deleting an image is resolved

Objective: To ensure that the main issue with deleting attachments is resolved

Have a chat with 2 users: User A and User B.
Share a few attachments between them. Share one last attachment with User B.
With User A, open the last attachment User B shared.
With User B, delete the chat message which shared the last attachment (long press or 2nd mouse click on the chat message and select "Delete Comment").

Expected Outcomes:

From the point of view of User A, nothing happens, they are still inside the carousel and can see the deleted image until they close the carousel.
From the point of view of User B, the image is deleted.
When User A exits the carousel / attachment view, the image is no longer available in the chat and they can't open it again (it no longer appears if they open the carousel again).

Offline tests

N/A

Opening an attachment while offline should have the same behavior as it did before - the attachment appears to load indefinitely. We can close the attachment view and return to the chat.

QA Steps

Test 1: Verify Existing Functionality is Unaltered

Objective: To ensure that the implemented changes haven't affected the existing functionality.

Navigate to a chat with multiple attachments.
Open different types of attachments (e.g., images, documents), and verify that they are correctly displayed in the carousel.
Download a variety of attachments and check that the downloading functionality remains intact.

Expected Outcomes:

Each attachment should open and display correctly when selected from the chat.
Navigation between attachments in the carousel should be smooth, with attachments appearing in the correct order.
All downloads should proceed as before, with no new issues introduced.

Note: This PR does not address existing known issues such as the carousel occasionally displaying the wrong image (as detailed in #18150 (comment)).

Test 2: Verify the issue with deleting an image is resolved

Objective: To ensure that the main issue with deleting attachments is resolved

Have a chat with 2 users: User A and User B.
Share a few attachments between them. Share one last attachment with User B.
With User A, open the last attachment User B shared.
With User B, delete the chat message which shared the last attachment (long press or 2nd mouse click on the chat message and select "Delete Comment").

Expected Outcomes:

From the point of view of User A, nothing happens, they are still inside the carousel and can see the deleted image until they close the carousel.
From the point of view of User B, the image is deleted.
When User A exits the carousel / attachment view, the image is no longer available in the chat and they can't open it again (it no longer appears if they open the carousel again).

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-06-06.at.21.23.53.mov

Mobile Web - Chrome

2023-06-21_22-06-18.mp4

Mobile Web - Safari

Screen.Recording.2023-06-06.at.21.27.54.mov

Desktop

Screen.Recording.2023-06-06.at.21.27.54.mov

iOS

Screen.Recording.2023-06-06.at.21.23.53.mov

Android

2023-06-21_22-10-18.mp4

kidroca

This PR is ready for code review

~~I'll be able to continue and add testing steps and check all platforms tomorrow~~ ✅

kidroca · 2023-06-05T13:45:17Z

src/ROUTES.js

+    REPORT_ATTACHMENT: 'r/:reportID/attachment',
+    getReportAttachmentRoute: (reportID, source) => `r/${reportID}/attachment?source=${encodeURI(source)}`,


I think I can remove the changes here and in linking which would disable deep linking, but also prevent me from using the global Navigation object in ImageRenderer (I'll post a similar note there)

Right now if we refresh the page or copy paste the URL while we have the attachment carousel open, it would load the page with the attachment carousel open

The question is, do we want to deliberately not allow this? It can be used as a feature linking to specific attachments

src/components/AttachmentCarousel/index.js

kidroca · 2023-06-05T13:54:27Z

src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

+                        const route = ROUTES.getReportAttachmentRoute(report.reportID, source);
+                        Navigation.navigate(route);


This can be replaced by a call to useNavigation and then navigation.navigate('Report_Attachment', {.reportID, source })

Everywhere else we use Navigation.navigate and ROUTES so I've opted for the same approach here, but this allows deep linking and link sharing and this feature might be unwanted

melvin-bot · 2023-06-05T13:58:37Z

@NikkiWines @rushatgabhane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

The Carousel already updates the parent component this way, when attachments change Updating it here allows the parent component to not need `isAuthenticated` and `originalFileName` for the initial image source provided, since they are obtained here as well

rushatgabhane · 2023-06-11T21:53:24Z

@kidroca @NikkiWines sorry i was out sick and couldn't review the PR in time.
Someone else from the team should take over, thank you!

allroundexperts · 2023-06-11T21:57:06Z

@NikkiWines I'll be taking this over. Please assign this to me.

allroundexperts · 2023-06-12T15:11:04Z

@kidroca Can you please resolve conflicts?

kidroca · 2023-06-13T16:03:48Z

Yes, I'll look into it tonight

…vigator`

Otherwise, when closing the modal there's a blank screen displayed briefly It seems the previous screen does not stay underneath the modal unless it's on the root

kidroca

Please be aware that a significant overhaul to the navigation structure in the main branch, introduced some merge conflicts. I've managed to resolve these conflicts, but in the process, I found it necessary to implement some additional changes to ensure seamless integration and functionality.

All merge conflicts have been successfully resolved ✅

kidroca · 2023-06-14T13:34:39Z

src/libs/Navigation/AppNavigator/AuthScreens.js

+                <RootStack.Screen
+                    name={SCREENS.REPORT_ATTACHMENTS}
+                    options={{
+                        headerShown: false,
+                        presentation: 'transparentModal',
+                    }}
+                    getComponent={() => {
+                        const ReportAttachments = require('../../../pages/home/report/ReportAttachments').default;
+                        return ReportAttachments;
+                    }}
+                    listeners={modalScreenListeners}
+                />


Utilizing RootStack.Screen proved to be the only viable solution for overlaying the modal over the opened chat. I tested alternative navigators such as CENTRAL_PANE_NAVIGATOR, FULL_SCREEN_NAVIGATOR, and RIGHT_MODAL_NAVIGATOR. However, these alternatives resulted in a blank screen beneath the modal. This undesired outcome was visible for at least half a second during the modal's closing transition.

Sample Behavior When Not Using RootStack

Please refer to the following link for a demonstration of this behavior without the use of RootStack:

Android.Emulator.-.Pixel_2_API_29_5554.2023-06-14.16-28-43.mp4

Sample Behavior When Using RootStack

For comparison, the following link shows the preferred outcome achieved when utilizing RootStack:

Android.Emulator.-.Pixel_2_API_29_5554.2023-06-14.16-31-27.mp4

kidroca · 2023-06-14T13:37:24Z

src/libs/Navigation/Navigation.js

-    if (lastRoute.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR || lastRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) {
-        // if we are not in the target report, we need to navigate to it after dismissing the modal
-        if (targetReportID && targetReportID !== getTopmostReportId(rootState)) {
-            const state = getStateFromPath(ROUTES.getReportRoute(targetReportID));
-
-            const action = getActionFromState(state, linkingConfig.config);
-            action.type = 'REPLACE';
-            navigationRef.current.dispatch(action);
-        } else {
-            navigationRef.current.dispatch(StackActions.pop());
+    switch (lastRoute.name) {
+        case NAVIGATORS.RIGHT_MODAL_NAVIGATOR:
+        case NAVIGATORS.FULL_SCREEN_NAVIGATOR:
+        case SCREENS.REPORT_ATTACHMENTS:
+            // if we are not in the target report, we need to navigate to it after dismissing the modal
+            if (targetReportID && targetReportID !== getTopmostReportId(rootState)) {
+                const state = getStateFromPath(ROUTES.getReportRoute(targetReportID));
+
+                const action = getActionFromState(state, linkingConfig.config);
+                action.type = 'REPLACE';
+                navigationRef.current.dispatch(action);
+            } else {
+                navigationRef.current.dispatch(StackActions.pop());
+            }
+            break;
+        default: {
+            Log.hmmm('[Navigation] dismissModal failed because there is no modal stack to dismiss');


I found it necessary to introduce an extra OR condition in our logic. For better readability and understanding of the flow, I decided to convert this structure into a switch statement.

allroundexperts · 2023-06-17T21:14:55Z

@kidroca Can you please resolve the conflicts again?

rushatgabhane · 2023-06-19T03:16:51Z

@allroundexperts just dropping a friendly note that you should review the code without waiting for @kidroca to resolve the conflicts. Especially if the conflicts are something trivial.

allroundexperts · 2023-06-19T03:18:17Z

@allroundexperts just dropping a friendly note that you should review the code without waiting for @kidroca to resolve the conflicts. Especially if the conflicts are something trivial.

Thanks @rushatgabhane!
I'll just continue with the review then.

# Conflicts: # src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js # src/libs/Navigation/Navigation.js

kidroca · 2023-06-19T11:41:47Z

@allroundexperts @rushatgabhane

The updates introduced in PR #20338 have led to a new merge conflict that isn't straightforward to resolve. In these changes, the AttachmentModal now expects a full report object via the report prop. However, my solution—where the modal functions as a route—relied on simple, serializable values such as reportID.

The approach from my PR, which depended on receiving only the report ID as a string, is no longer compatible:

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

Lines 57 to 58 in 3d5bc85

    
           const route = ROUTES.getReportAttachmentRoute(report.reportID, source); 
        
           Navigation.navigate(route);

To address this, I propose adjusting the logic so that we continue to pass only the reportID as a route parameter, with the screen subsequently obtaining and passing down the report object. This approach necessitates some modifications to my current logic and a partial reversion of the changes that introduced the merge conflict.

kidroca · 2023-06-21T19:19:32Z

Conflicts resolved ✅

Screenshots / videos updated

allroundexperts

This is looking good. All yours @NikkiWines!

NikkiWines

Looks good and tests well - Going to add @chiragsalian as a reviewer for a third set of eyes since he helped review the proposal

NikkiWines · 2023-06-21T23:34:38Z

src/pages/home/report/ReportAttachments.js

+    const reportID = _.get(props, ['route', 'params', 'reportID']);
+    const report = ReportUtils.getReport(reportID);
+    const source = decodeURI(_.get(props, ['route', 'params', 'source']));


NAB: these can all be used inline instead of declared separately

chiragsalian

Code LGTM and seems to work well. There is a feature freeze atm so i won't be merging this PR but i had one question, shouldn't we update the attachment URL when the user switches between attachments? I'm a little concerned that it could be confusing for a user seeing the same attachment URL while switching between multiple attachments

allroundexperts · 2023-06-23T23:00:31Z

Code LGTM and seems to work well. There is a feature freeze atm so i won't be merging this PR but i had one question, shouldn't we update the attachment URL when the user switches between attachments? I'm a little concerned that it could be confusing for a user seeing the same attachment URL while switching between multiple attachments

I had the same question @chiragsalian. I agree that it is confusing for the user.

kidroca · 2023-06-27T21:01:59Z

I appreciate the thought of changing the URL to provide feedback to the user when they switch attachments. This could indeed be helpful, especially in scenarios where the same attachment might be uploaded multiple times consecutively.

However, I am unsure about the overall benefit of this change. Only a small portion of the URL (around 5-10 digits we see) would vary between different attachments, while the majority of the URL would remain the same. This might not provide a significant difference in feedback to the user.

If we decide to proceed with this change, I would suggest implementing it as a separate pull request. This way, it would be easier to review and test independently.

NikkiWines · 2023-06-30T23:07:08Z

Feature freeze is over! @kidroca might be good just to pull main into this just to be safe, but we should be good to merge this 🚀

chiragsalian · 2023-06-30T23:08:18Z

Hmm sure i am fine with a separate pull request to make things easier to review/test.

Only a small portion of the URL (around 5-10 digits we see) would vary between different attachments, while the majority of the URL would remain the same.

Yeah no matter how small the change i like URLs to match what the user sees. Its always possible one chat report to another has just 1 different digit, but it is still valuable to keep the URL up to date imo.

chiragsalian · 2023-06-30T23:14:22Z

I'll go ahead and merge this PR to get it out the door soon. If something is wrong with it QA should hopefully catch problems. I'll mention in the issue that we'll be updating the URL realtime as a follow up.

OSBotify · 2023-07-01T00:16:01Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-07-03T14:49:20Z

🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.36-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Julesssss · 2023-07-04T09:47:35Z

Hey @kidroca, is it possible that changing how our attachments work triggered this issue? #22133

If so I don't think it's necessarily a App regression, but knowing this might help us to fix the issue.

kidroca · 2023-07-04T13:27:14Z

@Julesssss
I don't think so, I've explicitly included a test step regarding downloading various attachments in the PR description:

Download a variety of attachments and check that the downloading functionality remains intact.

If this PR did break downloads it should have been caught
I've also tested downloading myself and it worked

I'll take a look for a potential cause as I'm already working on another carousel update

Julesssss · 2023-07-04T13:30:55Z

Thanks for looking, what you said makes sense 👍 This is just occuring on staging, so it might not have shown up anyway during testing.

deetergp · 2023-07-04T20:58:22Z

We have a blocker (a relatively trivial one, IMO) that was introduced in this last release and since this PR is the only one that has to do with attachments, I'm wonder if didn't introduce this bug.

kidroca · 2023-07-05T07:56:03Z

@deetergp, you've made a valid point.

Prior to this, I was unaware of the existing functionality that relied upon a specific hierarchy to restrict drag and drop actions over attachments. It's plausible that by altering this hierarchy, I unintentionally introduced the issue at hand.

OSBotify · 2023-07-05T17:12:33Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.36-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

s77rt · 2024-02-05T02:34:50Z

src/pages/home/report/ReportAttachments.js

+function ReportAttachments(props) {
+    const reportID = _.get(props, ['route', 'params', 'reportID']);
+    const report = ReportUtils.getReport(reportID);
+    const source = decodeURI(_.get(props, ['route', 'params', 'source']));


The source can be a number (viewing assets in native) but the decoded uri is always a string which makes the image fail to load. We needed to preserve the type of the passed source. (Coming from #31138)

parasharrajat · 2024-06-06T12:46:08Z

We missed handling a case for inaccessible report images. When the user opens a link to attachment where the user does not have access to the report, the attachment modal keeps on loading. #23374

kidroca commented Jun 5, 2023

View reviewed changes

kidroca marked this pull request as ready for review June 5, 2023 13:58

kidroca requested a review from a team as a code owner June 5, 2023 13:58

melvin-bot bot requested review from NikkiWines and rushatgabhane and removed request for a team June 5, 2023 13:58

kidroca mentioned this pull request Jun 5, 2023

[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 #18451

Closed

6 tasks

kidroca added 6 commits June 8, 2023 00:53

Add REPORT_ATTACHMENT route

7b00c12

AttachmentModal: add defaultOpen prop

eb3e092

AttachmentModal: make passing children optional

2c758a4

Add src/pages/home/report/ReportAttachments.js modal screen

236df33

Apply prettier on the changes

2b164e9

NikkiWines requested review from allroundexperts and removed request for rushatgabhane June 12, 2023 15:09

kidroca added 3 commits June 14, 2023 13:55

Refactor after rebasing - move Report_Attachments to `CentralPaneNa…

ca27f0d

…vigator`

Refactor after rebasing - move Report_Attachments to root navigation

d5c6e4c

Otherwise, when closing the modal there's a blank screen displayed briefly It seems the previous screen does not stay underneath the modal unless it's on the root

Refactor: extract "Report_Attachments" string to SCREENS

3d5bc85

kidroca force-pushed the kidroca/feat/attachment-modal-route branch from ff20435 to 3d5bc85 Compare June 14, 2023 13:13

kidroca commented Jun 14, 2023

View reviewed changes

Merge branch 'main' into kidroca/feat/attachment-modal-route

f08d740

# Conflicts: # src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js # src/libs/Navigation/Navigation.js

allroundexperts approved these changes Jun 21, 2023

View reviewed changes

NikkiWines approved these changes Jun 22, 2023

View reviewed changes

NikkiWines requested a review from chiragsalian June 22, 2023 00:40

chiragsalian approved these changes Jun 23, 2023

View reviewed changes

chiragsalian merged commit 206f107 into Expensify:main Jun 30, 2023

allroundexperts mentioned this pull request Jul 1, 2023

[HOLD for payment 2023-07-20] [$1000] Attachments flagged as offensive can be viewed and downloaded from attachements previews #21625

Closed

6 tasks

OSBotify mentioned this pull request Jul 3, 2023

Deploy Checklist: New Expensify 2023-07-03 #22116

Closed

40 tasks

kidroca deleted the kidroca/feat/attachment-modal-route branch July 4, 2023 13:03

This was referenced Jul 4, 2023

Refactor AttachmentModal to function component #20365

Merged

[HOLD for payment 2023-07-17] [$1000] Web - User is unable to download attachment #22133

Closed

Julesssss mentioned this pull request Jul 5, 2023

[HOLD for payment 2023-09-04] [$1000] Not allowed sign does not show when an attachment is drag and dropped #22218

Closed

6 tasks

allroundexperts mentioned this pull request Jul 29, 2023

[$2000] Web - Clicking on PDF causes NewDot to crash #22624

Closed

6 tasks

s77rt reviewed Feb 5, 2024

View reviewed changes

s77rt mentioned this pull request Feb 5, 2024

[PAID] [$500] Distance - Receipt opens Not here page instead of e-receipt when Distance is created offline #31138

Closed

6 tasks

parasharrajat mentioned this pull request Jun 6, 2024

[HOLD for payment 2024-06-13] [$500] Infinite loading spinner when trying to open a link to an Image that is in a room I have no access to. #23374

Closed

6 tasks

		REPORT_ATTACHMENT: 'r/:reportID/attachment',
		getReportAttachmentRoute: (reportID, source) => `r/${reportID}/attachment?source=${encodeURI(source)}`,

		const route = ROUTES.getReportAttachmentRoute(report.reportID, source);
		Navigation.navigate(route);

Report Attachments Modal Route #20167

Report Attachments Modal Route #20167

Conversation

kidroca commented Jun 5, 2023 • edited Loading

Details

Changes:

Fixed Issues

Tests

Test 1: Verify Existing Functionality is Unaltered

Test 2: Verify the issue with deleting an image is resolved

Offline tests

QA Steps

Test 1: Verify Existing Functionality is Unaltered

Test 2: Verify the issue with deleting an image is resolved

PR Author Checklist

Screenshots/Videos

kidroca left a comment • edited Loading

Choose a reason for hiding this comment

kidroca Jun 5, 2023

Choose a reason for hiding this comment

kidroca Jun 5, 2023

Choose a reason for hiding this comment

melvin-bot bot commented Jun 5, 2023

rushatgabhane commented Jun 11, 2023 • edited Loading

allroundexperts commented Jun 11, 2023

allroundexperts commented Jun 12, 2023

kidroca commented Jun 13, 2023

kidroca left a comment

Choose a reason for hiding this comment

kidroca Jun 14, 2023

Choose a reason for hiding this comment

Sample Behavior When Not Using RootStack

Sample Behavior When Using RootStack

kidroca Jun 14, 2023

Choose a reason for hiding this comment

allroundexperts commented Jun 17, 2023

rushatgabhane commented Jun 19, 2023 • edited Loading

allroundexperts commented Jun 19, 2023

kidroca commented Jun 19, 2023

kidroca commented Jun 21, 2023

allroundexperts left a comment

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

NikkiWines Jun 21, 2023

Choose a reason for hiding this comment

chiragsalian left a comment

Choose a reason for hiding this comment

allroundexperts commented Jun 23, 2023

kidroca commented Jun 27, 2023

NikkiWines commented Jun 30, 2023

chiragsalian commented Jun 30, 2023

chiragsalian commented Jun 30, 2023

OSBotify commented Jul 1, 2023

OSBotify commented Jul 3, 2023

Julesssss commented Jul 4, 2023

kidroca commented Jul 4, 2023

Julesssss commented Jul 4, 2023 • edited Loading

deetergp commented Jul 4, 2023

kidroca commented Jul 5, 2023

OSBotify commented Jul 5, 2023

s77rt Feb 5, 2024

Choose a reason for hiding this comment

parasharrajat commented Jun 6, 2024

kidroca commented Jun 5, 2023 •

edited

Loading

kidroca left a comment •

edited

Loading

rushatgabhane commented Jun 11, 2023 •

edited

Loading

Sample Behavior When Not Using `RootStack`

Sample Behavior When Using `RootStack`

rushatgabhane commented Jun 19, 2023 •

edited

Loading

Julesssss commented Jul 4, 2023 •

edited

Loading