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
2.99k stars 2.5k forks source link

[Search v1] Opening transactions on search page can lead to not found page #41578

Open luacmartins opened 2 weeks ago

luacmartins commented 2 weeks 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: 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 Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

Action Performed:

  1. Open the search page, Profile > Troubleshoot > New Search Page
  2. Click on a transaction
  3. Verify that you see a not found page

Note: this seems to only happen for older transactions that don't have a transaction thread created since the value for transactionThreadReportID is 0.

Expected Result:

We should create the transaction thread and let users open the transaction

Actual Result:

Not found page is shown

Workaround:

N/A

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/22219519/ed64830c-ca4d-4ba7-b95e-c066fe5d6084 Screenshot 2024-05-03 at 8 25 40 AM

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b78bbfebf5ef557c
  • Upwork Job ID: 1786400627125800960
  • Last Price Increase: 2024-05-03
Issue OwnerCurrent Issue Owner: @grgia
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @Santhosh-Sellavel (Internal)

grgia commented 2 weeks ago

What we chatted about before: Create a command - OpenTransactionThread (maybe OpenSearchItemPreview if we later need to add support for previewing reports? ) That calls auth.transaction.createTransactionThread if it doesn't already exist

https://github.com/Expensify/App/blob/b475b28d1319dabf2ae11eb474ceebf2226119c4/src/components/Search.tsx#L68

Use the loading indicator on the page

jliexpensify commented 1 week ago

Not overdue. Should we switch to Weekly to stop the pings?

melvin-bot[bot] commented 1 week ago

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

luacmartins commented 1 week ago

I think we might need a new command as @grgia described above, since calling openReport wouldn't be enough because we might not have a lot of the data needed, e.g. reportActions, parentReportActions, participant list, etc

luacmartins commented 1 week ago

@jliexpensify I think we should leave this as daily, since it's high priority

Kicu commented 1 week ago

@luacmartins @grgia I did some testing and in my experience the problem is almost exclusively that a lot of items simply have reportID set to 0 and so we try to open route with 0 inside and that gets us the "Not Found" page - which is correct behavior.

A lot of other items that looked legit opened correctly:

https://github.com/Expensify/App/assets/3929868/eae7796a-59fc-4bd7-b4c2-cf65d8ef115c

Sorry if I'm misunderstanding something - can't this be fixed in api by simply always returning some existing reportID?

luacmartins commented 1 week ago

You understood it right. This happens because in the past we didn't create these threads for all reports, so they might not exist and we end up returning 0. The solution is to create a new command that will create the thread if it doesn't exist.

luacmartins commented 4 days ago

Still looking for volunteers. @grgia are you interested in working on this one?

lakchote commented 3 days ago

You understood it right. This happens because in the past we didn't create these threads for all reports, so they might not exist and we end up returning 0. The solution is to create a new command that will create the thread if it doesn't exist.

It should be fixed by https://github.com/Expensify/Auth/pull/10456. We should create threads automatically from now on, even for old preexisting transactions.

trjExpensify commented 3 days ago

Nice, @lakchote! That's on prod, so we can retest then, yeah? If so, @grgia @Kicu did you have some transactions in your account that definitely weren't working before to validate @lakchote's theory?

Kicu commented 3 days ago

@trjExpensify @lakchote

For me on dev every row that "looks" normally is correctly opening a report/IOU in RHP. However every row with __fake__ field has no transactionID and opens error page. I have no idea what these rows are and where do they come from.

Seems there is an issue for this: https://github.com/Expensify/App/issues/41985

// edit btw Im using staging api if that changes anything

Screenshot 2024-05-15 at 13 26 52

sample object with fake

{
  accountID: 0,
  action: "view",
  amount: 55500,
  canDelete: false,
  category: "",
  comment: {
    comment: "",
  },
  created: "2024-04-29 10:13:59",
  currency: "PLN",
  hasEReceipt: false,
  managerID: 0,
  merchant: "(none)",
  modifiedAmount: 0,
  modifiedCreated: "",
  modifiedCurrency: "",
  modifiedMerchant: "",
  parentTransactionID: "",
  policyID: "",
  reportID: "-2",
  reportType: "",
  tag: "",
  transactionID: "5321901916525874903",
  transactionThreadReportID: "0",
  type: "cash",
  from: {
    accountID: 0,
    avatar:
      "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_1.png",
    displayName: "__fake__",
    firstName: "",
    lastName: "",
    login: "__fake__",
    payPalMeAddress: "",
    phoneNumber: "",
    pronouns: "",
    timezone: {
      automatic: true,
      selected: "America/Los_Angeles",
    },
    validated: false,
  },
  to: {
    accountID: 0,
    avatar:
      "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_1.png",
    displayName: "__fake__",
    firstName: "",
    lastName: "",
    login: "__fake__",
    payPalMeAddress: "",
    phoneNumber: "",
    pronouns: "",
    timezone: {
      automatic: true,
      selected: "America/Los_Angeles",
    },
    validated: false,
  },
  shouldShowMerchant: true,
  shouldShowCategory: false,
  shouldShowTag: false,
  shouldShowTax: false,
  keyForList: "5321901916525874903",
}
grgia commented 3 days ago

@trjExpensify we haven't implemented the fix for this one yet

grgia commented 3 days ago

I missed this comment https://github.com/Expensify/App/issues/41578#issuecomment-2111774637 let's see

grgia commented 3 days ago

@lakchote the chronos expenses are making this hard to test since those won't open / take up all of my results

https://github.com/Expensify/App/assets/38015950/dcae9f9f-4ebb-4107-b970-11d800c60801

trjExpensify commented 3 days ago

Cool yeah, we have an issue for expenses created via Track expense showing as FAKE #41985

As for Chronos, are we going to hide those entries or something? They shouldn't be in the expenses filter really.

melvin-bot[bot] commented 1 day ago

@luacmartins @jliexpensify @grgia @Santhosh-Sellavel 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!