Skip to content
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

[Awaiting Payment][$1000] Web - URL preview not removed when editing a chat #21191

Closed
6 tasks done
kbecciv opened this issue Jun 21, 2023 · 64 comments
Closed
6 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jun 21, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to any chat
  2. Send website URL (ex. google.com)
  3. Observe the site preview thumbnail
  4. Hover over message > click Edit Comment > write a message
  5. Observe the URL preview remains

Expected Result:

The preview should be gone after editing the message

Actual Result:

Preview is visible after editing the message

Workaround:

Uknown

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.27-6

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

Screen.Recording.2023-06-14.at.12.54.33.PM.mov

Expensify/Expensify Issue URL:

Issue reported by: @niravkakadiya25

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686727685589039

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0164e3fa7f55938949
  • Upwork Job ID: 1671601917583003648
  • Last Price Increase: 2023-07-28
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@StevenKKC
Copy link
Contributor

StevenKKC commented Jun 21, 2023

Proposal

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

After editing the link to a simple message, the site preview does not disappear.

What is the root cause of that problem?

The site preview for the link is displayed in the LinkPreviewer.

{!_.isEmpty(props.action.linkMetadata) && (
<View style={props.draftMessage ? styles.chatItemReactionsDraftRight : {}}>
<LinkPreviewer linkMetadata={_.filter(props.action.linkMetadata, (item) => !_.isEmpty(item))} />
</View>
)}

After editing the link, the app sends ExpandURLPreview API to update site preview.

useEffect(() => {
const urls = ReportActionsUtils.extractLinksFromMessageHtml(props.action);
if (_.isEqual(downloadedPreviews.current, urls) || props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
return;
}
downloadedPreviews.current = urls;
Report.expandURLPreview(props.report.reportID, props.action.reportActionID);
}, [props.action, props.report.reportID]);

But for a simple message, the API returns empty onyxData, and the linkMetadata is not updated accordingly.
This is the root cause that the site preview does not disappear.

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

If message is edited, we should update linkMetadata.

Following case should be considered.
If user A send a message 'google.com' to user B, user A and user B will both see the google preview.
Then, if user A edits a message to simple text or other link, user B will see the updated message, but the link preview will not be updated.

Therefore, I think it's best to clear linkMetadata in useEffects.

    useEffect(() => {
        const urls = ReportActionsUtils.extractLinksFromMessageHtml(props.action);
+       if (_.isEmpty(urls) && props.action.linkMetadata) {
+           Report.clearURLPreview(props.report.reportID, props.action.reportActionID);
+           return;
+       }
        if (_.isEqual(downloadedPreviews.current, urls) || props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
            return;
        }

        // When open report, `downloadedPreviews.current` is empty, and `linkMetadata` maybe not empty, 
        // so clear `linkMetata` only if `downloadedPreviews.current` is not empty and different with `urls`.
+       if (!_.isEmpty(downloadedPreviews.current)) {
+           Report.clearURLPreview(props.report.reportID, props.action.reportActionID);
+       }

        downloadedPreviews.current = urls;
        Report.expandURLPreview(props.report.reportID, props.action.reportActionID);
    }, [props.action, props.report.reportID]);

And add a clearURLPreview function in Report.js.

function clearURLPreview(reportID, reportActionID) {
    Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {
        [reportActionID]: {
            linkMetadata: [],
        },
    });
}

What alternative solutions did you explore? (Optional)

None.

@hungvu193
Copy link
Contributor

Proposal

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

Web - After edit site link to simple message then also preview is show

What is the root cause of that problem?

We are updating the linkMetaData in here:

useEffect(() => {
const urls = ReportActionsUtils.extractLinksFromMessageHtml(props.action);
if (_.isEqual(downloadedPreviews.current, urls) || props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
return;
}
downloadedPreviews.current = urls;
Report.expandURLPreview(props.report.reportID, props.action.reportActionID);
}, [props.action, props.report.reportID]);

After editing the link to some text, it called Report.expandURLPreview(props.report.reportID, props.action.reportActionID); again, but it's returning empty so our old linkMetaData still persisted which caused this issue.

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

In our editReportComment function, we should check if the newComment doesn't have urls, then we should update our linkMetaData in our reportAction to empty.

