-
Notifications
You must be signed in to change notification settings - Fork 204
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.
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
Add notice for version compatibility. #2450
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@Aunshon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
includes/Admin/Notices/Manager.php (3)
195-199
: Consider extracting version comparison logic.The version comparison logic could be made more maintainable by extracting it into a separate method or using constants for version numbers.
Consider refactoring to:
+ private const MINIMUM_COMPATIBLE_VERSION = '3.14.0'; + + private function is_pro_version_outdated(): bool { + return version_compare( DOKAN_PLUGIN_VERSION, self::MINIMUM_COMPATIBLE_VERSION, '>=' ) && + defined( 'DOKAN_PRO_PLUGIN_VERSION' ) && + version_compare( DOKAN_PRO_PLUGIN_VERSION, self::MINIMUM_COMPATIBLE_VERSION, '<' ); + } + public function show_admin_commission_notice( $notices ) { - if ( - version_compare( DOKAN_PLUGIN_VERSION, '3.14.0', '>=' ) && - defined( 'DOKAN_PRO_PLUGIN_VERSION' ) && - version_compare( DOKAN_PRO_PLUGIN_VERSION, '3.14.0', '<' ) - ) { + if ( $this->is_pro_version_outdated() ) {
205-206
: Add translator comments for better context.The translatable strings would benefit from translator comments explaining the context in which these messages appear.
Add translator comments:
- 'title' => __( 'Upgrade to Dokan Pro Latest Version', 'dokan-lite' ), - 'description' => __( 'Your Dokan Lite version is updated to the latest version but you are still using the Dokan pro old version. Upgrade to Dokan Pro Latest version to enable the new features.', 'dokan-lite' ), + /* translators: Admin notice title shown when Dokan Pro version is outdated */ + 'title' => __( 'Upgrade to Dokan Pro Latest Version', 'dokan-lite' ), + /* translators: Admin notice description explaining the version mismatch between Dokan Lite and Pro */ + 'description' => __( 'Your Dokan Lite version is updated to the latest version but you are still using the Dokan pro old version. Upgrade to Dokan Pro Latest version to enable the new features.', 'dokan-lite' ),
200-208
: Consider adding an action button to the notice.The notice informs users about the version mismatch but doesn't provide a direct way to resolve it. Consider adding an action button linking to the upgrade page or documentation.
Add an action button:
$notices[] = [ 'priority' => 1, 'show_close_button' => false, 'type' => 'alert', 'scope' => 'global', 'title' => __( 'Upgrade to Dokan Pro Latest Version', 'dokan-lite' ), 'description' => __( 'Your Dokan Lite version is updated to the latest version but you are still using the Dokan pro old version. Upgrade to Dokan Pro Latest version to enable the new features.', 'dokan-lite' ), + 'actions' => [ + [ + 'type' => 'primary', + 'text' => __( 'Upgrade Now', 'dokan-lite' ), + 'action' => admin_url( 'update-core.php' ), + ], + ], ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Admin/Notices/Manager.php
(2 hunks)
🔇 Additional comments (1)
includes/Admin/Notices/Manager.php (1)
53-53
: LGTM! Hook registration follows WordPress standards.
The filter registration is consistent with the existing pattern and clearly indicates its purpose.
includes/Admin/Notices/Manager.php
Outdated
@@ -189,4 +190,23 @@ private function dismiss_notice( string $option_name ) { | |||
update_option( $option_name, 'yes' ); | |||
wp_send_json_success(); | |||
} | |||
|
|||
public function show_admin_commission_notice( $notices ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add PHPDoc block for the method.
Following WordPress coding standards and maintaining consistency with other methods in this class, please add a PHPDoc block documenting the method's purpose, parameters, and return value.
Add the following documentation:
+ /**
+ * Display version compatibility notice for Dokan Pro
+ *
+ * @since 3.14.0
+ *
+ * @param array $notices Array of admin notices
+ *
+ * @return array Modified array of admin notices
+ */
public function show_admin_commission_notice( $notices ) {
📝 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 show_admin_commission_notice( $notices ) { | |
/** | |
* Display version compatibility notice for Dokan Pro | |
* | |
* @since 3.14.0 | |
* | |
* @param array $notices Array of admin notices | |
* | |
* @return array Modified array of admin notices | |
*/ | |
public function show_admin_commission_notice( $notices ) { |
includes/Admin/Notices/Manager.php
Outdated
'title' => __( 'Upgrade to Dokan Pro Latest Version', 'dokan-lite' ), | ||
'description' => __( 'Your Dokan Lite version is updated to the latest version but you are still using the Dokan pro old version. Upgrade to Dokan Pro Latest version to enable the new features.', 'dokan-lite' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls consult about the title
and description
with the @getdokan/product team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
includes/Admin/Notices/Manager.php (1)
195-199
: Consider extracting version comparison logic.The version comparison logic could be made more maintainable by extracting it into a separate method, especially if similar checks might be needed elsewhere.
+ /** + * Check if Dokan Pro needs version compatibility notice + * + * @since 3.14.0 + * + * @return bool + */ + private function needs_version_compatibility_notice() { + return version_compare( DOKAN_PLUGIN_VERSION, '3.14.0', '>=' ) && + defined( 'DOKAN_PRO_PLUGIN_VERSION' ) && + version_compare( DOKAN_PRO_PLUGIN_VERSION, '3.14.0', '<' ); + } + public function show_admin_commission_notice( $notices ) { - if ( - version_compare( DOKAN_PLUGIN_VERSION, '3.14.0', '>=' ) && - defined( 'DOKAN_PRO_PLUGIN_VERSION' ) && - version_compare( DOKAN_PRO_PLUGIN_VERSION, '3.14.0', '<' ) - ) { + if ( $this->needs_version_compatibility_notice() ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Admin/Notices/Manager.php
(2 hunks)
🔇 Additional comments (4)
includes/Admin/Notices/Manager.php (4)
53-53
: LGTM!
The hook registration follows the established pattern and is correctly placed within the init_hooks
method.
194-194
: Add PHPDoc block for the method.
Following WordPress coding standards and maintaining consistency with other methods in this class, please add a PHPDoc block documenting the method's purpose, parameters, and return value.
200-207
: Review notice configuration with product team.
As mentioned in the previous review comments, please consult with @getdokan/product team about:
- The notice title "Dokan Update Required"
- The description message about version compatibility
- The decision to make this notice non-dismissible (show_close_button: false)
Additional considerations:
- The notice uses priority 1, making it highly visible
- The 'alert' type is appropriate for version incompatibility
- The 'global' scope ensures visibility across admin pages
196-198
: Verify constant definitions.
Let's verify that these constants are properly defined in their respective plugins.
✅ Verification successful
Let me analyze the findings:
DOKAN_PLUGIN_VERSION
is properly defined indokan-class.php
using the$this->version
value.- For
DOKAN_PRO_PLUGIN_VERSION
, we see it being used in multiple places but don't see its definition. Since this is a pro version constant, it's likely defined in the pro plugin which is a separate package.
Let's verify if there are any other files that might contain the pro version constant definition:
Constants usage appears correct in this context
The code is checking for version compatibility between Dokan Lite (DOKAN_PLUGIN_VERSION
) and Dokan Pro (DOKAN_PRO_PLUGIN_VERSION
). From the verification:
DOKAN_PLUGIN_VERSION
is properly defined indokan-class.php
DOKAN_PRO_PLUGIN_VERSION
is used in multiple places with properdefined()
checks- The absence of
DOKAN_PRO_PLUGIN_VERSION
definition in this codebase is expected since it belongs to the pro version plugin, which is a separate package
The code follows best practices by:
- Using
defined()
checks before accessing the constants - Properly comparing versions for compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constant definitions in the codebase
echo "Searching for DOKAN_PLUGIN_VERSION..."
rg "define.*DOKAN_PLUGIN_VERSION" -A 2
echo "Searching for DOKAN_PRO_PLUGIN_VERSION..."
rg "define.*DOKAN_PRO_PLUGIN_VERSION" -A 2
Length of output: 1343
Script:
#!/bin/bash
# Search for files with "dokan" and "pro" in their name that might contain the version definition
fd -i dokan.*pro -x rg "define.*DOKAN.*VERSION" {} \;
# Also search for any bootstrap or main plugin files
fd -e php -x rg "define.*DOKAN_PRO.*VERSION" {} \;
Length of output: 339
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
includes/Admin/Notices/Manager.php (1)
204-208
: Add defensive checks for undefined constantsThe version comparison logic should handle cases where constants might not be defined.
if ( version_compare( DOKAN_PLUGIN_VERSION, '3.14.0', '>=' ) && - defined( 'DOKAN_PRO_PLUGIN_VERSION' ) && - version_compare( DOKAN_PRO_PLUGIN_VERSION, '3.14.0', '<' ) + defined( 'DOKAN_PRO_PLUGIN_VERSION' ) && DOKAN_PRO_PLUGIN_VERSION && + version_compare( (string) DOKAN_PRO_PLUGIN_VERSION, '3.14.0', '<' ) ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Admin/Notices/Manager.php
(2 hunks)
🔇 Additional comments (3)
includes/Admin/Notices/Manager.php (3)
53-53
: LGTM: Hook registration is correct
The filter hook registration follows WordPress coding standards and is consistently placed with other notice hooks.
203-220
: LGTM: Core functionality is well implemented
The notice implementation:
- Correctly checks version compatibility
- Uses appropriate notice type (alert) for version mismatch
- Makes the notice non-dismissible due to its importance
- Has global scope to ensure visibility
209-216
: Review notice content with product team
As mentioned in past review comments, please consult with @getdokan/product team about the notice title and description to ensure the messaging is clear and effective.
✅ Verification successful
Let me search for more context about update notices to better understand the messaging patterns.
Let me search for any documentation about notice standards or messaging guidelines.
Based on the search results, I can now generate a final response. The search shows several update notices in the codebase, and the messaging follows a consistent pattern of being clear, concise, and informative about version requirements and compatibility.
The notice content is appropriate and follows established patterns
The current notice messaging is consistent with other update notices in the codebase and effectively communicates:
- A clear title indicating the required action ("Dokan Update Required")
- A concise description explaining both the reason ("to ensure feature compatibility and accessibility") and the specific requirement ("Dokan Pro minimum v3.14.0")
The messaging follows good practices by:
- Being direct and actionable
- Providing specific version information
- Explaining the benefit/reason for the update
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if similar notices exist in the codebase for consistency in messaging
rg -B 2 -A 2 "'title'\s*=>\s*__\(\s*'Dokan Update Required'" --type php
Length of output: 616
Script:
#!/bin/bash
# Search for similar update-related notices in the codebase
rg -B 2 -A 2 "'title'\s*=>\s*__\(\s*['\"].*(?i)(update|upgrade|version).*['\"]" --type php
# Also check for any documentation about notices
fd "README|CONTRIBUTING" --type f --exec cat {}
Length of output: 41754
Script:
#!/bin/bash
# Search for any documentation files that might contain notice guidelines
fd "CONTRIBUTING|DEVELOPMENT|STANDARDS|GUIDELINES" --type f --exec cat {}
# Also search for similar update notices in other files
rg -B 2 -A 2 "'description'\s*=>\s*__\(\s*['\"].*(?i)(compatibility|update|upgrade|version).*['\"]" --type php
Length of output: 6362
/** | ||
* Show admin notice if dokan lite is updated to v3.14.0 and dokan pro is not updated to minimum v3.14.0. | ||
* | ||
* @since DOKAN_SINCE | ||
* | ||
* @param $notices | ||
* | ||
* @return mixed | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Update PHPDoc block with proper version and types
The PHPDoc block needs improvements:
- Replace
DOKAN_SINCE
with actual version number - Add proper parameter and return type documentation
/**
* Show admin notice if dokan lite is updated to v3.14.0 and dokan pro is not updated to minimum v3.14.0.
*
- * @since DOKAN_SINCE
+ * @since 3.14.0
*
- * @param $notices
+ * @param array $notices Array of admin notices
*
- * @return mixed
+ * @return array Modified array of admin notices
*/
📝 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.
/** | |
* Show admin notice if dokan lite is updated to v3.14.0 and dokan pro is not updated to minimum v3.14.0. | |
* | |
* @since DOKAN_SINCE | |
* | |
* @param $notices | |
* | |
* @return mixed | |
*/ | |
/** | |
* Show admin notice if dokan lite is updated to v3.14.0 and dokan pro is not updated to minimum v3.14.0. | |
* | |
* @since 3.14.0 | |
* | |
* @param array $notices Array of admin notices | |
* | |
* @return array Modified array of admin notices | |
*/ |
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