Add notice and notification in Advanced Fraud Settings page #8762
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +149 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
…ub.com/Automattic/woocommerce-payments into fix/6497-advanced-fraud-settings-empty
daquinons
changed the title
There was a problem hiding this comment.
I see 3 paths in this flow:
- The user is currently on basic and saves an empty advanced setting.
- In this case, couldn't we return early to avoid the "Settings saved" toast?
- Since there are no changes to save, I think it would be safe to just skip the save and present just the
At least one risk filter needs to be enabled for advanced protection.message.
- The user is currently on basic and saves a filled advanced setting.
- The
Current protection level is set to "advanced".makes sense.
- The
- The user is currently on advanced setting and saves an empty advanced setting (resets the form)
- The user will be notified by an
At least one risk filter needs to be enabled for advanced protection.message, but I think a better message could beProtection level set to "Basic" due to empty advanced configuration.in this case.
- The user will be notified by an
What do you think?
|
Thanks for the suggestions @eduardoumpierre !
At the beginning I assumed that we were sharing the context with the main Settings screen so that's why it had to be saved even entering on the Advanced risk screen. However, I just tested it and we are not sharing it so exiting early makes sense here :) There is a problem though, exiting early without saving the settings will break it in the example I give in the description: a user with advanced settings enabled and one rule enable, if it disables the rule, I wouldn't be able to move it down to "Basic" level. I've added the save only inside that check (
👍
I've changed to |
eduardoumpierre
approved these changes
May 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6497
Changes proposed in this Pull Request
Advanced fraud protection should not be enabled if no rules are enabled. Right now, because there is no check when that happens, we save it gracefully but then it's reverted back.
This PR adds a new notice at the top of the page to indicate that at least one rule needs to be enabled, and dispatches an error notice when that's not the case and the user saves its settings.
In order to avoid weird states (such as the user enables Advanced with one rule set, saves it, but then goes back to it and disables the rule set), the level is stored as "basic" when clicking on save without any rule enabled (if the current level is not already "basic").
Testing instructions
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge