[$1000] Users list not showing on split confirm

amyevans 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!

This is a follow up issue from https://github.com/Expensify/App/pull/17349#issuecomment-1537823597

Action Performed:

Open any group chat on web app
Click + icon > Split bill
Copy url from address bar (url should be like this format: http://localhost:8080/split/new/xxx)
Open desktop app
Paste url in new tab and open desktop app from popup
Enter any amount and click Next

Expected Result:

User list is present

Actual Result:

User list is missing

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?

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: Reproducible in staging?: Reproducible in production?: 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

https://github.com/Expensify/App/assets/96077027/212e466f-79c8-4663-93a0-18afffef70d3

Expensify/Expensify Issue URL: Issue reported by: @aimane-chnaif Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0108aaabc2bc8e4aeb
Upwork Job ID: 1665886572966985728
Last Price Increase: 2023-06-06

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @miljakljajic (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)

[ ] 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

amyevans commented 1 year ago

@jczekalski please comment on the issue so I can assign you

jczekalski commented 1 year ago

@amyevans

melvin-bot[bot] commented 1 year ago

@amyevans, @miljakljajic, @jczekalski, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

miljakljajic commented 1 year ago

Apologies - I have been at EC3 so only just getting to this today. I am OOO reapplying the label so that we can reproduce this - can we get some more detailed reproduction steps for the next BZ member @amyevans - I have tried to reproduce this using your comment above but I'm not able to. Can you break it down step by step for the next person?

I'll assign it to you for now and then can you reapply the label when you've updated the steps? Thank you!

kowczarz commented 1 year ago

Hey, I've also tried to reproduce it and I didn't manage to do so.

melvin-bot[bot] commented 1 year ago

📣 @kowczarz! 📣 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:

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.
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.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

amyevans commented 1 year ago

@aimane-chnaif are you able to clarify reproduction steps for this issue? Otherwise I'm going to close it out. Thanks!

amyevans commented 1 year ago

FYI: I'll be OOO tomorrow and Monday.

melvin-bot[bot] commented 1 year ago

@amyevans, @jczekalski, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

amyevans commented 1 year ago

Closing for now due to lack of reproducible steps, we can reopen if needed

aimane-chnaif commented 1 year ago

@amyevans sorry missed this one. Here's clear reproducible step:

Open any group chat on web app
Click + icon in composer > Split bill
Copy url from address bar (url should be like this format: http://localhost:8080/split/new/xxx)
Open desktop app (logout if already logged in)
Paste url in new tab and open desktop app from popup
Enter any amount and click Next

https://github.com/Expensify/App/assets/96077027/212e466f-79c8-4663-93a0-18afffef70d3

amyevans commented 1 year ago

Thanks @aimane-chnaif, I was able to reproduce. @kowczarz I'm going to assign you per previous direction from @jczekalski. Can you confirm you can reproduce now also?

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @mallenexpensify (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)

[ ] 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

amyevans commented 1 year ago

Reapplied the Bug label to draft a BZ team member to handle reporter bonus payout (when the time comes).

kowczarz commented 1 year ago

Yep I managed to reproduce the issue.

kowczarz commented 1 year ago

I've managed to find the root of the problem, I'm preparing a proposal for it.

melvin-bot[bot] commented 1 year ago

@amyevans @mallenexpensify @kowczarz @aimane-chnaif this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0108aaabc2bc8e4aeb

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

@kowczarz any luck with the proposal? Added External to create the Upwork job.

kowczarz commented 1 year ago

Proposal

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

Users list not showing on split confirm

What is the root cause of that problem?

In (MoneyRequestModal we are setting state)[https://github.com/Expensify/App/blob/main/src/pages/iou/MoneyRequestModal.js#L107-L111], that is acquired from onyx, but we are not setting any dependencies arrays to the local state, so if onyx state changes, the local state doesn't update.

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

We can add effect that will update the state if report or personalData props are updated. That will cause that as soon as users list will be fetched it will cause a re-render and it will be displayed on split confirm.

What alternative solutions did you explore? (Optional)

NA

aimane-chnaif commented 1 year ago

Proposal looks good. After @amyevans confirms, we can move to PR.

amyevans commented 1 year ago

Proposal looks good @kowczarz 👍

melvin-bot[bot] commented 1 year ago

@amyevans, @mallenexpensify, @kowczarz, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 year ago

@amyevans, @mallenexpensify, @kowczarz, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 year ago

@amyevans, @mallenexpensify, @kowczarz, @aimane-chnaif 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

amyevans commented 1 year ago

The PR is awaiting review from @aimane-chnaif 🙏

aimane-chnaif commented 1 year ago

reviewing in an hr

melvin-bot[bot] commented 1 year ago

@amyevans, @mallenexpensify, @kowczarz, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

mallenexpensify commented 1 year ago

@kowczarz and @aimane-chnaif , can you provide an update? Is the issue not reproducible?

aimane-chnaif commented 1 year ago

@mallenexpensify I confirmed that this is fixed while refactoring money request pages and our PR can be safely closed. We can close this issue. I might be eligible for compensation according to doc.

melvin-bot[bot] commented 1 year ago

@amyevans, @mallenexpensify, @kowczarz, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@amyevans, @mallenexpensify, @kowczarz, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

mallenexpensify commented 1 year ago

Thanks @aimane-chnaif , for auditing purpose, if you think you are due payment, can you please provide reasoning and and what amount you believe you're due? Please use 25%, 50% or 100% for determining the amount. Thx

melvin-bot[bot] commented 1 year ago

Current assignee @amyevans is eligible for the Engineering assigner, not assigning anyone new.

aimane-chnaif commented 1 year ago

reason: reviewed & approved proposal, PR created, reviewed PR. I am not sure about %. I will defer to you @mallenexpensify. I think doc is when contributor assigned but agency issue has a bit different process? Also eligible for reporting.

mallenexpensify commented 1 year ago

@aimane-chnaif I don't see a 🎀 above, are C+ still using those? It makes it easier to find approved proposals.

I think compensation should either be 50 or 100% for C+, @amyevans , since you reviewed the PR, which do you think?

aimane-chnaif commented 1 year ago

I don't see a 🎀 above, are C+ still using those

yes still using. I didn't 🎀 here because agency and engineer were assigned already so it was clear who to raise PR and who to review.

amyevans commented 1 year ago

@mallenexpensify I think 50 makes sense here (plus the reporter bonus), there was certainly some effort involved but the PR didn't get through full review/testing prior to closing and there was only one proposal to evaluate as opposed to multiple

mallenexpensify commented 1 year ago

Thanks @amyevans, @aimane-chnaif , can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01ef8163337d32b284

mallenexpensify commented 1 year ago

@aimane-chnaif paid $750, thanks!

Expensify / App