Skip to content

Fix "Save changes?" modal saves the options after selecting the 'Discard' option #36160

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.

Sign up for GitHub

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

Jump to bottom
Merged
chihsuan merged 4 commits into from
Dec 28, 2022
Merged

Fix "Save changes?" modal saves the options after selecting the 'Discard' option #36160

chihsuan merged 4 commits into from
Dec 28, 2022

Conversation

@chihsuan
Copy link
Member

@chihsuan chihsuan commented Dec 23, 2022 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

Closes #35934

"Save changes?" modal saves the options after selecting the 'Discard' option because we mutate the value of the profiler items prop.

Changes in this PR:

  • Fixes the bug
  • Adds e2e tests.
  • Tweaks the save changes modal UI

Before:

Screen Shot 2022-12-23 at 18 00 57

After:

Screen Shot 2022-12-23 at 17 59 49

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Start with OBW.
  2. Go to Industry Page.
  3. Select some options
  4. Click on the next page for 'Product Types'
  5. When prompted by 'Save changes?' modal, click 'Discard'
  6. Navigate back to Industry page observe the selected options are NOT saved.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@chihsuan chihsuan self-assigned this Dec 23, 2022
@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Dec 23, 2022
@chihsuan chihsuan marked this pull request as ready for review December 23, 2022 10:01
@chihsuan chihsuan requested review from a team, ilyasfoo and moon0326 December 23, 2022 10:02
@github-actions
Copy link
Contributor

github-actions bot commented Dec 23, 2022 •
edited
Loading

Test Results Summary

Commit SHA: 6457489

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 51s
E2E Tests189006019513m 35s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@chihsuan chihsuan force-pushed the fix/obw-save-changes branch from fb3c3fc to 6457489 Compare December 23, 2022 11:23
ilyasfoo
ilyasfoo approved these changes Dec 27, 2022
View reviewed changes
Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and adding e2e tests, @chihsuan! Excellent work, I have a question to pick your brain but LGTM! 🚢

plugins/woocommerce-admin/client/profile-wizard/steps/industry.js
let selected = profileItems.industry || [];

let selected = Array.isArray( profileItems.industry )
? [ ...profileItems.industry ]
Copy link
Contributor

@ilyasfoo ilyasfoo Dec 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, why do we need to clone the value here? Is there an issue to reuse the same object reference? Or are there cases where it's not truly an array?

Copy link
Member Author

@chihsuan chihsuan Dec 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's the same object reference issue.

Previously, we used the same object in the local state; when we update the local "selected" state, it persists the changes in the global profileItems. So when a user discards the changes and navigates back from a different screen, the user would still see the "changed" view.

Copy link
Contributor

@ilyasfoo ilyasfoo Dec 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant, thanks!

rjchow
rjchow approved these changes Dec 28, 2022
View reviewed changes
Copy link
Contributor

@rjchow rjchow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests! 😄

@chihsuan chihsuan merged commit 8c3f06c into trunk Dec 28, 2022
@chihsuan chihsuan deleted the fix/obw-save-changes branch December 28, 2022 08:23
@github-actions github-actions bot added this to the 7.4.0 milestone Dec 28, 2022
joelclimbsthings pushed a commit that referenced this pull request Jan 6, 2023
@chihsuan @joelclimbsthings
Fix "Save changes?" modal saves the options after selecting the 'Disc…
c212af7 
…ard' option (#36160)

* Tweak save change modal padding

* Fix obw save change and add e2e tests

* Add changelog

* Fix product_types step
@github-actions github-actions bot mentioned this pull request Feb 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moon0326 moon0326 Awaiting requested review from moon0326

2 more reviewers

@ilyasfoo ilyasfoo ilyasfoo approved these changes

@rjchow rjchow rjchow approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chihsuan chihsuan

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.4.0

Development

Successfully merging this pull request may close these issues.

[GlobalStep] Discard button of "Save changes?" modal saves the options from "Industry" and "Product Types" steps for OBW flow.

4 participants

@chihsuan @ilyasfoo @rjchow