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.53k stars 2.88k forks source link

[$500] Expense report name does not match the actual report ID #46410

Closed m-natarajan closed 3 weeks ago

m-natarajan commented 3 months 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: 9.0.13-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 Expensify/Expensify Issue URL: Issue reported by: @dangrous Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1720548953850039

Action Performed:

  1. create a workspace in OldDot with daily submit
  2. Create an expense (via Old or NewDot) on the policy, do not add a category.
  3. Navigate to reports, enable Scheduled Submit and set "How often expenses submit:" to daily
  4. Navigate to Categories > "People must categorize expenses" and enable.
  5. Create another expense (in NewDot) with no violations (ie. you'll need to add a category).
  6. wait 24 hours for auto-submission to run
  7. concierge should create a new report and move the expense with the violation (ie. the one without a category tab) to it
  8. concierge should auto-submit the existing report

Expected Result:

After step 4 the actual report ID on the URL and report title should match

Actual Result:

The URL is https://staging.new.expensify.com/r/6415032685621410, but title is 2836679535954098 It's the same reportName as the expense report that existed prior to harvesting ran to create a new report to house the expense held back

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

image (3)

Snip - (4) New Expensify - Google Chrome (5)

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d53445b11cfe279f
  • Upwork Job ID: 1829670879168607496
  • Last Price Increase: 2024-09-13
Issue OwnerCurrent Issue Owner: @tgolen
melvin-bot[bot] commented 3 months ago

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

mallenexpensify commented 3 months ago

Updated steps in OP, testing.

melvin-bot[bot] commented 3 months ago

@mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

mallenexpensify commented 3 months ago

Adding Needs Reproduction and bumping to weekly since I don't think this bug affects real users and it was taking me a while to test.

MelvinBot commented 3 months ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

melvin-bot[bot] commented 2 months ago

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

mallenexpensify commented 2 months ago

Just tried to repro and wasn't able to after 15 mins cuz new workspace shows in olddot but not new ¯_(ツ)_/¯

mallenexpensify commented 2 months ago

Waiting 24 hours to auto-submit

image
melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

mallenexpensify commented 2 months ago

Reproduced! (any bug that requires me to wait a day is extra-hard)

image

@situchan , plz attempt reproduction and comment if you don't think this can be external (I initially thought it could but now I'm second guessing myself)

situchan commented 2 months ago

reproduced

image

I think we can look for proposals

situchan commented 2 months ago

Awaiting proposals

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $500

situchan commented 1 month ago

Same ^

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @stephanieelliott (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.

mallenexpensify commented 1 month ago

@stephanieelliott I'm out til Tuesday, can you keep 👀 on plz? Thx
Posted this in the Callstack room to see if an agency dev might be able to help.

rezkiy37 commented 1 month ago

Hi, I am Michael (Mykhailo) from Callstack, an expert agency and I can work on this issue.

rezkiy37 commented 1 month ago

I'm done with the first steps and waiting 24 hours for the auto-submission to run.

Screenshot 2024-09-12 at 16 11 43
rezkiy37 commented 1 month ago

I've reproduced the bug on my side. I can confirm it exists. However, looks like it is a backend issue. It assigned another reportID to reportName. I checked and this reportID exists and is assigned properly to another expense.

Details https://github.com/user-attachments/assets/b60f6e8e-3385-4ffb-a0ba-67262b3e9d4c Screenshot 2024-09-13 at 16 40 16

We need an internal engineer here.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

situchan commented 1 month ago

🎀👀🎀

melvin-bot[bot] commented 1 month ago

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

mallenexpensify commented 1 month ago

@AndrewGable , 👀 plz above. I think the action is "confirm that you agree it's backend and provide details". After that I think you can either keep yourself assigned, if you want to work on it, or unassign and I can go 🎣 for an internal engineer.

AndrewGable commented 1 month ago

It does look like a backend issue to me, I don't think I can make progress on this in the short term, so fishing might be best!

mallenexpensify commented 1 month ago

Made internal, leaving daily and maintaining ownership of the issue. Added Hot Pick since this affects real world users and it'd be confusing if one of our customers ran across the bug.

trjExpensify commented 1 month ago

So the issue here is that the reportName for the expense report created to hold the moved expense is taking the reportName of the old expense report. Is that right?

mallenexpensify commented 1 month ago

So the issue here is that the reportName for the expense report created for the moved expense is taking the reportName of the old expense report. Is that right?

I'm unsure where the incorrect report number comes from in the title

trjExpensify commented 1 month ago

It's the same reportName as the expense report that existed prior to harvesting ran to create a new report to house the expense held back.

mallenexpensify commented 1 month ago

That would make sense @trjExpensify, I added that to the OP as part of the Actual Behaviour.

melvin-bot[bot] commented 1 month ago

@AndrewGable, @mallenexpensify, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

tgolen commented 1 month ago

I've found the code responsible for this. It turns out, it's been like this for nearly 7 years:

https://github.com/Expensify/Web-Expensify/blob/b295facc276d37bff84aca7906fc3c6e6f361d0e/script/bwm/UserHarvester.php#L243

It's clearly creating the new report and using the name from the previous report (which would include the previous report's reportID).

