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

[$500] mWeb-Conversation- Green line is shown creating request money in DM but not shown while split money #29376

Closed izarutskaya closed 9 months ago

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


Version Number: 1.3.81-4

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

Expensify/Expensify Issue URL:

Issue reported by: Applause-Internal Team

Slack conversation: @

Action Performed:

  1. Go to https://staging.new.expensify.com/

  2. Tap on any 1:1 report

  3. Tap plus icon and select request money

  4. Enter amount and complete the request flow

  5. Tap on IOU request created

  6. Tap on header from link

  7. Note the green line above IOU request created

  8. Navigate back to LHN

  9. Tap any group chat

  10. Tap plus icon and select split bill

  11. Enter an amount and complete split money

  12. Tap on split money transaction

  13. Navigate back to split money created page

Expected Result:

A green line is shown on creating request money in DM but not shown while creating split money in group page and there should not be this inconsistent behaviour.

Actual Result:

A green line is shown on creating request money in DM but not shown while creating split money in group page inconsistently.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Android: Native
Android: mWeb Chrome https://github.com/Expensify/App/assets/115492554/926b0277-6c33-464b-9566-5aacfde2719d
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01dd10227ee3f8eac1
  • Upwork Job ID: 1712197891396800512
  • Last Price Increase: 2023-11-08
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01dd10227ee3f8eac1

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

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

teneeto commented 1 year ago

Hi, I'm Eto from Callstack - expert contributor group - and I would like to take a look at this issue.

mountiny commented 1 year ago

@gedu @MonilBhavsar this is related to the unread marker, would you want to take care of this

melvin-bot[bot] commented 1 year ago

📣 @Santhosh-Sellavel Please request via NewDot manual requests for the Reviewer role ($500)

MonilBhavsar commented 1 year ago

Thanks! Following this and looking

teneeto commented 1 year ago

Hi @MonilBhavsar, are you looking into this? or just following the progress 😉

melvin-bot[bot] commented 1 year ago

@strepanier03, @teneeto, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

strepanier03 commented 1 year ago

Finished testing, sorry for the delay on that.

I'll let @MonilBhavsar weigh in, I'm not sure the plan there.

teneeto commented 1 year ago

I'll drop a proposal on this, hopefully today

MonilBhavsar commented 1 year ago

Teeneto sorry for confusion, If you have proposal, please share. Happy to manage the issue

MonilBhavsar commented 1 year ago

Waiting for proposals here

teneeto commented 1 year ago

Hi Monil, Apologies for the delay, There's just a little more check i need to do before posting a proposal here. I'll keep you posted as soon as I can. Thnaks

MonilBhavsar commented 1 year ago

Thank you for the update!

MonilBhavsar commented 1 year ago

Same, awaiting proposal

MonilBhavsar commented 1 year ago

Melvin, I commented above

teneeto commented 1 year ago

Hi Monil, again apologies, for the time delay on this, had to understand a few things about the flow with @gedu

teneeto commented 1 year ago

@MonilBhavsar Before I drop a proposal here, there are two possible solutions to enforce consistency; From the expected result of this issue, it isn't clear if we should allow new (the Unread Marker Indicator) for both request money and split bill or we should remove it?

MonilBhavsar commented 1 year ago

The marker appearing while requesting money on the user who is requesting the money looks like a bug and we should remove it. User creating an action/message should not see the marker

melvin-bot[bot] commented 1 year ago

@strepanier03 @teneeto @MonilBhavsar @Santhosh-Sellavel 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!

MonilBhavsar commented 1 year ago

We're waiting for proposal here @teneeto you might also want to confirm that the root cause of the issue is different from the issue that this PR fixes https://github.com/Expensify/App/pull/29860

MonilBhavsar commented 1 year ago

Same^

melvin-bot[bot] commented 1 year ago

@strepanier03 @teneeto @MonilBhavsar @Santhosh-Sellavel this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

Santhosh-Sellavel commented 1 year ago

Waiting for proposal

teneeto commented 1 year ago

Hey, I'm still working on this with help from @gedu.

MonilBhavsar commented 1 year ago

Thanks! Feel free to post any questions here. Happy to help

MonilBhavsar commented 1 year ago

Flagged over weekend, but awaiting proposal

teneeto commented 1 year ago

Hi @MonilBhavsar, This needed some critical thoughts and considerations from my part and assistance from colleagues (@roksanaz and @gedu ) as well - really didn't seem to be a big deal at first, but have finally taken a lot of time, Apologies.

Here is my Observation so far - basically chopping from @roksanaz 😉!

I think there's not enough data to distinguish if the last request was made by the logged in person or the other person. The thing here is Split Bill and Request Money use distinct Previews and really not same since request money is more thread-like. When you Request Money, you have all requests in a summary (report review) and each request isn't really displayed as a separate message, hence the inconsistency. Screenshot 2023-11-06 at 16 56 08 The summary message (kind of a thread thing, marked red on the screenshot) is a report review

Possible suggestions!

  1. Don't show green line for report review at all
  2. Add the information about last IOU's author's on the BE side, so that FE can consume it and add a new condition to the shouldDisplayNewMarker method, something like:
if (reportAction.actionName === 'REPORTPREVIEW' && reportAction.lastActorAccountID === Report.getCurrentUserAccountID()) {
    return shouldDisplay // it's false at this stage
}

Let me know what you think? I can proceed with either approach, and I'm open to your suggestions too. Thanks and again apologies to my slow attack to this issue.

melvin-bot[bot] commented 1 year ago

@strepanier03 @teneeto @MonilBhavsar @Santhosh-Sellavel 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 @Santhosh-Sellavel is eligible for the Internal assigner, not assigning anyone new.

MonilBhavsar commented 1 year ago

That's a great question!

Don't show green line for report review at all

I don't think we should do this as Request Money is one of the core feature of the App and we want user to be notified or aware of new transaction is associated with IOU report

Add the information about last IOU's author's on the BE side, so that FE can consume it and add a new condition to the shouldDisplayNewMarker method, something like:

I like this approach. I can look into this today and add that property 👍

Thanks for looking!

teneeto commented 1 year ago

Thank you @MonilBhavsar

strepanier03 commented 1 year ago

All good Melvin, things are moving forward.

teneeto commented 1 year ago

Hi @MonilBhavsar, just a warm follow-up here. 😉

MonilBhavsar commented 1 year ago

We discussed approach internally and I hope to get this live by early next week

MonilBhavsar commented 1 year ago

Adding Reviewing label as we need two PR's to be deployed in order

teneeto commented 12 months ago

internally

Ayye, nice... Although the link didn't work for me.

melvin-bot[bot] commented 12 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 11 months ago

@strepanier03, @teneeto, @MonilBhavsar, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MonilBhavsar commented 11 months ago

PR should be on staging tonight and we'll be able to test this proposal https://github.com/Expensify/App/issues/29376#issuecomment-1801509543

MonilBhavsar commented 11 months ago

@teneeto On staging app, for money preview actions, you should be able to view childLastActorAccountID property in Onyx DB. You can test this proposal now https://github.com/Expensify/App/issues/29376#issuecomment-1801509543

teneeto commented 11 months ago

Nice let me check and try it out, nice work @MonilBhavsar

MonilBhavsar commented 11 months ago

Awesome, thanks!! 😄 Let me know how it goes and If I could help

melvin-bot[bot] commented 10 months ago

This issue has not been updated in over 15 days. @strepanier03, @teneeto, @MonilBhavsar, @Santhosh-Sellavel eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

MonilBhavsar commented 10 months ago

We can close this. @Santhosh-Sellavel needs to be paid for C+ review. Are you paid through newDot?

Santhosh-Sellavel commented 10 months ago

@strepanier03 Can I get a payment summary message?

strepanier03 commented 9 months ago

Sure thing @Santhosh-Sellavel, sorry for the hold up. in the future if these can be switched to Daily when payment is needed that would help because with it on Monthly I'm not likely to look at this until it goes overdue.

strepanier03 commented 9 months ago

Payment Summary: