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

[$250] Workspace - Toggle btn changed to green but the dot oriented incorrectly in WS settings #47637

Closed lanitochka17 closed 4 weeks ago

lanitochka17 commented 2 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.21-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4864753&group_by=cases:section_id&group_id=316342&group_order=asc Email or phone of affected tester (no customers): applausetester+omco4@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Precondition: create Control workspace in OD

  1. Sign in with the same account in ND
  2. Navigate to Settings > Control Workspace > More features
  3. Enable "Report Fields" option
  4. Navigate to Report Fields settings page
  5. Add field with any name and Text type
  6. Set any Initial value for the text field and tap on "Save" button
  7. Navigate to "More Features" page
  8. Disable "Report Fields" toggle button
  9. In OD update with any new text the initial value for created Text field
  10. Observe the "Report Fields" toggle button status

Expected Result:

"Report Fields" button should be be displayed green color and toggle on state should be enabled

Actual Result:

"Report Fields" buttons changed color to green, but toggle button state remains disabled (the the dot inside the button is oriented incorrectly)

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/91514fd0-a4a9-4768-a15c-18a04e11ad84

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014197c427158deea5
  • Upwork Job ID: 1825967658437935245
  • Last Price Increase: 2024-09-03
  • Automatic offers:
    • alitoshmatov | Reviewer | 103803779
    • FitseTLT | Contributor | 103803780
Issue OwnerCurrent Issue Owner: @alitoshmatov
melvin-bot[bot] commented 2 months ago

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

lanitochka17 commented 2 months ago

We think that this bug might be related to #wave-control

lanitochka17 commented 2 months ago

@puneetlath 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

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~014197c427158deea5

melvin-bot[bot] commented 2 months ago

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

FitseTLT commented 2 months ago

Proposal

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

Tags settings –Tags not appears in the LHP of the workspace settings if added in OldDot and Warning: Maximum update depth exceeded loop bug occurs

What is the root cause of that problem?

The root cause is when the report field enabling comes from OD enabledItem is set to the reportField item so highlighted will be true for the reportField item https://github.com/Expensify/App/blob/d4d5a2586910ff46147219ee3e98bb3e936f8037/src/pages/workspace/WorkspaceInitialPage.tsx#L433-L434 But the problem is shouldHighlight will be fixed to be true in useAnimatedHighlightStyle which will set startHighlight https://github.com/Expensify/App/blob/d4d5a2586910ff46147219ee3e98bb3e936f8037/src/hooks/useAnimatedHighlightStyle/index.ts#L68-L69 and that will in turn run this effect https://github.com/Expensify/App/blob/d4d5a2586910ff46147219ee3e98bb3e936f8037/src/hooks/useAnimatedHighlightStyle/index.ts#L75-L76 which will set it to false so we will get into a loop and Warning: Maximum update depth exceeded occurs.

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

The starting of the highlight should only be triggered on shouldHighlight change not be triggered continously if it is true which will cause a loop as in this current issue. So we need to change https://github.com/Expensify/App/blob/d4d5a2586910ff46147219ee3e98bb3e936f8037/src/hooks/useAnimatedHighlightStyle/index.ts#L69-L70

    }, [shouldHighlight]);

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 months ago

@puneetlath, @alitoshmatov Huh... This is 4 days overdue. Who can take care of this?

alitoshmatov commented 2 months ago

@FitseTLT Thank you for your proposal. You solution is fixing Maximum update depth exceeded error but toggle is not getting enabled.

There is just a lot of issue going on here. For example when there is a change in old dot, Report fields toggle is getting enabled but it is not getting added to workspace menu. Moreover the same happening after some changes in old dot, the Report fields is not appearing in the menu after enabling it using toggle

FitseTLT commented 2 months ago

@FitseTLT Thank you for your proposal. You solution is fixing Maximum update depth exceeded error but toggle is not getting enabled.

There is just a lot of issue going on here. For example when there is a change in old dot, Report fields toggle is getting enabled but it is not getting added to workspace menu. Moreover the same happening after some changes in old dot, the Report fields is not appearing in the menu after enabling it using toggle

I don't understand @alitoshmatov Everything is working fine for me:

https://github.com/user-attachments/assets/a35a96cd-65c6-4e62-ba3a-2fe6e24c7b19

melvin-bot[bot] commented 2 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] commented 2 months ago

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

puneetlath commented 2 months ago

@alitoshmatov any thoughts?

melvin-bot[bot] commented 2 months ago

@puneetlath @alitoshmatov this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 2 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

alitoshmatov commented 2 months ago

Sorry for late response, @FitseTLT I remember having some issues back then but now everything is working correctly after applying your solution maybe messed up something last time.

alitoshmatov commented 2 months ago

We can go with @FitseTLT 's proposal

C+ reviewed πŸŽ€ πŸ‘€ πŸŽ€

melvin-bot[bot] commented 2 months ago

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

πŸ“£ @alitoshmatov πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 months ago

πŸ“£ @FitseTLT πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 month ago

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

FitseTLT commented 1 month ago

@puneetlath payment overdue here

alitoshmatov commented 1 month ago

This issue eroded to monthly issue. The pr went into production on 20th of september.

puneetlath commented 4 weeks ago

All paid. Sorry for the delay.