I believe if we really want to "fix" this, all we need to do is pass null for the report name and then it will fallback to whatever a normal default report is named based on the policy settings.

An alternative solution would be to make the name more descriptive like "Report holding violations from report #blahblah`

cc @iwiznia since you originally added this logic and if you have any thoughts on improving it.

iwiznia commented 1 month ago

I don't recall this that well (unsurprisingly). I was under the impression that if a the policy had a formula title (does it have one in this case?) then the name would be recomputed using that formula and update the name to the correct one. I checked one report randomly from these logs:

UserHarvester - Creating report to hold violations for harvesting ~~ violationsReportID: '3789728730289969' copiedFrom: '7556138432854604' Name from DB:


ionatan@db1.rno:~$ getreport.sh 3789728730289969
Report 3789728730289969 is Open
State: 0: Open Status: 0: Open
Report:
created,submitted,approved,reportID,accountID,managerID,reportName,state,total,currency,status
"2024-10-01 08:43:45",,,3789728730289969,16362953,,"Olutosin Sonuyi Expense Report 2024-10-01",0,-5117,USD,0

ionatan@db1.rno:~$ getreport.sh 7556138432854604 Report 7556138432854604 is Approved State: 2: ManualReimbursed Status: 3: Approved Report: created,submitted,approved,reportID,accountID,managerID,reportName,state,total,currency,status "2024-09-27 16:20:52","2024-10-01 09:16:14","2024-10-01 09:16:14",7556138432854604,16362953,5445868,"Olutosin Sonuyi Expense Report 2024-09-23",2,-11453,USD,3



Seems to be working as I expected in those cases.
tgolen commented 1 month ago

@mallenexpensify or @trjExpensify What do you think would be a good solution here?

  1. Come up with a better report name like Report holding violations from report #blahblah
  2. Keep the same report name as now, but just reference the new reportID Expense Report #whatever
  3. Fallback to what the policy report formula is (by default, I think it's {report:type} {report:startdate})
iwiznia commented 1 month ago

I think we first need to investigate why 3 did not happen in this case, as that's how allegedly it should work and has been for 7 years. If it did not work in this case, it's a bug.

tgolen commented 1 month ago

3 doesn't happen because when createReport() is called, the report name of $reportThatNeedsSplitting->getName() is passed (so it's using the name of the previous report, which has the report ID of the previous report).

trjExpensify commented 1 month ago

At heart, I agree with Ionatan to use the default report formula of the workspace when we create a new report. I don't think we need a special name for these kind of reports, there's also a bunch of cases to manage. I.e violations, held expenses, pending expenses, scanning expenses etc.