function editReportComment(reportID, originalReportAction, textForNewComment) {
// or we can create a new function to get html from text.
const urls = ReportActionsUtils.extractLinksFromMessageHtml({message: [{html: htmlForNewComment}]});

// update the linkMetaData if newComment doesn't have the urls.
    if (_.isEmpty(urls)) {
        optimisticReportActions[reportActionID].linkMetadata = [];
    }

What alternative solutions did you explore? (Optional)

N/A

@Christinadobrzyn Christinadobrzyn changed the title Web - After edit site link to simple message then also preview is show Web - URL preview not removed when editing a chat Jun 21, 2023
@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Jun 21, 2023
@melvin-bot melvin-bot bot changed the title Web - URL preview not removed when editing a chat [$1000] Web - URL preview not removed when editing a chat Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0164e3fa7f55938949

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@Christinadobrzyn
Copy link
Contributor

I can reproduce - adding External label

@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

Triggered auto assignment to @MonilBhavsar (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@melvin-bot melvin-bot bot added the Overdue label Jun 23, 2023
@Christinadobrzyn
Copy link
Contributor

@eVoloshchak can you review the proposals when you have a moment? Thank you!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 23, 2023
@eVoloshchak
Copy link
Contributor

@StevenKKC's proposal looks good to me!
Both proposals are pretty similar and were posted very closely on terms of time, so let's proceed with the first proposal

🎀👀🎀 C+ reviewed!
cc: @Christinadobrzyn

@melvin-bot melvin-bot bot removed the Overdue label Jun 26, 2023
@Christinadobrzyn
Copy link
Contributor

Hey @MonilBhavsar Can you give your thoughts on ^ #21191 (comment)

@hungvu193
Copy link
Contributor

hungvu193 commented Jun 27, 2023

@StevenKKC's proposal looks good to me! Both proposals are pretty similar and were posted very closely on terms of time, so let's proceed with the first proposal

🎀👀🎀 C+ reviewed! cc: @Christinadobrzyn

@eVoloshchak I think the difference between my proposal and Steven is that, my proposal only cleared the preview when there's no url inside our message.
If we always clear our message when editing, this will cause another bug when you edit your message from this link to similar link. IE you edited your message from google.com to google.com hello. So if we already had some urls in our message, let expandURLPreview handle the rest instead of clearing them.

@melvin-bot
Copy link

melvin-bot bot commented Jun 28, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@MonilBhavsar
Copy link
Contributor

Sorry for delay. Catching up from ooo...
@eVoloshchak I think I agree with @hungvu193 's comment here #21191 (comment)
Could you please take a look

@MonilBhavsar
Copy link
Contributor

I have pushed a PR in internal repo. Thank you everyone for good discussion on this issue 🙇

@MonilBhavsar MonilBhavsar added the Reviewing Has a PR in review label Jul 31, 2023
@Christinadobrzyn
Copy link
Contributor

back from ooo - I'll take this back @flaviadefaria - thanks for handling it for me.

@MonilBhavsar can we move this to weekly while the PR is under review?

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Aug 1, 2023

PR is merged.
And since this was internal, we have one payment for issue reporter - @niravkakadiya25


And add regression tests for URL preview messages -

  1. Send a message with url - expensify.com
  2. Verify link preview is displayed
  3. Edit that message and remove the link - expensify.com
  4. Ensure link preview is also removed
  5. Send a message with two url's - expensify.com and google.com
  6. Verify both link previews are displayed
  7. Edit that message and remove one link - google.com and verify preview of that link is removed
  8. Edit that message and remove another link too - expensify.com and verify preview of that link is removed
  9. Send a message with url - slack.com
  10. Edit that message to expensify.com
  11. Verify link preview is also updated

We can close this issue then

@MonilBhavsar MonilBhavsar added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Aug 1, 2023
@MonilBhavsar MonilBhavsar changed the title [$1000] Web - URL preview not removed when editing a chat [Awaiting Payment][$1000] Web - URL preview not removed when editing a chat Aug 1, 2023
@MonilBhavsar MonilBhavsar added Weekly KSv2 and removed Daily KSv2 labels Aug 1, 2023
@Christinadobrzyn Christinadobrzyn added Internal Requires API changes or must be handled by Expensify staff and removed Help Wanted Apply this label when an issue is open to proposals by contributors Internal Requires API changes or must be handled by Expensify staff labels Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Current assignee @eVoloshchak is eligible for the Internal assigner, not assigning anyone new.

@Christinadobrzyn Christinadobrzyn added External Added to denote the issue can be worked on by a contributor and removed Internal Requires API changes or must be handled by Expensify staff labels Aug 1, 2023
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Current assignees @flaviadefaria and @Christinadobrzyn are eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

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

@Christinadobrzyn Christinadobrzyn added Weekly KSv2 Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Current assignee @eVoloshchak is eligible for the Internal assigner, not assigning anyone new.

@Christinadobrzyn
Copy link
Contributor

Sorry for all the labels, trying to get an Upwork job auto-created since the original one for this is closed now. Didn't work.

Manually created a job in Upwork
Internal job- https://www.upwork.com/ab/applicants/1686465413539557376/job-details
External job- https://www.upwork.com/jobs/~01300e05a1b86d26fc

@niravkakadiya25 - I sent you an offer for $250 as reporter - can you please accept so I can pay you?
@eVoloshchak are we paying you through NewExpensify? The payment will be $1000, correct?

Regression test here - https://github.com/Expensify/Expensify/issues/304725

@Christinadobrzyn
Copy link
Contributor

Per this comment #21191 (comment)

Paid @niravkakadiya25 $250 as reporter

Closing this GH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants