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

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

Closed
6 tasks done
lanitochka17 opened this issue Jun 7, 2024 · 18 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 7, 2024

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.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:

  • Distance rates and Taxes are enabled
  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Distance rates
  3. Go offline
  4. Click Settings
  5. Enable or disable Track tax switch

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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6505501_1717788210052.bandicam_2024-06-08_03-21-06-590.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jun 7, 2024
Copy link

melvin-bot bot commented Jun 7, 2024

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

Copy link
Contributor

github-actions bot commented Jun 7, 2024

👋 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
Copy link
Author

@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
Copy link
Author

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

@srikarparsi
Copy link
Contributor

I believe this is related to this feature

@srikarparsi
Copy link
Contributor

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

@luacmartins
Copy link
Contributor

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

@luacmartins luacmartins added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API Hourly KSv2 labels Jun 7, 2024
@b4s36t4
Copy link
Contributor

b4s36t4 commented Jun 7, 2024

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.

<OfflineWithFeedback
pendingAction={pendingAction}
errors={errors}
errorRowStyles={[styles.mt2]}
style={[wrapperStyle]}
onClose={onCloseError}
>

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

<OfflineWithFeedback errorRowStyles={styles.mh5}>
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

Kapture.2024-06-08.at.02.27.44.mp4

@dominictb
Copy link
Contributor

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

@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2024
@srikarparsi
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 10, 2024
@nkdengineer
Copy link
Contributor

I'll raise PR soon

@nkdengineer
Copy link
Contributor

@eVoloshchak please check this PR

@Pujan92
Copy link
Contributor

Pujan92 commented Jun 10, 2024

@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
Copy link
Contributor

Yes, thank you for letting me know

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

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!

Copy link

melvin-bot bot commented Aug 14, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@srikarparsi srikarparsi added the Bug Something is broken. Auto assigns a BugZero manager. label Sep 10, 2024
Copy link

melvin-bot bot commented Sep 10, 2024

Triggered auto assignment to @sonialiap (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 melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Sep 10, 2024
@srikarparsi
Copy link
Contributor

Thought we might need to still pay for this issue but it looks like this was a regression caused by this PR. Since the authors are the same for the original PR and this fix, I don't think there's anything left to do for this issue so closing out. But please let me know if I need to re-open @nkdengineer @eVoloshchak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants