Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workspace - Reimburse expenses- Rate equals 0 is displayed after offline changes on Reimbursement page #12012

Closed
1 task done
kbecciv opened this issue Oct 19, 2022 · 10 comments
Assignees
Labels
Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Oct 19, 2022

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. Go to https://staging.new.expensify.com/ and login with expensifail account
  2. Create a workspace (or it could be existing one)
  3. Go to Reimburse expenses page of the workspace
  4. Disable the internet connection
  5. Change rate and and unit values
  6. Enable internet connection
  7. Refresh the page

Expected Result:

Rate and unit values should stay the same as were chosen offline

Actual Result:

Rate value changes to 0.

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.2.18.2

Reproducible in staging?: Yes

Reproducible in production?: No

Notes/Photos/Videos: Any additional supporting documentation

Bug5784086_video_2022-10-19_16-32-04-1.mp4

Issue reported by: Applause - Internal Team

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Oct 19, 2022
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Oct 19, 2022

Triggered auto assignment to @cead22 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@thienlnam thienlnam assigned thienlnam and unassigned cead22 Oct 20, 2022
@thienlnam
Copy link
Contributor

Looks like we had a similar issue #10877 in the past

@thienlnam
Copy link
Contributor

There does seem to be something wrong with customUnits in the policy

For some reason, we have the same element keyed multiple times in customUnits

Screen Shot 2022-10-20 at 10 45 01 AM

When you make a change optimistically, it appears to only update the key under rates
Screen Shot 2022-10-20 at 10 47 25 AM

Then we you go back online, the customUnits: null
Screen Shot 2022-10-20 at 10 51 42 AM

There seems to a few things here -

  1. Why are these keyed incorrectly
  2. When we reconnect, we might be doing a SET somewhere and not fetching the policy customUnits

@thienlnam
Copy link
Contributor

thienlnam commented Oct 20, 2022

Okay, it actually seems like the keys are intentional so that is fine, though I'm unsure why there is onyxRates and rates which have the same values. I'm a little confused on how it's being set to null though. This is the order of responses that happens when you go back online after editing the custom units offline.

Screen Shot 2022-10-20 at 11 08 16 AM

Reconnect app is where the customUnits is set to null
https://github.com/Expensify/Web-Expensify/blob/fc8ad875d159f0da57f09e362aa8a6172d4be1e3/lib/AppInit.php#L211-L212

But in the next two commands, is where we should have an updated custom unit.
Screen Shot 2022-10-20 at 11 09 36 AM

It seems like because it's slow to merge a collection METHOD_MERGE_COLLECTION like we do in ReconnectApp, it might be happening after the updates and merges.

@thienlnam thienlnam added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Oct 20, 2022
@thienlnam
Copy link
Contributor

thienlnam commented Oct 20, 2022

Out of the staging changes that could have impacted this, we might have uncovered some bugs due to the new onyx changes #11945 due to merge being faster, otherwise nothing else is sticking out.

I was able to get this to work by removing this line Expensify/Web-Expensify@fc8ad87/lib/AppInit.php#L211-L212 so that the customRate is never set to null, but it's still flipping between values (Fetches the old value first, but then shows the new value after)

I'm going to remove the deploy blocker since we this will have to update this with seperate changes, and the user can just see the updated rate by opening the screen again.

To sum up the issue, currently, the ReconnectApp API call when you come back online is merging null into CustomUnit here. This leads to the custom unit showing as 0 because METHOD_MERGE_COLLECTION takes longer than merge / set so our UpdateWorkspaceCustomUnitRate API pusher changes happen first, and then get overwritten with null. If you remove the customUnit: null we still have another issue where the new optimistic value in onyx is being overwritten somewhere in one of the Get / Reconnect api calls before UpdateWorkspaceCustomUnitRate API call is complete which leads to briefly seeing the old value, and then the new value once you come back online.

cc @amyevans / @jasperhuangg / @yuwenmemon
since you all worked on this code, do any of you have some additional context to this / does my investigation make sense?

@amyevans
Copy link
Contributor

amyevans commented Oct 20, 2022

I think your investigation makes sense @thienlnam!

  • Removing customUnit: null should clear up the overwriting with null problem
  • The other issue you describe

the new optimistic value in onyx is being overwritten somewhere in one of the Get / Reconnect api calls before UpdateWorkspaceCustomUnitRate API call is complete

is documented in https://github.com/Expensify/Expensify/issues/229696 so I think we should prioritize finding a fix for that, though it sounds like it's a tricky problem

Curious if others agree though too!

@thienlnam
Copy link
Contributor

I created https://github.com/Expensify/Web-Expensify/pull/35251 to fix the customUnit: null issue, and it sounds like the flickering issue is addressed in https://github.com/Expensify/Expensify/issues/229696

@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2022

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

@amyevans amyevans added the Reviewing Has a PR in review label Oct 25, 2022
@melvin-bot melvin-bot bot removed the Overdue label Oct 25, 2022
@thienlnam
Copy link
Contributor

customUnit: null PR has been merged, closing this one out and https://github.com/Expensify/Expensify/issues/229696 will take care of the rest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

5 participants