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

[HOLD for payment 2023-08-28] [HOLD for payment 2023-08-24] LNH - The conversation will be removed from the LHN if the last message is an attachment #24593

Closed
6 tasks done
lanitochka17 opened this issue Aug 15, 2023 · 42 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 15, 2023

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


Issue found when executing PR #24466

Action Performed:

  1. Open New Expensify app or https://staging.new.expensify.com/
  2. Navigate to any conversation
  3. Click the action menu in the conversation
  4. Click on "Add attachment"
  5. Add any valid attachment
  6. Navigate to another conversation with another user

Expected Result:

A conversation should not disappear from the LNH menu, even if the last message was an attachment

Actual Result:

If the last message in a conversation is an attachment, that conversation disappears from the LNH menu when you switch to another conversation

Workaround:

Unknown

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.54.8

Reproducible in staging?: Yes

Reproducible in production?: No

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

Bug6165898_Recording__26__2_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team / @namhihi237

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

View all open jobs on GitHub

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Aug 15, 2023
@OSBotify
Copy link
Contributor

👋 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.

@namhihi237
Copy link
Contributor

I think I had reported here

@melvin-bot
Copy link

melvin-bot bot commented Aug 15, 2023

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

@Nikhil-Vats
Copy link
Contributor

Proposal

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

LHN - The conversation is be removed from the LHN if the last message is an attachment.

What is the root cause of that problem?

In ReportUtils, we check if chat is empty by checking the key lastMessageText for the object returned by function getLastVisibleMessage.

App/src/libs/ReportUtils.js

Lines 2505 to 2510 in 2e8e699

const isEmptyChat = !ReportActionsUtils.getLastVisibleMessage(report.reportID).lastMessageText;
// Hide only chat threads that haven't been commented on (other threads are actionable)
if (isChatThread(report) && isEmptyChat) {
return false;
}

But in getLastVisibleMessage, we return a different key lastMessageTranslationKey for attachments -

function getLastVisibleMessage(reportID, actionsToMerge = {}) {
const lastVisibleAction = getLastVisibleAction(reportID, actionsToMerge);
const message = lodashGet(lastVisibleAction, ['message', 0], {});
if (isReportMessageAttachment(message)) {
return {
lastMessageTranslationKey: CONST.TRANSLATION_KEYS.ATTACHMENT,
};
}

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

We should check for both keys in shouldReportBeInOptionList while evaluating isEmptyChat -

const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
const isEmptyChat = !lastVisibleMessage.lastMessageText && !lastVisibleMessage.lastMessageTranslationKey;

What alternative solutions did you explore? (Optional)

NA

@Nikhil-Vats
Copy link
Contributor

Nikhil-Vats commented Aug 15, 2023

Hey @luacmartins @mountiny, this is a regression from #19321. I have found the root cause and tested that it is working. I can open a PR within right now if you agree.

@PauloGasparSv
Copy link
Contributor

@Nikhil-Vats's proposal looks good to me!

@Nikhil-Vats
Copy link
Contributor

@PauloGasparSv can I create a PR then?

@melvin-bot melvin-bot bot added Daily KSv2 and removed Hourly KSv2 labels Aug 15, 2023
@PauloGasparSv
Copy link
Contributor

@PauloGasparSv can I create a PR then?

Let's just wait on @johnmlee101 too since they got assigned here : )

@mountiny
Copy link
Contributor

@PauloGasparSv I dont think we have to wait for John for deploy blockers, as author of the original PR you have the best context, @Nikhil-Vats I think you can raise the PR with urgency as this is a deploy blcoker 🚀 thanks!

@PauloGasparSv
Copy link
Contributor

@PauloGasparSv I dont think we have to wait for John for deploy blockers, as author of the original PR you have the best context, @Nikhil-Vats I think you can raise the PR with urgency as this is a deploy blcoker 🚀 thanks!

Will keep this in mind for next time!!!

Sorry for the delay then @Nikhil-Vats

@mountiny
Copy link
Contributor

No worries, with deploy blockers its better to act as fast as possible 🚤

@Nikhil-Vats
Copy link
Contributor

On it.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 15, 2023
@Nikhil-Vats
Copy link
Contributor

PR is ready for review.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 17, 2023
@melvin-bot melvin-bot bot changed the title LNH - The conversation will be removed from the LHN if the last message is an attachment [HOLD for payment 2023-08-24] LNH - The conversation will be removed from the LHN if the last message is an attachment Aug 17, 2023
@johnmlee101
Copy link
Contributor

Assigning bug so we have a bug-zero member to handle the issue management!

@abdulrahuman5196
Copy link
Contributor

Just to note @zanyrenney #24593 (comment)

@melvin-bot melvin-bot bot added the Overdue label Aug 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 29, 2023

@johnmlee101, @Nikhil-Vats, @zanyrenney Eep! 4 days overdue now. Issues have feelings too...

@zanyrenney
Copy link
Contributor

Hm @johnmlee101 where is the upwork job? I don't know what you mean by this comment:

Assigning bug so we have a bug-zero member to handle the issue management!

@melvin-bot melvin-bot bot removed the Overdue label Aug 29, 2023
@abdulrahuman5196
Copy link
Contributor

@zanyrenney This was a deployment blocker bug. Upwork link was not created(Not sure why)

Assigning bug so we have a bug-zero member to handle the issue management!

AFAIK, This was meant to close the payments on this issue.

@johnmlee101
Copy link
Contributor

Correct, we'll need to manage payments for this, which I don't have control over

@zanyrenney
Copy link
Contributor

zanyrenney commented Sep 1, 2023

job is here - can't find the upwork profiles for the c and c+,, please instead apply to the job @Nikhil-Vats @abdulrahuman5196

also the price is wrong and upwork isn't letting me change it.

The payment summary is:

Reporting bonus - $250 @namhihi237
Contributor payment $1000 [@Nikhil-Vats] + $500 urgency bonus
contributor plus payment $1000 [@abdulrahuman5196] + $500 urgency bonus

@melvin-bot melvin-bot bot removed the Overdue label Sep 1, 2023
@zanyrenney
Copy link
Contributor

I will pay the $1000 to each via upwork manually.

@Nikhil-Vats
Copy link
Contributor

@zanyrenney There is also urgency bonus for this. Also, can you please add the upwork job link? I can't find it.

@namhihi237
Copy link
Contributor

@zanyrenney Hello I am reporter, why I can not payment for issue?

@zanyrenney
Copy link
Contributor

hey @namhihi237 sorry i thought it said reported by internal team - Applause.

@zanyrenney
Copy link
Contributor

@namhihi237
Copy link
Contributor

@zanyrenney I reported before on slack as you can see the reporter has my name above. @mountiny Can you confirm?

@zanyrenney
Copy link
Contributor

Yep it's fine, I can see that, i've updated it. can you share your upwork profile? i can't find you in previous hires.

@namhihi237
Copy link
Contributor

@zanyrenney
Copy link
Contributor

zanyrenney commented Sep 1, 2023

@abdulrahuman5196 - invited to job
@Nikhil-Vats - offer sent
@namhihi237 - invited to job

@namhihi237
Copy link
Contributor

I just submitted the job.

@zanyrenney
Copy link
Contributor

zanyrenney commented Sep 1, 2023

payment summary
Reporting bonus - $250 @namhihi237 [PAID]
Contributor payment $1000 [@Nikhil-Vats] + $500 urgency bonus [PAID]
contributor plus payment $1000 [@abdulrahuman5196] + $500 urgency bonus [PAID]

@abdulrahuman5196
Copy link
Contributor

@zanyrenney yes that's me

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@zanyrenney
Copy link
Contributor

Thanks! please accept the job so I can payout.

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@zanyrenney
Copy link
Contributor

zanyrenney commented Sep 4, 2023

All paid, see payment summary above: #24593 (comment)

@zanyrenney
Copy link
Contributor

Closing!

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. Daily KSv2 Engineering
Projects
None yet
Development

No branches or pull requests

10 participants