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] [$1000] Inconsistent user count in split bill preview and above the welcome message for a group #20538

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!


Action Performed:

  1. Open the app
  2. Click on plus and click on split bill
  3. Enter any amount, select 5 or more users and click on split
  4. In the group icon, observe user count over 4th icon and in the split bill message, observe user count over 4th icon

    Expected Result:

    App should maintain consistency on how it displays user count for 5 or more users on group icon and in split bill message

    Actual Result:

    App always displays +1 count in split bill message icon then group icon for 5 or more users

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.26-1 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

Screenshot (367)

https://github.com/Expensify/App/assets/43996225/b02fe7b1-09bb-4469-b2db-e36be2b48621

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685992027991919

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018746c14ba64b431d
  • Upwork Job ID: 1668281720411238400
  • Last Price Increase: 2023-06-12
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

BhuvaneshPatil commented 1 year ago

Proposal

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

Inconsistent user count in split bill preview and above the welcome message for a group

What is the root cause of that problem?

In case of Avatars shown in Header of chat report, the participants are obtained from chat report (not IOU Report). And this report doesn't contains the the current logged in user in the list. That's why the No. of Avatars will be 1 less than original in Header. Same is the case when we create a group with multiple users. https://github.com/Expensify/App/blob/d5374be5435eea4b94978f93588ebad8ffdc0867/src/pages/home/report/ReportActionItemCreated.js#L39-L45

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

While creating the IOU, if the report is being created we can add the current user as participant. This can be done in createSplitsAndOnyxData() in IOU.js

What alternative solutions did you explore? (Optional)

Above fix can affect many places, and the bug fix is for only one place in ReportActionItemCreated. We can explicitly add the avatar/icon of current user if it's not present. We can use getIconsForParticipants() function for this.

twisterdotcom commented 1 year ago

Took me a while to figure out, but I see it:

image
melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~018746c14ba64b431d

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

BhuvaneshPatil commented 1 year ago

I have tried investigating into this, What I have observed is that if there suppose five participants - A, B, C, D, E If we view the preview from user A - BCDE will appear from user B - ACDE will appear. Hence if we try to change the logic for Onyx data's participant we can't be sure where it will affect at other places where it expects the participant remain to be the way it to be.

jasperhuangg commented 1 year ago

@BhuvaneshPatil why not when we render that component, we check if the current user is a part of the list of participants, and if not we'll know we need to render an extra circle for them, and increment the count accordingly.

BhuvaneshPatil commented 1 year ago

@jasperhuangg Yes, this will be a great addition. It will add extra layer of caution to the solution, will avoid the duplication.

aslambaba commented 1 year ago

Dear @BhuvaneshPatil,

I hope this message finds you well. I came across your Upwork listing regarding the issue you are facing, and I am eager to offer my expertise as a Full Stack Engineer to help resolve it efficiently. With over 3 years of experience in this field, I am confident that I can contribute to your team and address the problem promptly.

What's My Approach?

To address the error you mentioned, I suspect that the issue lies in the addition of the user itself, which results in an extra count. My initial approach would be to thoroughly examine how the state is being handled to identify the root cause of the problem.

I have thoroughly reviewed the Contributor Guidelines and fully agree with all the conditions mentioned therein.

Contributor details Your Expensify account email: developerswagsofficial@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0198fd1761c025c837

Best regards, Aslam Sarfraz

melvin-bot[bot] commented 1 year ago

📣 @aslambaba! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
sobitneupane commented 1 year ago

I am not sure if it is a bug or intended behavior. In group chat, we don't show current user in welcome message, chat header and details page. But in IOU preview and detail we include "Who paid for?" (payer) as well.

Screenshot 2023-06-13 at 13 21 15

cc: @jasperhuangg

twisterdotcom commented 1 year ago

@sobitneupane are you saying that in these two participants lists: https://github.com/Expensify/App/issues/20538#issuecomment-1587588204, there is actually 1 fewer at the top, than the bottom?

BhuvaneshPatil commented 1 year ago

@twisterdotcom , for report that is created when we create a split bill, for that, in report's participant list there will be -1 number of partcipants (the one not present is the current user). For report action, the split bill - there will be all participant including current user

sobitneupane commented 1 year ago

@twisterdotcom We don't include current user (the logged in user) in the group. We have been doing this from long before.

Screenshot 2023-06-13 at 21 30 01

But in Split Preview and Details, we show all the participants involved in the split including the user who paid the bill which I think is expected.

Screenshot 2023-06-13 at 21 31 43

So, I don't think it's a bug.

twisterdotcom commented 1 year ago

Hmm okay. So we show Who paid in the Split report, but we don't show them in the participants of the chat report.

I see, that is normal, but it was confusing to me. I'll ask in the thread if anybody else thinks we should creatively solve that, or not. I see the argument for closing it as expected though.

twisterdotcom commented 1 year ago

Let's see what Shawn says: https://expensify.slack.com/archives/C049HHMV9SM/p1686671859606519?thread_ts=1685992027.991919&cid=C049HHMV9SM

twisterdotcom commented 1 year ago

Okay cool. Design says close: https://expensify.slack.com/archives/C049HHMV9SM/p1686672188423959?thread_ts=1685992027.991919&cid=C049HHMV9SM.

Thanks all! Onto the next one.