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

[$1000] Web - Sidebar re-arrange on moving to report from split group #23905

Closed kbecciv closed 1 year ago

kbecciv 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. Split a bill
  2. Add 3 users
  3. Create the bill
  4. Observe the sidebar
  5. Click on the bill in the split group
  6. Click on any profile in split with section in the sidebar
  7. Select message [name]
  8. Observer the pattern
  9. You can repeat from step 5 for other users

Expected Result:

When clicking on any report, it's position in LHN should not change until data has been added to the compose box.

Actual Result:

The reports in LHN changes it's indexes.

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.47.3 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

https://github.com/Expensify/App/assets/93399543/88d3a3e0-2931-44e7-b395-5e6f521ec8c1

https://github.com/Expensify/App/assets/93399543/eabdaf38-c31a-4015-be63-8f1f3b61be50

Expensify/Expensify Issue URL: Issue reported by: @krishna2323 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690663983822269

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01005e4b179e77b28f
  • Upwork Job ID: 1694361641774350336
  • Last Price Increase: 2023-08-23
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)

jeet-dhandha commented 1 year ago

Proposal

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

What is the root cause of that problem?

  1. Let say we have A (Logged in user), B and C users in a group.
  2. Now we created a split bill with those three users, the lastVisibleActionCreated property of group's report is updated but not of the individual user's report of B and C.
  3. Now we open the B's individual report from particpants list and after that the lastVisibleActionCreated property of B's report is updated.
  4. Same for C's Report.

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

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 year ago

@mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@mallenexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 1 year ago

@mallenexpensify Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

mallenexpensify commented 1 year ago

Was off for few days and swamped with a project deadline, will get to this soon

melvin-bot[bot] commented 1 year ago

@mallenexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 1 year ago

@mallenexpensify 10 days overdue. Is anyone even seeing these? Hello?

melvin-bot[bot] commented 1 year ago

@mallenexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 1 year ago

@mallenexpensify 10 days overdue. I'm getting more depressed than Marvin.

melvin-bot[bot] commented 1 year ago

@mallenexpensify 12 days overdue now... This issue's end is nigh!

mallenexpensify commented 1 year ago

The reports in LHN should not change its position on moving to someone's profile. @Krishna2323 efore I updated the expected behaviour to be more broad.
When clicking on any report, it's position in LHN should not change until data has been added to the compose box.

  1. I wonder why this is specifically happening with reports from split bills
  2. I wonder if it's happening anywhere else.
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01005e4b179e77b28f

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

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

mallenexpensify commented 1 year ago

Removed/readded the Bug label to hopefully reset the timer since I didn't address this for weeks and I don't think it needs to be/go internal

melvin-bot[bot] commented 1 year ago

@mallenexpensify @abdulrahuman5196 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

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

abdulrahuman5196 commented 1 year ago

This only became external couple of days back. This shouldn't be internal ideally. But anyways will review the only proposal we have and update.

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

mallenexpensify commented 1 year ago

@abdulrahuman5196 , please review @jeet-dhandha 's proposal when you have a minute https://github.com/Expensify/App/issues/23905#issuecomment-1657598424

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

@MitchExpensify I'm off this week, can you please keep 👀 on this then I'll snag it back on Monday? Thx

MitchExpensify commented 1 year ago

Can do @mallenexpensify

mallenexpensify commented 1 year ago

@abdulrahuman5196 , please review @jeet-dhandha 's proposal when you have a minute https://github.com/Expensify/App/issues/23905#issuecomment-1657598424

Thanks @MitchExpensify for 👀

abdulrahuman5196 commented 1 year ago

Hi, @jeet-dhandha.

I am not able to understand both the root cause and the solution from proposal. Could you provide more detailed information on the same?

jeet-dhandha commented 1 year ago

I will get back by tomorrow. As i also forgot what the solution was as its been more than 1.5 month.

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

abdulrahuman5196 commented 1 year ago

We are not having any more proposals to review. @jeet-dhandha needs to update proposal

jeet-dhandha commented 1 year ago

Proposal Updated https://github.com/Expensify/App/issues/23905#issuecomment-1657598424. Sorry for delay.

mallenexpensify commented 1 year ago

Thanks. @abdulrahuman5196 , can you review @jeet-dhandha 's updated proposal above?

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

Reassigning C+ due to inactivity @parasharrajat please review https://github.com/Expensify/App/issues/23905#issuecomment-1657598424

parasharrajat commented 1 year ago

Reviewing in 2 hours.

parasharrajat commented 1 year ago

So I checked and @jeet-dhandha's proposal looks correct. We will have to make changes from both the front end and back end. It might be possible that we don't have to change the backend.

But I am not an expert on this subject either. It would be great to have it reviewed by an internal team member.

I am approving to assign an Engineer and then we can discuss the proposal further. @jeet-dhandha Do you mind adding more detail to the solution to explain how will achieve it?

I would suggest that you repost a fresh proposal so that we don't have to scroll all the way back.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 1 year ago

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

jeet-dhandha commented 1 year ago

I will need some time to create a full proposal as all the time related implementation is a bit hard to understand so trying to figure out the best way to handle this. 👍

jeet-dhandha commented 1 year ago
Screenshot 2023-09-27 at 12 42 34 AM

How should all the chats be ordered after the split bill is created and rest IOU Reports or Chat Reports are created or updated?

pecanoro commented 1 year ago

Hmmm I think this might be on purpose since they get ordered from last seen, but I will check.

jeet-dhandha commented 1 year ago

https://github.com/Expensify/App/assets/78416198/db43fcdc-889e-4ee8-99ff-584d7a98609e

Also the timestamp is updated from backend instead of using the sent lastVisibleActionCreated.

SENT - ONYX Data

"lastVisibleActionCreated": "2023-09-27 03:24:13.036", => GROUP (report_6606139833156882)
"lastVisibleActionCreated": "2023-09-27 03:24:13.038", => CHAT (report_7926651244820297)
"lastVisibleActionCreated": "2023-09-27 03:24:13.037", => IOU (report_5105409794206698)
"lastVisibleActionCreated": "2023-09-27 03:24:13.041", => CHAT (report_7690011831439707)
"lastVisibleActionCreated": "2023-09-27 03:24:13.040", => IOU (report_1974921079093219)

RECEIVED - ONYX Data

"lastVisibleActionCreated": "2023-09-27 03:24:13.345", => IOU (report_5105409794206698) 
"lastVisibleActionCreated": "2023-09-27 03:24:13.347", => IOU (report_1974921079093219)
"lastVisibleActionCreated": "2023-09-27 03:24:13.343", => GROUP (report_6606139833156882)
pecanoro commented 1 year ago

I don't see any bugs here, I think this is working just fine. @mallenexpensify Do you have anything against closing this one?

jeet-dhandha commented 1 year ago

@pecanoro The above video is after the fix i added for incremental “lastVisibleActionCreated”.

jeet-dhandha commented 1 year ago

As we need incremental timestamp and also we need to stop updating “lastReadTime” to current time as its making Reports marked as read instead of showing in “bold”

pecanoro commented 1 year ago

As we need incremental timestamp and also we need to stop updating “lastReadTime” to current time as its making Reports marked as read instead of showing in “bold”

I see them bold unless I open them.

jeet-dhandha commented 1 year ago

Please test same thing on “main” the IOU reports are not highlighted (gets bold).