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.12k stars 2.61k forks source link

Track tax - Track tax switch does not appear grayed out when enabled/disabled offline #43318

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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.4.80-14 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Expected Result:

Track tax switch will appear grayed out when enabled/disabled offline

Actual Result:

Track tax switch does not appear grayed out when enabled/disabled offline

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/a29950f8-f741-4b95-99e4-6915360a2d2b

View all open jobs on GitHub

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @srikarparsi (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: 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.
lanitochka17 commented 1 month ago

@srikarparsi FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 1 month ago

We think that this bug might be related to #vip-vsp

srikarparsi commented 1 month ago

I believe this is related to this feature

srikarparsi commented 1 month ago

@nkdengineer @eVoloshchak are you available to take a look at this?

luacmartins commented 1 month ago

I think we can demote this to NAB and let the contributors work on a fix.

b4s36t4 commented 1 month ago

Proposal

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

Track tax - Track tax switch does not appear grayed out when enabled/disabled offline

What is the root cause of that problem?

We're not using any styles for the offline state in the Track Tax setting page like we do at the same places.

https://github.com/Expensify/App/blob/a5ad54af17eaac1b575458aa7cc283159d5f6fb9/src/pages/workspace/workflows/ToggleSettingsOptionRow.tsx#L85-L91

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

We just need to provide the offline state of the OfflineFeedback component here https://github.com/Expensify/App/blob/d22eacf4ec925a79337ab469c41ac5db638ed5f8/src/pages/workspace/distanceRates/PolicyDistanceRatesSettingsPage.tsx#L125 in this line

<OfflineWithFeedback errorRowStyles={styles.mh5} pendingAction={isPendingAction}>

We track isPendingAction with useMemo like as follows

  const isPendingAction = useMemo(() => {
        if(!policy){
            return null
        }
        return policy.pendingFields?.["customUnits"]
    },[policy])

Which tracks the pendingFeilds state from the onyx which updates according to the Offline UX.

What alternative solutions did you explore? (Optional)

NA

Demo

https://github.com/Expensify/App/assets/59088937/4542cc63-cda4-4ac9-844a-de8535a4314e

dominictb commented 3 weeks ago

Proposal

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

Track tax switch does not appear grayed out when enabled/disabled offline

What is the root cause of that problem?

We don't pass pendingAction props to OfflineWithFeedback component here

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

I think we should update the correct pendingFields for track tax here. It should be taxEnabled instead of customUnits because customUnits has many fields to update and we cannot know what exact field is updated

And then pass pendingAction prop as policy.pendingFields.taxEnabled here

Additional, we also should generic error for EnableDistanceRequestTax API in failure data and pass this error as errors prop into OfflineWithFeedback here to show the error and create a function to clear this error.

What alternative solutions did you explore? (Optional)

NA

srikarparsi commented 3 weeks ago

Let's wait on @nkdengineer @eVoloshchak since they were the PR authors.

nkdengineer commented 3 weeks ago

I'll raise PR soon

nkdengineer commented 3 weeks ago

@eVoloshchak please check this PR

Pujan92 commented 3 weeks ago

@srikarparsi I got assigned to the PR, Can you plz request review from @eVoloshchak and remove me. Or let me know if I need to review that.

srikarparsi commented 3 weeks ago

Yes, thank you for letting me know

melvin-bot[bot] commented 4 days ago

This issue has not been updated in over 15 days. @srikarparsi 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!