[Skip] Payment from Setup wizard when no payment method is active #2434
Conversation
osmansufy
added
the
Needs: Dev Review
osmansufy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
includes/Vendor/SetupWizard.php (1)
698-737: Set the initial step correctly when initializing stepsIn the
set_steps()method, after setting$this->steps, the initial$this->stepis set usingcurrent( array_keys( $this->steps ) ). However, if$this->stepis already set elsewhere (e.g., from sanitized user input), this might overwrite it unexpectedly.Consider checking if
$this->stepis already set before overwriting it:$this->steps = apply_filters( 'dokan_seller_wizard_steps', $steps ); -$this->step = current( array_keys( $this->steps ) ); +if ( empty( $this->step ) ) { + $this->step = current( array_keys( $this->steps ) ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Vendor/SetupWizard.php(4 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.
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.
🛠️ Refactor suggestion
Clarify operator precedence in conditional statement
The condition in line 509 mixes && and || operators without explicit parentheses, which can lead to confusion about the intended logic and potential bugs due to operator precedence rules.
Apply this diff to add parentheses and improve readability:
} 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'] ) ) )
+ (
+ ( 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'] ) )
+ )
) {Consider refactoring the condition for better clarity:
$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 );
if (
(
$country_has_states
&& $state_is_empty
) || (
! $country_has_states
&& $state_is_empty
)
) {
// Handle the error condition
}🧰 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.
includes/Vendor/SetupWizard.php
Outdated
| $keys = array_keys( $this->steps ); | ||
| $step = array_search( $this->step, $keys, true ); | ||
| $next_step = $keys[ $step + 1 ]; | ||
|
|
||
| // If next step is payment but there are no active methods, skip to the following step | ||
| if ( 'payment' === $next_step ) { | ||
| $active_methods = dokan_withdraw_get_active_methods(); | ||
| if ( empty( $active_methods ) ) { | ||
| $next_step = $keys[ $step + 2 ]; | ||
| } | ||
| } | ||
|
|
||
| return add_query_arg( | ||
| [ | ||
| 'step' => $next_step, | ||
| '_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ), | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure array indices are valid to prevent 'undefined offset' errors
In the get_next_step_link() method, accessing $keys[ $step + 1 ] and $keys[ $step + 2 ] without checking if the indices exist can lead to 'undefined offset' notices if the current step is at the end of the array.
Apply this diff to validate array indices before accessing them:
$keys = array_keys( $this->steps );
$step = array_search( $this->step, $keys, true );
-$next_step = $keys[ $step + 1 ];
+$next_step = isset( $keys[ $step + 1 ] ) ? $keys[ $step + 1 ] : '';
// If next step is payment but there are no active methods, skip to the following step
if ( 'payment' === $next_step ) {
$active_methods = dokan_withdraw_get_active_methods();
if ( empty( $active_methods ) ) {
- $next_step = $keys[ $step + 2 ];
+ $next_step = isset( $keys[ $step + 2 ] ) ? $keys[ $step + 2 ] : '';
}
}
+if ( empty( $next_step ) ) {
+ // No next step available, handle accordingly
+ return '';
+}
return add_query_arg(
[
'step' => $next_step,
'_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ),
]
);This ensures that you do not attempt to access an array index that does not exist, preventing potential errors.
📝 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.
| $keys = array_keys( $this->steps ); | |
| $step = array_search( $this->step, $keys, true ); | |
| $next_step = $keys[ $step + 1 ]; | |
| // If next step is payment but there are no active methods, skip to the following step | |
| if ( 'payment' === $next_step ) { | |
| $active_methods = dokan_withdraw_get_active_methods(); | |
| if ( empty( $active_methods ) ) { | |
| $next_step = $keys[ $step + 2 ]; | |
| } | |
| } | |
| return add_query_arg( | |
| [ | |
| 'step' => $next_step, | |
| '_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ), | |
| ] | |
| ); | |
| } | |
| $keys = array_keys( $this->steps ); | |
| $step = array_search( $this->step, $keys, true ); | |
| $next_step = isset( $keys[ $step + 1 ] ) ? $keys[ $step + 1 ] : ''; | |
| // If next step is payment but there are no active methods, skip to the following step | |
| if ( 'payment' === $next_step ) { | |
| $active_methods = dokan_withdraw_get_active_methods(); | |
| if ( empty( $active_methods ) ) { | |
| $next_step = isset( $keys[ $step + 2 ] ) ? $keys[ $step + 2 ] : ''; | |
| } | |
| } | |
| if ( empty( $next_step ) ) { | |
| // No next step available, handle accordingly | |
| return ''; | |
| } | |
| return add_query_arg( | |
| [ | |
| 'step' => $next_step, | |
| '_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ), | |
| ] | |
| ); | |
| } |
includes/Vendor/SetupWizard.php
Outdated
| public function get_next_step_link(): string { | ||
| $keys = array_keys( $this->steps ); | ||
| $step = array_search( $this->step, $keys, true ); | ||
| $next_step = $keys[ $step + 1 ]; |
There was a problem hiding this comment.
- Just increase the next step index
$step++here and inside theifcondition. - Assign
$next_stepafter if condition.
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
includes/Admin/SetupWizard.php (2)
295-298: Consider improving step navigation readability.While the current implementation is functionally correct, consider extracting the step navigation logic into dedicated methods for better readability and maintainability.
+ private function get_step_index($step) { + return array_search($step, array_keys($this->steps), true); + } + + private function is_step_complete($step) { + return $this->get_step_index($this->current_step) > $this->get_step_index($step); + } + public function get_next_step_link() { $keys = array_keys( $this->steps ); return add_query_arg( [ - 'step' => $keys[ array_search( $this->current_step, array_keys( $this->steps ), true ) + 1 ], + 'step' => $keys[ $this->get_step_index($this->current_step) + 1 ], '_admin_sw_nonce' => wp_create_nonce( 'dokan_admin_setup_wizard_nonce' ), ] ); }Also applies to: 350-352
Line range hint
584-587: Improve withdraw limit validation.The current validation has potential issues:
- No explicit type casting before numeric check
- Negative values could be saved despite the >= 0 check due to type coercion
Apply this fix:
- if ( ! empty( $_POST['withdraw_limit'] ) ) { - $input_limit = sanitize_text_field( wp_unslash( $_POST['withdraw_limit'] ) ); - $options['withdraw_limit'] = is_numeric( $input_limit ) && $input_limit >= 0 ? wc_format_decimal( $input_limit ) : 0; - } else { - $options['withdraw_limit'] = 0; - } + $input_limit = ! empty( $_POST['withdraw_limit'] ) ? sanitize_text_field( wp_unslash( $_POST['withdraw_limit'] ) ) : ''; + $numeric_limit = (float) $input_limit; + $options['withdraw_limit'] = $numeric_limit > 0 ? wc_format_decimal( $numeric_limit ) : 0;
📜 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 (7)
includes/Admin/SetupWizardNoWC.php (1)
76-82: LGTM! Consider documenting the inherited property.
The renaming from $this->step to $this->current_step maintains consistency with changes in other files. However, since $current_step is inherited from the parent class, it would be helpful to document it in this class for better code readability.
Add a class property documentation:
/**
* @inheritdoc
* @var string
*/
protected $current_step;Let's verify the property declaration in the parent class:
includes/Admin/SetupWizard.php (2)
15-15: LGTM! Property type declaration improves code safety.
The explicit string type declaration for $current_step enhances code clarity and type safety.
269-272: LGTM! Secure initialization of setup wizard step.
The code properly handles:
- Secure nonce verification for step parameter
- Proper sanitization of URL parameters
- Safe method invocation for step handlers
Also applies to: 281-282
includes/Vendor/SetupWizard.php (4)
12-13: Property renamed to improve clarity and type safety
Renaming $step to $current_step with an explicit string type improves code readability and ensures type safety.
76-77: Encapsulate step initialization in a dedicated method
Using $this->set_steps() enhances code modularity by separating the step setup logic into its own method.
509-509: Clarify operator precedence in the conditional statement
The condition mixes && and || operators without parentheses, which can cause confusion and potential logic errors.
🧰 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.
695-734: Dynamic step setup enhances flexibility
The set_steps() method dynamically includes the payment step based on active withdrawal methods, improving the setup wizard's flexibility and user experience.
includes/Vendor/SetupWizard.php
Outdated
|
|
||
| // get step from url | ||
| if ( isset( $_GET['_admin_sw_nonce'], $_GET['step'] ) && wp_verify_nonce( sanitize_key( wp_unslash( $_GET['_admin_sw_nonce'] ) ), 'dokan_admin_setup_wizard_nonce' ) ) { | ||
| $this->step = sanitize_key( wp_unslash( $_GET['step'] ) ); | ||
| $this->current_step = sanitize_key( wp_unslash( $_GET['step'] ) ) ?? current( array_keys( $this->steps ) ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure default assignment of $this->current_step
The current assignment might not handle the absence of $_GET['step'] correctly. Use a conditional check to assign a default value when $_GET['step'] is not set.
Apply this diff to set a default step:
- $this->current_step = sanitize_key( wp_unslash( $_GET['step'] ) ) ?? current( array_keys( $this->steps ) );
+ $this->current_step = isset( $_GET['step'] ) ? sanitize_key( wp_unslash( $_GET['step'] ) ) : current( array_keys( $this->steps ) );📝 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.
| $this->current_step = sanitize_key( wp_unslash( $_GET['step'] ) ) ?? current( array_keys( $this->steps ) ); | |
| $this->current_step = isset( $_GET['step'] ) ? sanitize_key( wp_unslash( $_GET['step'] ) ) : current( array_keys( $this->steps ) ); |
includes/Vendor/SetupWizard.php
Outdated
| 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.
Handle potential undefined offsets in $keys array
When accessing $keys[ $step ], ensure the index exists to prevent 'undefined offset' notices if $step exceeds the array bounds.
Apply this diff to add a safety check:
++$step;
- if ( 'payment' === $keys[ $step ] && empty( dokan_withdraw_get_active_methods() ) ) {
+ if ( isset( $keys[ $step ] ) && 'payment' === $keys[ $step ] && empty( dokan_withdraw_get_active_methods() ) ) {
++$step;
}
- $next_step = $keys[ $step ] ?? '';
+ $next_step = isset( $keys[ $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 next step is payment but there are no active methods, skip to the following step | |
| if ( isset( $keys[ $step ] ) && 'payment' === $keys[ $step ] && empty( dokan_withdraw_get_active_methods() ) ) { | |
| ++$step; | |
| } | |
| $next_step = isset( $keys[ $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' ), | |
| ] | |
| ); | |
| } |
includes/Vendor/SetupWizard.php
Outdated
| if ( isset( $_GET['step'] ) && 'payment' === $_GET['step'] ) { | ||
| $active_methods = dokan_withdraw_get_active_methods(); | ||
| if ( empty( $active_methods ) ) { | ||
| wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); | ||
| exit; | ||
| } | ||
| } |
There was a problem hiding this comment.
Use $this->current_step instead of directly accessing $_GET['step']
Accessing $_GET['step'] directly without sanitization can lead to security vulnerabilities. Since $this->current_step is already sanitized, use it to check the current step.
Apply this diff to use the sanitized property:
- if ( isset( $_GET['step'] ) && 'payment' === $_GET['step'] ) {
+ if ( 'payment' === $this->current_step ) {
$active_methods = dokan_withdraw_get_active_methods();
if ( empty( $active_methods ) ) {
wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) );
exit;
}
}📝 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.
| if ( isset( $_GET['step'] ) && 'payment' === $_GET['step'] ) { | |
| $active_methods = dokan_withdraw_get_active_methods(); | |
| if ( empty( $active_methods ) ) { | |
| wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); | |
| exit; | |
| } | |
| } | |
| if ( 'payment' === $this->current_step ) { | |
| $active_methods = dokan_withdraw_get_active_methods(); | |
| if ( empty( $active_methods ) ) { | |
| wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); | |
| exit; | |
| } | |
| } |
osmansufy
added
Needs: Dev Review
includes/Vendor/SetupWizard.php
Outdated
| ++$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() ) ) { |
There was a problem hiding this comment.
Pls handle the undefined index 'payment' === $keys[ $step ] with 'payment' === ($keys[ $step ] ?? '' ).
includes/Vendor/SetupWizard.php
Outdated
| // If payment step is accessed but no active methods exist, redirect to next step | ||
| if ( isset( $_GET['step'] ) && 'payment' === $_GET['step'] ) { | ||
| $active_methods = dokan_withdraw_get_active_methods(); | ||
| if ( empty( $active_methods ) ) { | ||
| wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); | ||
| exit; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are we replicating the logic?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
includes/Vendor/SetupWizard.php (2)
526-530: Fix indentation in payment step check.The indentation is inconsistent and the closing brace is on the same line as the code.
Apply this diff to fix the formatting:
- if ( 'payment' === $this->current_step && empty( $methods ) ) { - wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); - exit; } + if ( 'payment' === $this->current_step && empty( $methods ) ) { + wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); + exit; + }
589-593: Improve array_filter callback readability.The callback function's formatting could be improved for better readability.
Apply this diff:
- $user_bank_data = array_filter( - $dokan_settings['payment']['bank'], function ( $item ) { - return ! empty( $item ); - } - ); + $user_bank_data = array_filter( + $dokan_settings['payment']['bank'], + function ( $item ) { + return ! empty( $item ); + } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Vendor/SetupWizard.php(7 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Vendor/SetupWizard.php
[failure] 500-500:
Mixing different binary boolean operators within an expression without using parentheses to clarify precedence is not allowed.
🔇 Additional comments (5)
includes/Vendor/SetupWizard.php (5)
12-13: LGTM! Property type declaration improves code clarity.
The change from $step to $current_step with an explicit string type declaration enhances code readability and type safety.
76-85: LGTM! Improved initialization flow.
The setup wizard now properly initializes steps before processing the current step, ensuring a more robust initialization sequence.
663-680: LGTM! Well-implemented step navigation logic.
The get_next_step_link method effectively handles:
- Dynamic step navigation
- Payment step skipping when no methods are active
- Proper filtering of the next step
692-731: LGTM! Clean implementation of conditional steps.
The set_steps method:
- Clearly defines the base steps
- Conditionally includes the payment step
- Provides proper filtering of steps
500-502:
Fix operator precedence in conditional statement.
The condition mixes && and || operators without clear precedence, which could lead to unexpected behavior.
Apply this diff to clarify the logic:
- } 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'] )
+ )
+ )
+ ) {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 500-500:
Mixing different binary boolean operators within an expression without using parentheses to clarify precedence is not allowed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
includes/Vendor/SetupWizard.php (4)
76-81: Verify initialization order of steps and current stepThe current step is being accessed before steps are initialized. While it works due to the default empty string initialization, consider moving the
set_steps()call before accessing$this->current_step.- $this->current_step = sanitize_key( wp_unslash( $_GET['step'] ) ) ?? current( array_keys( $this->steps ) ); - - // Setup wizard steps - $this->set_steps(); + // Setup wizard steps + $this->set_steps(); + + $this->current_step = sanitize_key( wp_unslash( $_GET['step'] ) ) ?? current( array_keys( $this->steps ) );
Line range hint
482-504: Improve state validation logic readabilityThe state validation logic has been improved but could be more readable by extracting the validation into a separate method.
+ private function is_state_required( string $country, string $state ): bool { + $country_obj = new WC_Countries(); + $states = $country_obj->states; + $country_has_states = isset( $states[ $country ] ) && count( $states[ $country ] ) > 0; + return ( $country_has_states || ! isset( $states[ $country ] ) ) && empty( $state ); + } public function dokan_setup_store_save() { // ... existing code ... - $country_has_states = isset( $states[ $country ] ) && count( $states[ $country ] ) > 0; - $state_is_empty = empty( $state ); - if ( ( $country_has_states && $state_is_empty ) || ( ! $country_has_states && $state_is_empty ) ) { + if ( $this->is_state_required( $country, $state ) ) { $is_valid_form = false; $_POST['error_address[state]'] = 'error'; }
528-531: Fix indentation in payment step access checkThe indentation of the closing brace is incorrect and should be aligned with the if statement.
if ( 'payment' === $this->current_step && empty( $methods ) ) { wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); exit; - } +}
591-595: Improve array_filter callback readabilityThe array_filter callback could be more readable with consistent indentation.
$user_bank_data = array_filter( $dokan_settings['payment']['bank'], - function ( $item ) { - return ! empty( $item ); - } + function ($item) { + return !empty($item); + } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Vendor/SetupWizard.php(8 hunks)
🔇 Additional comments (3)
includes/Vendor/SetupWizard.php (3)
12-13: LGTM: Property type declaration improves code clarity
The change from $step to $current_step with an explicit string type declaration enhances code readability and type safety.
654-682: LGTM: Well-structured next step link generation
The get_next_step_link() method is well-implemented with:
- Clear type hints
- Proper null coalescing for array access
- Filter hook for extensibility
- Secure nonce generation
684-733: LGTM: Improved steps configuration with conditional payment step
The new set_steps() method:
- Clearly organizes wizard steps
- Conditionally includes payment step based on active methods
- Provides filter hook for extensibility
- Properly initializes current step
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
includes/Vendor/SetupWizard.php (3)
528-531: Fix indentation in the payment step check.The closing brace is incorrectly indented and the conditional block structure is not properly formatted.
Apply this diff to fix the formatting:
if ( 'payment' === $this->current_step && empty( $methods ) ) { - wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); - exit; } + wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); + exit; +}
591-595: Improve array filtering readability.The array filtering logic can be made more readable by using arrow function syntax (PHP 7.4+).
Consider this alternative:
- $user_bank_data = array_filter( - $dokan_settings['payment']['bank'], function ( $item ) { - return ! empty( $item ); - } - ); + $user_bank_data = array_filter( + $dokan_settings['payment']['bank'], + fn($item) => !empty($item) + );
482-485: Improve state validation readability.The state validation logic can be made more clear by using better variable names and consistent spacing.
Consider this refactor:
- $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 ); + $selected_country = $dokan_settings['address']['country'] ?? ''; + $selected_state = $dokan_settings['address']['state'] ?? ''; + $country_has_states = isset($states[$selected_country]) && count($states[$selected_country]) > 0; + $state_is_empty = empty($selected_state);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Vendor/SetupWizard.php(8 hunks)
🔇 Additional comments (3)
includes/Vendor/SetupWizard.php (3)
12-13: LGTM! Property type declaration improves code clarity.
The change from $step to $current_step with an explicit string type declaration enhances code readability and type safety.
654-682: LGTM! Well-implemented step navigation with safety checks.
The get_next_step_link() method:
- Properly handles array bounds using null coalescing
- Includes security nonce
- Allows filter customization
- Correctly skips payment step when no methods are active
684-733: LGTM! Well-structured step configuration.
The set_steps() method:
- Logically organizes wizard steps
- Conditionally includes payment step based on active methods
- Provides filter hook for customization
- Properly initializes current step
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
WalkthroughThe pull request introduces changes primarily focused on renaming the variable Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (75)
⛔ Files not processed due to max files limit (19)
💤 Files with no reviewable changes (75)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
includes/Admin/SetupWizardNoWC.php (1)
76-83: Add error handling for invalid stepsThe current implementation redirects to 'install_woocommerce' step when the current step's view is not found, but this step might not exist in
$this->steps. Consider adding proper error handling.public function setup_wizard_content() { if ( empty( $this->steps[ $this->current_step ]['view'] ) ) { - wp_safe_redirect( esc_url_raw( add_query_arg( 'step', 'install_woocommerce' ) ) ); + $first_step = array_key_first( $this->steps ); + wp_safe_redirect( esc_url_raw( add_query_arg( 'step', $first_step ) ) ); exit; } echo '<div class="wc-setup-content">'; call_user_func( $this->steps[ $this->current_step ]['view'] ); echo '</div>'; }
📜 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(8 hunks)
🔇 Additional comments (5)
includes/Admin/SetupWizardNoWC.php (2)
76-76: LGTM: Variable renaming is consistent
The renaming of $this->step to $this->current_step is consistent with the changes mentioned in the AI summary and maintains code clarity.
Also applies to: 82-82
76-83:
Verify implementation of PR objective
The PR objective is to skip the payment step when no payment methods are active, but this functionality appears to be missing in the current implementation. The setup wizard will still attempt to show the payment step regardless of active payment methods.
Let's verify the payment methods check implementation:
Consider implementing the payment step skip logic in the add_wc_steps_to_wizard method. Here's a suggested approach:
public static function add_wc_steps_to_wizard( $steps ) {
$new_steps = [];
foreach ( $steps as $step => $step_props ) {
$new_steps[ $step ] = $step_props;
if ( 'selling' === $step ) {
+ // Skip payment step if no payment methods are active
+ if ( self::has_active_payment_methods() ) {
$new_steps['payment'] = [
'name' => __( 'Payment', 'dokan-lite' ),
'view' => [ self::class, 'wc_setup_payment' ],
'handler' => [ self::class, 'wc_setup_payment_save' ],
];
+ }
$new_steps['shipping'] = [
'name' => __( 'Shipping', 'dokan-lite' ),
'view' => [ self::class, 'wc_setup_shipping' ],
'handler' => [ self::class, 'wc_setup_shipping_save' ],
];
}
}
return $new_steps;
}
+/**
+ * Check if any payment methods are active
+ *
+ * @since x.x.x
+ *
+ * @return bool
+ */
+protected static function has_active_payment_methods() {
+ if ( ! class_exists( 'WC_Payment_Gateways' ) ) {
+ return false;
+ }
+
+ $payment_gateways = WC()->payment_gateways()->payment_gateways();
+
+ foreach ( $payment_gateways as $gateway ) {
+ if ( 'yes' === $gateway->enabled ) {
+ return true;
+ }
+ }
+
+ return false;
+}includes/Admin/SetupWizard.php (3)
15-15: LGTM! Improved variable naming and type safety.
The change from $step to $current_step with string type hint enhances code clarity and type safety.
331-333: LGTM! Consistent variable usage across step handling.
The variable renaming has been consistently applied across the step handling logic.
Also applies to: 350-352, 366-372
269-272: Verify all variable references are updated consistently.
The variable renaming from step to current_step has been applied, but let's verify no references were missed.
Also applies to: 281-282, 295-295
✅ Verification successful
Let me verify the specific file and context more thoroughly.
No issues found with variable references in SetupWizard.php
The variable renaming from step to current_step has been correctly and consistently implemented in SetupWizard.php. The remaining $step usages found in the file are part of different contexts:
- Loop variable in template rendering:
$step_key => $step - URL parameter name:
stepin query args
All references to the class property correctly use $this->current_step.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old variable name
rg '\$step\b' --type php
Length of output: 2089
Script:
#!/bin/bash
# Search for variable references specifically in SetupWizard.php
rg '\$step\b' includes/Admin/SetupWizard.php -B 2 -A 2
# Also search for current_step to see the full context
rg '\$this->current_step\b' includes/Admin/SetupWizard.php -B 2 -A 2
Length of output: 2468
includes/Admin/SetupWizard.php
Outdated
| if ( empty( $this->steps[ $this->current_step ]['view'] ) ) { | ||
| wp_safe_redirect( esc_url_raw( add_query_arg( 'step', 'introduction' ) ) ); | ||
| exit; | ||
| } | ||
|
|
||
| echo '<div class="wc-setup-content">'; | ||
| call_user_func( $this->steps[ $this->step ]['view'] ); | ||
| call_user_func( $this->steps[ $this->current_step ]['view'] ); |
There was a problem hiding this comment.
Implementation missing for payment step skipping.
The PR objective is to skip the payment step when no payment method is active, but this functionality is not implemented in the setup wizard content handling.
Would you like me to help implement the logic for skipping the payment step when no payment methods are active? This would involve:
- Adding a check for active payment methods
- Modifying the step progression logic to skip the payment step when appropriate
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
includes/Vendor/SetupWizard.php (2)
Line range hint
485-507: Consider extracting state validation logic into a separate methodThe state validation logic is now more robust with null coalescing operators and descriptive variable names. However, it could be further improved by extracting it into a dedicated method.
+ protected function validate_state($country, $state, $states): bool { + $country_has_states = isset($states[$country]) && count($states[$country]) > 0; + $state_is_empty = empty($state); + return !($country_has_states && $state_is_empty) && !(!$country_has_states && $state_is_empty); + } public function dokan_setup_store_save() { // ... $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); - if (($country_has_states && $state_is_empty) || (!$country_has_states && $state_is_empty)) { + if (!$this->validate_state($country, $state, $states)) { $is_valid_form = false; $_POST['error_address[state]'] = 'error'; } // ... }
697-735: LGTM! Well-structured step initialization with extensibilityThe step initialization is well-organized and follows WordPress patterns with proper filter hooks. Consider caching the active methods check to avoid multiple database queries.
Consider adding:
private $active_methods = null; protected function get_active_methods() { if ($this->active_methods === null) { $this->active_methods = dokan_withdraw_get_active_methods(); } return $this->active_methods; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Vendor/SetupWizard.php(9 hunks)
🔇 Additional comments (2)
includes/Vendor/SetupWizard.php (2)
12-13: LGTM! Type declaration improves code safety
The addition of the string type declaration for $current_step enhances type safety and code clarity.
668-685: Verify step navigation for edge cases
The step navigation logic looks good but should be verified for edge cases, particularly when skipping the payment step.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Step navigation logic handles edge cases correctly
The verification shows the step navigation logic is well-implemented with proper safeguards:
- Payment step skipping is handled in two places:
- In
get_next_step_link()when checking next step - In
dokan_setup_payment()with a redirect if payment step is accessed directly
- In
- The
dokan_withdraw_get_active_methods()function is properly used across the codebase - Step navigation is consistently implemented with proper URL sanitization and nonce verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify step navigation logic handles all possible scenarios
# Test: Check if payment step is properly skipped when no payment methods are active
rg -A 5 "dokan_withdraw_get_active_methods"
# Test: Check if step navigation is consistent across the codebase
rg -A 5 "get_next_step_link"
Length of output: 14945
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
includes/Vendor/SetupWizard.php (3)
532-536: Fix indentation in the payment step validation.The code block has incorrect indentation which affects readability.
Apply this diff to fix the indentation:
// If payment step is accessed but no active methods exist, redirect to next step if ( 'payment' === $this->current_step && empty( $methods ) ) { - wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); - exit; + wp_safe_redirect( esc_url_raw( $this->get_next_step_link() ) ); + exit; }
596-600: Improve array filtering readability.The array filtering logic can be made more readable by using proper indentation and alignment.
Apply this diff to improve readability:
$user_bank_data = array_filter( $dokan_settings['payment']['bank'], - function ( $item ) { - return ! empty( $item ); - } + function ($item) { + return !empty($item); + } );
670-687: Consider adding null coalescing for array access safety.The array access in the
get_next_step_linkmethod could be made safer by using null coalescing consistently.Apply this diff to improve array access safety:
public function get_next_step_link(): string { $keys = array_keys( $this->steps ); - $step = array_search( $this->current_step, $keys, true ); + $step = array_search( $this->current_step, $keys, true ) ?: 0; ++$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 ] ?? ''; + $next_step = $keys[ $step ] ?? '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Vendor/SetupWizard.php(9 hunks)
🔇 Additional comments (3)
includes/Vendor/SetupWizard.php (3)
12-13: LGTM! Property type declaration improves code clarity.
The addition of the string type declaration for $current_step enhances type safety and code readability.
699-738: LGTM! Well-structured step management.
The set_steps method is well-organized with:
- Clear step definitions
- Conditional payment step inclusion
- Proper filter hooks for extensibility
740-742: LGTM! Simple and effective first step retrieval.
The get_first_step method is concise and uses the appropriate array function.
osmansufy
added
Needs: Dev Review
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
|
PHP CS & PHP STAN are fixed in this related PR: https://github.com/getdokan/dokan/pull/2414/files @mrabbani bhai |
osmansufy
added
Needs: Dev Review
mrabbani
added
Needs: Testing
osmansufy
force-pushed
the
fix/Setup-wizard-enhancements-#3454-
branch
from
874c57f to
490a380
Compare
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
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
$stepwith$current_stepthroughout the setup wizard classes.Styles