iwiznia commented 1 month ago

3 doesn't happen because when createReport() is called, the report name of $reportThatNeedsSplitting->getName() is passed (so it's using the name of the previous report, which has the report ID of the previous report).

The formula should recompute it, same as I shown that seems to be happening in the reports here

tgolen commented 1 month ago

Can you show me where the "recompute" is happening in the code? I don't think it does that. getName() just returns the value of the name field of the report.

mallenexpensify commented 1 month ago

At heart, I agree with Ionatan to use the default report formula of the workspace when we create a new report

Agree with Tom, if we can get this working with the expected behaviour (I hope) we should be good here.

iwiznia commented 1 month ago

Can you show me where the "recompute" is happening in the code? I don't think it does that. getName() just returns the value of the name field of the report.

Yeah, it is not recomputing it on create, it's done in here and (I think) its call to this

tgolen commented 1 month ago

OK, thanks! I want to dig into that further to understand it better. Because if that's true, I agree with you that it doesn't explain why 3 didn't happen.

iwiznia commented 1 month ago

One thing you could do is clear the report cache in the auth db by CQ and have a user with access to it open it and capture the requestID when they do (or get it afterwards from logs) in order to see the recompute cache code run and get logs on the process above. Loading the report without cache would recompute it, but IIRC that does not happen in support login mode (or maybe we do it but not save it? Don't recall).

iwiznia commented 1 month ago

Alternatively you can find previous logs where we did that process, search for Recomputing cache for report and the reportID.

tgolen commented 1 month ago

I'm definitely a little lost on how and when the report name is supposed to change, but for the latest example in this issue, I did find the logs for when it's cache was recomputed: logs.

I see a call in there to save the report's cache, but the JSON data doesn't include the name field. Should it?

For this specific example, these are the two reports in question:

What I can see is that the cache for A was recomputed before the harvester put the transactions with violations onto it.

  1. Is the problem that the cache was not recomputed afterwards?
  2. Is the problem that the cache does not appear to recompute the name?
iwiznia commented 1 month ago

I see a call in there to save the report's cache, but the JSON data doesn't include the name field. Should it?

I think I was wrong and name is not saved in the cache itself, but it does get computed in this flow.

For this specific example, these are the two reports in question:

Which one is the one with the wrong name? 2354632238060824 right?

Is the problem that the cache was not recomputed afterwards?

In this case, no, since the formula does not depend on the transactions?

But now I am confused, as this is the setting of the policy 453CC61456C05536:

image

Also, I think those logs are not good because in here we skip recomputing the formulas for that authToken which is where we rename the report based on the formula.

tgolen commented 1 month ago

Which one is the one with the wrong name? 2354632238060824 right?

Yes, correct.

Also, I think those logs are not good because in here we skip recomputing the formulas for that authToken which is where we rename the report based on the formula.

Hm, I kinda get that, but I'm not sure how you concluded that those logs triggered that early return. From how I look at it, those logs are the logs from the user themself mykhailo.kravchenko@callstack.com and they would have a normal non-supportal authToken.

What I'd like to do is have the time to test this locally, and add some breakpoints in PHP to debug exactly what's happening. Otherwise, it's not worth either of our time to guess about it. I won't have time to do this until next week (a bunch of meetings today, then I'm OOO thur/fri)

iwiznia commented 1 month ago

Hm, I kinda get that, but I'm not sure how you concluded that those logs triggered that early return. From how I look at it, those logs are the logs from the user themself mykhailo.kravchenko@callstack.com and they would have a normal non-supportal authToken.

Oh, you are right, the token is harvest and not in that list of skipped token types.

melvin-bot[bot] commented 1 month ago

@tgolen, @mallenexpensify, @situchan Eep! 4 days overdue now. Issues have feelings too...

tgolen commented 1 month ago

Daily Update

Next Steps

ETA