Fix/setup wizard enhancements #3454 #2467
Conversation
…454-' into fix/Setup-wizard-enhancements-#3454-
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
includes/Vendor/SetupWizard.php (1)
80-86: Sanitize the 'step' Parameter Before ComparisonConsider sanitizing the
$_GET['step']parameter usingsanitize_key()before comparing it to'payment'to ensure consistent data handling and enhance security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
includes/Admin/SetupWizard.php(7 hunks)includes/Admin/SetupWizardNoWC.php(1 hunks)includes/Vendor/SetupWizard.php(6 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Vendor/SetupWizard.php
[failure] 509-509:
Mixing different binary boolean operators within an expression without using parentheses to clarify precedence is not allowed.
🔇 Additional comments (1)
includes/Admin/SetupWizardNoWC.php (1)
76-82: Maintain Consistent Variable Usage
The update from $this->step to $this->current_step ensures consistency across the setup wizard classes. The changes correctly reference the current step when checking for a view and rendering it.
| /** @var string Currenct Step */ | ||
| protected $step = ''; | ||
| /** @var string Current Step */ | ||
| protected string $current_step = ''; |
There was a problem hiding this comment.
Confirm PHP Version Compatibility for Typed Properties
The use of typed properties (protected string $current_step) requires PHP 7.4 or higher. Please ensure that the project's minimum PHP version is set to 7.4 or higher to maintain compatibility with this feature.
includes/Vendor/SetupWizard.php
Outdated
| } | ||
| else { | ||
| if ( ( isset( $states[ $dokan_settings['address']['country'] ] ) && count( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) || ( ! isset( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) ) ) ) { | ||
| } elseif ( ( isset( $states[ $dokan_settings['address']['country'] ] ) && count( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) || ( ! isset( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) ) ) ) { |
There was a problem hiding this comment.
Clarify Boolean Expression with Parentheses
The condition on line 509 mixes && and || operators without explicit parentheses, which can cause confusion due to operator precedence rules. Adding parentheses will clarify the logic and ensure the condition evaluates as intended.
Apply this diff to fix the issue:
- } elseif ( ( isset( $states[ $dokan_settings['address']['country'] ] ) && count( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) || ( ! isset( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) ) ) ) {
+ } elseif (
+ (
+ isset( $states[ $dokan_settings['address']['country'] ] )
+ && count( $states[ $dokan_settings['address']['country'] ] )
+ && empty( $dokan_settings['address']['state'] )
+ )
+ || (
+ ! isset( $states[ $dokan_settings['address']['country'] ] )
+ && empty( $dokan_settings['address']['state'] )
+ )
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } elseif ( ( isset( $states[ $dokan_settings['address']['country'] ] ) && count( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) || ( ! isset( $states[ $dokan_settings['address']['country'] ] ) && empty( $dokan_settings['address']['state'] ) ) ) ) { | |
| } elseif ( | |
| ( | |
| isset( $states[ $dokan_settings['address']['country'] ] ) | |
| && count( $states[ $dokan_settings['address']['country'] ] ) | |
| && empty( $dokan_settings['address']['state'] ) | |
| ) | |
| || ( | |
| ! isset( $states[ $dokan_settings['address']['country'] ] ) | |
| && empty( $dokan_settings['address']['state'] ) | |
| ) | |
| ) { |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 509-509:
Mixing different binary boolean operators within an expression without using parentheses to clarify precedence is not allowed.
| public function get_next_step_link(): string { | ||
| $keys = array_keys( $this->steps ); | ||
| $step = array_search( $this->current_step, $keys, true ); | ||
| ++$step; | ||
|
|
||
| // If next step is payment but there are no active methods, skip to the following step | ||
| if ( 'payment' === $keys[ $step ] && empty( dokan_withdraw_get_active_methods() ) ) { | ||
| ++$step; | ||
| } | ||
| $next_step = $keys[ $step ] ?? ''; | ||
| return add_query_arg( | ||
| [ | ||
| 'step' => apply_filters( 'dokan_seller_wizard_next_step', $next_step, $this->current_step, $this->steps ), | ||
| '_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ), | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Prevent Potential Undefined Index in Step Navigation
In the get_next_step_link() method, incrementing $step without bounds checking may lead to an undefined index if $step exceeds the array length of $keys. Please ensure that $step remains within the valid range to prevent errors.
Apply this diff to fix the issue:
$keys = array_keys( $this->steps );
$step = array_search( $this->current_step, $keys, true );
++$step;
+ if ( ! isset( $keys[ $step ] ) ) {
+ $next_step = '';
+ } else {
+ // If next step is payment but there are no active methods, skip to the following step
+ if ( 'payment' === $keys[ $step ] && empty( dokan_withdraw_get_active_methods() ) ) {
+ ++$step;
+ }
+ $next_step = $keys[ $step ] ?? '';
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function get_next_step_link(): string { | |
| $keys = array_keys( $this->steps ); | |
| $step = array_search( $this->current_step, $keys, true ); | |
| ++$step; | |
| // If next step is payment but there are no active methods, skip to the following step | |
| if ( 'payment' === $keys[ $step ] && empty( dokan_withdraw_get_active_methods() ) ) { | |
| ++$step; | |
| } | |
| $next_step = $keys[ $step ] ?? ''; | |
| return add_query_arg( | |
| [ | |
| 'step' => apply_filters( 'dokan_seller_wizard_next_step', $next_step, $this->current_step, $this->steps ), | |
| '_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ), | |
| ] | |
| ); | |
| } | |
| public function get_next_step_link(): string { | |
| $keys = array_keys( $this->steps ); | |
| $step = array_search( $this->current_step, $keys, true ); | |
| ++$step; | |
| if ( ! isset( $keys[ $step ] ) ) { | |
| $next_step = ''; | |
| } else { | |
| // If next step is payment but there are no active methods, skip to the following step | |
| if ( 'payment' === $keys[ $step ] && empty( dokan_withdraw_get_active_methods() ) ) { | |
| ++$step; | |
| } | |
| $next_step = $keys[ $step ] ?? ''; | |
| } | |
| return add_query_arg( | |
| [ | |
| 'step' => apply_filters( 'dokan_seller_wizard_next_step', $next_step, $this->current_step, $this->steps ), | |
| '_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ), | |
| ] | |
| ); | |
| } |
| @@ -12,7 +12,7 @@ | |||
| class SetupWizard { | |||
|
|
|||
| /** @var string Currenct Step */ | |||
| protected $step = ''; | |||
| protected string $current_step = ''; | |||
There was a problem hiding this comment.
Confirm PHP Version Compatibility for Typed Properties
The use of typed properties (protected string $current_step) requires PHP 7.4 or higher. Please ensure that the project's minimum PHP version is set to 7.4 or higher to maintain compatibility.
| @@ -292,7 +292,7 @@ | |||
|
|
|||
| return add_query_arg( | |||
| [ | |||
| 'step' => $keys[ array_search( $this->step, array_keys( $this->steps ), true ) + 1 ], | |||
| 'step' => $keys[ array_search( $this->current_step, array_keys( $this->steps ), true ) + 1 ], | |||
There was a problem hiding this comment.
Ensure Array Index Exists When Navigating Steps
In the get_next_step_link() method, accessing $keys[ array_search( $this->current_step, array_keys( $this->steps ), true ) + 1 ] may result in an undefined index if the current step is the last in the array. Please add a check to confirm that the next index exists before accessing it.
Apply this diff to fix the issue:
$keys = array_keys( $this->steps );
$current_index = array_search( $this->current_step, $keys, true );
+ if ( false !== $current_index && isset( $keys[ $current_index + 1 ] ) ) {
$next_step = $keys[ $current_index + 1 ];
+ } else {
+ $next_step = '';
+ }
return add_query_arg(
[
'step' => $next_step,
'_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ),
]
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
includes/Vendor/SetupWizard.php (4)
Line range hint
496-521: Simplify state validation logicThe state validation logic can be simplified for better readability and maintainability.
Consider this refactoring:
- $country = $dokan_settings['address']['country'] ?? ''; - $state = $dokan_settings['address']['state'] ?? ''; - $country_has_states = isset( $states[ $country ] ) && count( $states[ $country ] ) > 0; - $state_is_empty = empty( $state ); - } elseif ( ( $country_has_states && $state_is_empty ) || ( ! $country_has_states && $state_is_empty ) ) { + } elseif ( empty( $dokan_settings['address']['state'] ) ) { $is_valid_form = false; $_POST['error_address[state]'] = 'error'; }The simplified version achieves the same validation while being more concise.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 521-521:
Functions must not contain multiple empty lines in a row; found 2 empty lines
521-521: Fix code style: Remove extra empty lineThere are multiple empty lines that should be reduced to a single empty line.
- - +🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 521-521:
Functions must not contain multiple empty lines in a row; found 2 empty lines
602-606: Improve code formatting for array_filter callbackThe indentation in the array_filter callback uses tabs instead of spaces.
$user_bank_data = array_filter( $dokan_settings['payment']['bank'], function ( $item ) { - return ! empty( $item ); - } + return ! empty( $item ); + } );
80-85: Consider moving payment step check to a separate methodThe payment step validation logic is duplicated between the setup_wizard() method and get_next_step_link(). Consider extracting this into a separate method.
+ /** + * Checks if payment step should be skipped + * + * @return bool True if payment step should be skipped + */ + protected function should_skip_payment_step(): bool { + return empty( dokan_withdraw_get_active_methods() ); + } + public function setup_wizard() { // ... - if ( isset( $_GET['step'] ) && 'payment' === $_GET['step'] ) { - $active_methods = dokan_withdraw_get_active_methods(); - if ( empty( $active_methods ) ) { + if ( isset( $_GET['step'] ) && 'payment' === $_GET['step'] && $this->should_skip_payment_step() ) { wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); exit; - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Vendor/SetupWizard.php(17 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Vendor/SetupWizard.php
[failure] 521-521:
Functions must not contain multiple empty lines in a row; found 2 empty lines
🔇 Additional comments (2)
includes/Vendor/SetupWizard.php (2)
12-13: LGTM: Property type declaration enhances code safety
The addition of the type declaration string for $current_step improves type safety and code clarity.
666-743: Well-structured implementation of wizard navigation
The new methods get_next_step_link() and set_steps() provide a robust implementation for wizard navigation:
- Proper handling of payment step skipping when no withdrawal methods are active
- Clean separation of step configuration
- Good use of filters for extensibility
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor