-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2024-10-14] [$250] mWeb - Attachment - Opened offline attachment directed to conversation page on online #48173
Comments
Triggered auto assignment to @kevinksullivan ( |
ProposalPlease restate the problem we are trying to solve with this issue.mWeb - Attachment - Opened offline attachment directed to conversation page on online. What is the root cause of this problem?When we are offline, we upload an attachment and click on it to view the details by clicking the attachment. When we return online, we invoke App/src/components/AttachmentModal.tsx Lines 257 to 273 in 6697c26
What changes do you think we should make in order to solve the problem?We can add a new parameter to the URL or a parameter inside the function to indicate how the AttachmentModal was opened. For example, we can use What alternative solutions did you explore? (Optional)We can check if there is existing Onyx data for the attachment. If there is, it means the attachment has been submitted but not yet added to the backend. In this case, we can avoid invoking setIsModalOpen(false) and keep the modal open. |
Proposal |
Edited by proposal-police: This proposal was edited at 2024-08-28 14:38:24 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.When opening a sent attachment while offline, after going online, the app navigates to a report. What is the root cause of that problem?When viewing an attachment image, the App/src/components/Attachments/AttachmentCarousel/index.tsx Lines 91 to 94 in 6697c26
The lines detects if the viewed image is deleted by comparing the current source with the updated attachment properties retrieved from When the user is offline, the current source is a The current detection logic mistakenly identifies the image as deleted because the What changes do you think we should make in order to solve the problem?We could add a check in the The revised code could look like this, assuming a report action can have multiple images (i.e., an array): if (initialPage === -1 && attachments.find(compareImage)) {
let isUploadedImage = false;
// maybe cheching for wheter the file is blob uri object is enough, but for more code completeness I am adding additional checks.
// If we want to checks whether the blob uri object exist we can use `FileUtils.readFileAsync`
if (source.startsWith('blob')) {
const uploadedAttachment = attachments.find(compareImage);
const possibleTargetUploads = targetAttachments.filter(attachment => {
return attachment.reportActionID === uploadedAttachment.reportActionID;
});
isUploadedImage = possibleTargetUploads.some(attachment => {
return attachment.file.name === uploadedAttachment.file.name;
});
}
if (!isUploadedImage) {
Navigation.dismissModal();
}
} Alternatively, we could check for pending actions such as deletion or addition, if this approach is more accurate, or simply verify whether the What alternative solutions did you explore? (1)We can determine whether the report action has been deleted by checking the availability of currently viewed report attahcment's reportActionID in The code could be: const currentAttachmentReportActionID = attachments.find(compareImage)?.reportActionID;
let initialPage;
if (currentAttachmentReportActionID) {
initialPage = targetAttachments.findIndex(attachment => attachment.reportActionID === currentAttachmentReportActionID);
} else {
initialPage = targetAttachments.findIndex(compareImage);
} Or : let initialPage = targetAttachments.findIndex(compareImage);
const currentAttachmentReportActionID = attachments.find(compareImage)?.reportActionID;
const isReportActionExist = currentAttachmentReportActionID ? targetAttachments.find(attachment => attachment.reportActionID === currentAttachmentReportActionID) : initialPage !== -1;
// Dismiss the modal when deleting an attachment during its display in preview.
if (initialPage === -1 && attachments.find(compareImage)) {
if (!isReportActionExist) {
Navigation.dismissModal();
}
} What alternative solutions did you explore? (2)Expanding my first solution which I mention to use pendingAction data: In this line:
We need to modify it to: const html = ReportActionsUtils.getReportActionHtml(action).replace(/(<(?:(?=video )|(?=img )).+?)(\/*)>/gm, `$1 data-flagged="${hasBeenFlagged}" data-id="${action.reportActionID}" data-pending-action="${action?.pendingAction}"$2>`); Then, in this line and this line, we should add: pendingAction: attribs['data-pending-action'] In the const currentlyViewedAttachment = attachments.find(compareImage);
if (initialPage === -1 && !!currentlyViewedAttachment) {
if (currentlyViewedAttachment?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
Navigation.dismissModal();
}
} What alternative solutions did you explore? (3)This is a simplified version of my initial solution to determine whether a file is stored temporarily by the browser or app. We can check if the source starts with "blob" or "file," or similar characters that indicate it is a file stored in user storage. The code could be: if (initialPage === -1 && !!currentlyViewedAttachment) {
if (source.startsWith('blob') || source.startsWith('file')) {
return;
}
Navigation.dismissModal();
} Also there is a bug in when user send a text with image attachment, the reportActionID data will be undefined. This in because:
If user send a text comment and an image, there is const html = ReportActionsUtils.getReportActionHtml(action).replace(/(<img .+?)\/>/gm, `$1 data-flagged="${hasBeenFlagged}" data-id="${action.reportActionID}"/>`); To also fix for video: const html = ReportActionsUtils.getReportActionHtml(action).replace(/(<(?:(?=video )|(?=img )).+?)(\/*)>/gm, `$1 data-flagged="${hasBeenFlagged}" data-id="${action.reportActionID}"$2>`); |
Edited by proposal-police: This proposal was edited at 2024-08-29 02:10:22 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Upload and open attachment in offline, then going online user directed to conversation page What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
if(isLocalFile(source)){
return
}
Navigation.dismissModal();
What alternative solutions did you explore? (Optional) |
Job added to Upwork: https://www.upwork.com/jobs/~01e366edd029d4e402 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt ( |
This is not the case. The function is invoked as soon as you click the submit button (while offline) |
@tsa321 Your RCA is correct. However the suggested solution is a workaround. The correct approach would be to make the comparison function correctly identify that we are viewing an image that does exist. |
@daledah Your RCA is correct but same note as above regarding the solution. The attachment carousel should be able to tell that we are still viewing the same image even though the source did change. IOW |
It's worth mentioning that the |
Proposal updated
|
@daledah A file name is not a unique identifier and thus we cannot use it for lookup. |
Imagine user A sends a reportAction containing 3 images to user B, all linked to a single reportAction key. User B opens the carousel to view the 2nd image. Then, user A edits the reportAction and removes the 2nd image. As a result, for user B, the carousel does not close. |
@tsa321 Thanks, Edit: see #48173 (comment) |
@daledah I'm not sure if multiple images in one report action is supported. So far I keep getting empty images, let me double check |
@daledah It turns out we can send multiple images in one report action (although it seems a bit broken; the report action id for the second image is undefined) ![image1.jpeg](https://img.freepik.com/free-photo/autumn-tree-forest-leaves-bright-yellow-generative-ai_188544-12668.jpg)
![image2.jpeg](https://img.freepik.com/free-photo/photorealistic-view-tree-nature-with-branches-trunk_23-2151478039.jpg) Thus we can't use the report action id for image identification either cc @tsa321 |
Proposalon alternative solution 2 and 3. |
Updated proposal
|
@wildan-m The new behaviour seems to work well and does fix the issue. Let's go with alternative solution (3) without the 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @rlinoz, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @wildan-m 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Note to self... PR hit staging on Oct 4th (posting as a reminder in case production deploy automation fails, it should be fixed now though) |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.45-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-10-14. 🎊 For reference, here are some details about the assignees on this issue:
|
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:
|
|
Contributor: @wildan-m paid $250 via Upwork I posted in the other issue to state we need a regression test there Thx! |
$250 approved for @s77rt |
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.25
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
Upload and open attachment in offline, then going online user must stay in same attachment page
Actual Result:
Upload and open attachment in offline, then going online user directed to conversation page
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6584997_1724833122321.Screenrecorder-2024-08-27-17-12-45-779_compress_1.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @mallenexpensifyThe text was updated successfully, but these errors were encountered: