App icon indicating copy to clipboard operation
App copied to clipboard

[$500] IOU/LHN - IOU not been pinned by default

Open lanitochka17 opened this issue 1 year ago β€’ 58 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.3.99-0 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:

Precondition: user should be signed in

  1. Open app
  2. Create IOU money request with any user
  3. Navigate back to LHN
  4. Observe the IOU report position in LHN and its icons

Expected Result:

Step 4: Transaction thread report is pinned by default

Actual Result:

Step 4: Transaction thread report isn't pinned by default

Workaround:

Unknown

Platforms:

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

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/02ee1e05-17a3-4d1d-b1e0-22c8751cdb10

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0159bd7d65d5681c02
  • Upwork Job ID: 1724881721908154368
  • Last Price Increase: 2023-11-29
  • Automatic offers:
    • dukenv0307 | Contributor | 28037472
Issue OwnerCurrent Issue Owner: @neil-marcellini

lanitochka17 avatar Nov 15 '23 20:11 lanitochka17

Job added to Upwork: https://www.upwork.com/jobs/~0159bd7d65d5681c02

melvin-bot[bot] avatar Nov 15 '23 20:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 15 '23 20:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 15 '23 20:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 15 '23 20:11 melvin-bot[bot]

Test Rail: https://expensify.testrail.io/index.php?/tests/view/3974494&group_by=cases:section_id&group_order=asc&group_id=229063

lanitochka17 avatar Nov 15 '23 20:11 lanitochka17

Proposal

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

When requesting money in a conversation, the latter should be pinned.

What is the root cause of that problem?

IOU.requestMoney does not pin the report.

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

I added the following line to IOU.requestMoney :

Report.notifyNewAction(chatReport.reportID, payeeAccountID);

raphaelppp avatar Nov 15 '23 20:11 raphaelppp

Proposal

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

The problem is that the IOU request is not automatically pinning the conversation after it is made

Also the context menu containing the pin option but this would be resolved by having the IOU request return isPinned true.

What is the root cause of that problem?

Essentially the togglePinnedState is not being called in the current IOU.js RequestMoney function.

Report.togglePinnedState(chatReport.reportID, false);

additionally the context menu issue would be resolved by this fix because it would now be returning isPinnedChat flag as true

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

Incorporating a Report.TogglePin on the already existing IOU.RequsetMoney function should return this to normalcy

What alternative solutions did you explore? (Optional)

Alternatively the actual backend for the requestMoney could perform a similar action and just handle this but this may be this may not be the most simple, predictable and trackable solution in the current setup

riqts avatar Nov 15 '23 21:11 riqts

πŸ“£ @riqts! πŸ“£ 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. Make sure you've read and understood the contributing guidelines.
  2. 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.
  3. 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.
  4. 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>

melvin-bot[bot] avatar Nov 15 '23 21:11 melvin-bot[bot]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~014853d868137150b0

riqts avatar Nov 15 '23 21:11 riqts

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] avatar Nov 15 '23 21:11 melvin-bot[bot]

This looks like a backend issue. IOU details report should have isPinned: true when creating, but it isn't now

s-alves10 avatar Nov 15 '23 23:11 s-alves10

Proposal

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

  1. Transaction thread report isn't pinned by default

What is the root cause of that problem?

  1. BE return transaction thread report with isPinned as false. When we build optimisticData for transaction thread report, we don't set isPinned as true by default

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

  1. BE should return transaction thread report with isPinned as true. When we build optimisticData for transaction thread report, we also need to set isPinned as true

What alternative solutions did you explore? (Optional)

dukenv0307 avatar Nov 16 '23 04:11 dukenv0307

Few proposals ready for you to review @Santhosh-Sellavel

bfitzexpensify avatar Nov 20 '23 22:11 bfitzexpensify

Sorry @bfitzexpensify Not sure how I missed this. I've unassigned newly assigned issues due to low bandwidth somehow missed it. If any C+ can review this immediately please Reassign. Otherwise I can look into this tomorrow.

Santhosh-Sellavel avatar Nov 21 '23 18:11 Santhosh-Sellavel

All good! Tomorrow is fine.

bfitzexpensify avatar Nov 21 '23 21:11 bfitzexpensify

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Nov 22 '23 16:11 melvin-bot[bot]

Sorry again for the delay here

@lanitochka17 I'm confused here about Expected Result.

Step 4: created IOU report should be pinned by default in LHN as mentioned in the TestRail case steps

Are you referring to each IOU Request thread that should be pinned or each report Can you share me test rail link/ID

Steps 7 and 11: the pinned option should not be displayed in the context menu on IOU report details page

Again I'm why should pinned option shouldn't be displayed? What do you mean by pinned option here, showing option both pin/unpin options?

As @dukenv0307 explains here, are these following issues?

  1. Transaction thread report isn't pinned by default
  2. Transaction thread report has pin option

Santhosh-Sellavel avatar Nov 23 '23 13:11 Santhosh-Sellavel

@bfitzexpensify Can you please confirm the bug and the expected behavior or add Engineering label to add a internal to confirm?

dukenv0307 avatar Nov 24 '23 05:11 dukenv0307

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

melvin-bot[bot] avatar Nov 27 '23 13:11 melvin-bot[bot]

bump @lanitochka17 cc: @bfitzexpensify

Santhosh-Sellavel avatar Nov 27 '23 13:11 Santhosh-Sellavel

@Santhosh-Sellavel Test Rail: https://expensify.testrail.io/index.php?/tests/view/3974494&group_by=cases:section_id&group_order=asc&group_id=229063

lanitochka17 avatar Nov 28 '23 17:11 lanitochka17

@bfitzexpensify @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!

melvin-bot[bot] avatar Nov 29 '23 12:11 melvin-bot[bot]

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Nov 29 '23 16:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 30 '23 23:11 melvin-bot[bot]

Thanks for the test rail. @lanitochka17 was this test passing earlier, when it got added.

@bfitzexpensify Can you check and provide us clear expected OP. From test rail I found this internal issue link https://github.com/Expensify/Expensify/issues/298942 but I don't have access to this was this a regression if so please provide more details from which issue this got added, thanks!

Santhosh-Sellavel avatar Dec 01 '23 17:12 Santhosh-Sellavel

@Santhosh-Sellavel OK, these are the TestRail steps:

  1. Create a IOU (or use an existing IOU conversation)
  2. Verify that IOU conversations (reports) are always pinned by default in the LHN
  3. Right-click on the IOU conversation (the one with a green dot) in the LHN
  4. Verify that there is no pin option on the context Menu

So @dukenv0307's identified issues here are correct:

  1. Transaction thread report isn't pinned by default
  2. Transaction thread report has pin option

Does that help clarify things? I also updated the actual/expected behaviours in the OP.

bfitzexpensify avatar Dec 01 '23 18:12 bfitzexpensify

@bfitzexpensify Can we unpin & pin iou report or not? Is it pinned always?

Santhosh-Sellavel avatar Dec 01 '23 18:12 Santhosh-Sellavel

Pinned always, so it shouldn't be possible to unpin

bfitzexpensify avatar Dec 01 '23 19:12 bfitzexpensify

Thanks then @dukenv0307's proposal looks good to me. Can you apply an internal label as we need a BE fix for this first?

C+ Reviewed πŸŽ€ πŸ‘€ πŸŽ€

Santhosh-Sellavel avatar Dec 04 '23 18:12 Santhosh-Sellavel

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

melvin-bot[bot] avatar Dec 04 '23 18:12 melvin-bot[bot]