Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2024-05-06] [HIGH] [Splits][$500] Split - Group chat is missing everywhere when created via FAB and bill is split in group chat #37519

Closed m-natarajan closed 6 months ago

m-natarajan commented 8 months ago

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: v1.4.45-1

Reproducible in staging?: Yes Reproducible in production?: Yes 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. Go to staging.new.expensify.com
  2. Click FAB > Request money > Manual.
  3. Click a split bill with two users that have no prior chat via FAB.
  4. Navigate to a different chat.
  5. Note that the group chat remains in LHN and appears in Search.
  6. Go to FAB > Start chat.
  7. Create a group chat with two users that have no prior chat.
  8. In the group chat, click + > Split bill.
  9. Create a bill split.
  10. Go to a different chat.
  11. Open Search.

    Expected Result:

    The group chat will remain in LHN and appear in Search list.

Actual Result:

The group chat disappears from LHN and is missing in Search list. This issue only happens when user creates a group chat first, then splits bill in the group chat (Step 6 - 11). The only way to retrieve the group chat is by recreating it.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

https://github.com/Expensify/App/assets/38435837/87159b78-56c6-46d5-a8ef-62a010e870e6

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016f541763635e6d84
  • Upwork Job ID: 1767854808847859712
  • Last Price Increase: 2024-03-13
  • Automatic offers:
    • mkhutornyi | Contributor | 0
    • bernhardoj | Contributor | 0
Issue OwnerCurrent Issue Owner: @mananjadhav
melvin-bot[bot] commented 8 months ago

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

m-natarajan commented 8 months ago

We think that this bug might be related to #vip-split-p2p-chat-groups cc @arielgreen

m-natarajan commented 8 months ago

@dylanexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

bernhardoj commented 8 months ago

Proposal

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

The group chat disappears from LHN even after splitting a bill and can't be found on the search page.

What is the root cause of that problem?

When we create a new group chat, the notificationPreference and visibleChatMemberAccountIDs from the BE response will be hidden and empty. After doing a split bill, both data are unchanged. It will be updated once we trigger the OpenReport request by reopening or refreshing the page. If notificationPreference is hidden, it will render nothing. https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/LHNOptionsList/OptionRowLHN.tsx#L60-L63

If visibleChatMemberAccountIDs is empty, we won't include it in the search results https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/libs/OptionsListUtils.ts#L1513-L1515

We previously encountered the notificationPreference issue here when adding a new comment to the report.

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

We can use the same solution from https://github.com/Expensify/App/pull/29681, that is to update the notificationPreference to always if it's currently hidden when splitting bill.

if (ReportUtils.getReportNotificationPreference(splitChatReport) === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
    splitChatReport.notificationPreference = CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS;
}

And to show it on the search page, based on @marcochavezf comment here, we want to replace visibleChatMemberAccountIDs with participants, so we need to update the way to get the IDs in OptionsListUtils. https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/libs/OptionsListUtils.ts#L1520

const accountIDs = isSelfDM ? [currentUserAccountID ?? 0] : Object.keys(report.participants ?? {}).map(Number);

(we probably would need to do the same for assign task, send, and request money, but depends on how BE do it because for request money, BE never updates notificationPreference)

melvin-bot[bot] commented 8 months ago

@dylanexpensify Huh... This is 4 days overdue. Who can take care of this?

dylanexpensify commented 8 months ago

Apologies, I've been OOO sick, but am reviewing today!

dylanexpensify commented 8 months ago

Hmm @m-natarajan can you confirm this is still reproducible?

dylanexpensify commented 8 months ago

Hm so the chat->bill create shows in the Search but not LHN (disappears). cc @arielgreen

dylanexpensify commented 8 months ago

confirming here

dylanexpensify commented 8 months ago

Confirmed and making external

melvin-bot[bot] commented 8 months ago

Job added to Upwork: https://www.upwork.com/jobs/~016f541763635e6d84

melvin-bot[bot] commented 8 months ago

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

abzokhattab commented 8 months ago

Proposal

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

The group chat disappears from the search page after creating a split bill.

What is the root cause of that problem?

when creating a split bill, the backend returns visibleChatMemberAccountIDs as empty arr which causes the group chat not to appear in the search records

so here we are assigning the visibleChatMemberAccountIDs to the accountIDs and excluding their reports from the result if the accountIDs list is empty:

https://github.com/Expensify/App/blob/8f32e60b377b382f04e2dc69c664ab0771ec7347/src/libs/OptionsListUtils.ts#L1478-L1478

https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/libs/OptionsListUtils.ts#L1513-L1515

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

according to Marcos comment we should use the participants object instead of the visibleChatMemberAccountIDs list when possible

so we need to change the prev line to:

const accountIDs = isSelfDM ? [currentUserAccountID ?? 0] : (report?.participants ? Object.keys(report.participants).map(Number) : report.visibleChatMemberAccountIDs) ?? [];

Test:

image
mkhutornyi commented 8 months ago

Taking this over from @ntdiary (context)

melvin-bot[bot] commented 8 months ago

@dylanexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

mkhutornyi commented 8 months ago

When we create a new group chat, the notificationPreference and visibleChatMemberAccountIDs from the BE response will be hidden and empty.

@bernhardoj From my testing, both data are unchanged even after OpenReport

https://github.com/Expensify/App/assets/97676131/c0a383df-b0ef-4692-8555-349f41082021

mkhutornyi commented 8 months ago

@abzokhattab we should fix the root cause of visibleChatMemberList being empty.

melvin-bot[bot] commented 8 months ago

@dylanexpensify Huh... This is 4 days overdue. Who can take care of this?

dylanexpensify commented 8 months ago

@mkhutornyi to review

melvin-bot[bot] commented 8 months ago

πŸ“£ @mkhutornyi πŸŽ‰ 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 πŸ“–

bernhardoj commented 8 months ago

@mkhutornyi hmm, you're right.

(we probably would need to do the same for assign task, send, and request money, but depends on how BE do it because for request money, BE never updates notificationPreference & visibleChatMemberAccountIDs)

There are indeed some cases where it doesn't update as I mention here, but for split bill, it does updated when I writing down the proposal.

In this case, then we should update the BE first to decide the behavior.

mkhutornyi commented 8 months ago

πŸŽ€ πŸ‘€ πŸŽ€ to confirm with engineer about the expected behavior

melvin-bot[bot] commented 8 months ago

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

dylanexpensify commented 8 months ago

@marcochavezf to review!

marcochavezf commented 8 months ago

I looked at the backend and found out we are in the process of deprecating participantAccountIDs and visibleChatMemberAccountIDs in favor of participants. So we should use participants instead when possible. @mkhutornyi @bernhardoj @abzokhattab

abzokhattab commented 8 months ago

thank you @marcochavezf for the update...

i have updated the proposal according to the last comment please have a look and let me know.

bernhardoj commented 8 months ago

Hmm, I can't reproduce this anymore. Now, the created group chat has a notificationPreference of always and the visibleChatMemberAccountIDs is not empty. Either it's a new BE bug or it's the new expected behavior. (or is it only me?)

abzokhattab commented 8 months ago

the issue still occurs

image
bernhardoj commented 8 months ago

My bad, I created a group with existing users. I have updated my proposal too based on @marcochavezf comment.

The only thing left is the notificationPreference expected behavior. Can you confirm whether we should update the report notificationPreference from hidden to always when doing a split bill in a group? (we did this when adding a comment on both BE and FE).

mananjadhav commented 7 months ago

Taking this over as per message

marcochavezf commented 7 months ago

Assigning you @mananjadhav as C+

dylanexpensify commented 7 months ago

thanks @mananjadhav!

marcochavezf commented 7 months ago

Hi @mananjadhav, can you review the updated proposals?

dylanexpensify commented 7 months ago

Bump @mananjadhav πŸ™‡β€β™‚οΈ

mananjadhav commented 7 months ago

I just reviewed the whole conversation.

Can you confirm whether we should update the report notificationPreference from hidden to always when doing a split bill in a group? (we did this when adding a comment on both BE and FE).

@marcochavezf Can you please confirm this behavior

Will then shortlist one of the posted proposals.

marcochavezf commented 7 months ago

If we're changing the notification preference when adding a comment, then yes. It makes sense to have consistent behavior when an action happens in a group, so the participants can be aware of the conversation happening about the money requested.

melvin-bot[bot] commented 7 months ago

@mananjadhav, @marcochavezf, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

dylanexpensify commented 7 months ago

@mananjadhav bump!

mananjadhav commented 7 months ago

Checking in 2-3 hours after a PR review.

dylanexpensify commented 7 months ago

@mananjadhav let us know if you have an update or if you need some help! πŸ™‡β€β™‚οΈ

mananjadhav commented 7 months ago

I just need to test both the proposals and update. I'll do that today. Apologies for the delay.

mananjadhav commented 7 months ago

Thanks for the patience. I think @bernhardoj's proposal looks good.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed.

melvin-bot[bot] commented 7 months ago

Current assignee @marcochavezf is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

dylanexpensify commented 7 months ago

@marcochavezf to review! All good @mananjadhav!

dylanexpensify commented 7 months ago

@marcochavezf will review today!

dylanexpensify commented 7 months ago

Mel look above

melvin-bot[bot] commented 7 months ago

@mananjadhav, @marcochavezf, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

marcochavezf commented 7 months ago

Assigning @bernhardoj πŸš€

melvin-bot[bot] commented 7 months ago

πŸ“£ @bernhardoj πŸŽ‰ 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 πŸ“–

bernhardoj commented 7 months ago

PR is ready

cc: @mananjadhav