Enhance : Admin Notice Show gloablly & extand API #2446
Conversation
WalkthroughThe changes in this pull request introduce new functionality for handling admin notices within the Dokan plugin. A new JavaScript file is added to manage these notices, while modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (8)
src/admin/notice/App.vue (2)
1-4: Add prop type validation and consider reactive interval handling.Since this is a new component, it would be beneficial to:
- Add prop type validation for the AdminNotice component
- Consider making the interval reactive in case it needs to be adjusted dynamically
<script setup> +import { defineProps } from 'vue'; import AdminNotice from "admin/components/AdminNotice.vue"; + +const props = defineProps({ + defaultInterval: { + type: Number, + default: 10000 + } +}); </script>
11-13: Remove empty style block.Since the style block is empty, it should be removed to maintain clean code. You can add it back when styles are needed.
-<style scoped lang="less"> - -</style>src/admin/notice/main.js (2)
6-11: Enhance initialization robustness and error handling.The current implementation could be improved in several ways:
- Add error handling for Vue initialization
- Implement proper cleanup on unmount
- Add loading state management
Consider applying this enhanced implementation:
-if ( $( '#dokan-admin-notices' ).length ) { - new Vue( { - el: '#dokan-admin-notices', - render: ( h ) => h( App ), - } ); -} +const initializeNotices = () => { + const mountPoint = document.getElementById('dokan-admin-notices'); + if (!mountPoint) return; + + try { + const app = new Vue({ + render: (h) => h(App), + }); + + // Mount with proper error handling + app.$mount(mountPoint); + + // Cleanup on admin page changes + window.addEventListener('unload', () => { + app.$destroy(); + }); + } catch (error) { + console.error('Failed to initialize Dokan admin notices:', error); + } +}; + +// Initialize when DOM is ready +$(document).ready(initializeNotices);
1-11: Consider adding TypeScript and Prop validation.To improve maintainability and prevent runtime errors:
- Consider migrating to TypeScript for better type safety
- Add prop validation for the App component
- Document the expected props and events
Example prop validation in App.vue:
export default { name: 'App', props: { context: { type: String, default: 'global', validator: value => ['global', 'local'].includes(value) }, interval: { type: Number, default: 30000 } } }includes/Admin/Notices/Helper.php (2)
13-18: LGTM! Consider adding PHPDoc for the property.The scope-type mapping is well-structured and logically organized. However, adding PHPDoc would improve code documentation.
Add documentation above the property:
+/** + * Mapping of notice scopes to their allowed notice types. + * + * @var array<string, string[]> + */ private static array $scope_type_mapping = [
167-185: LGTM! Fix indentation in the closure.The filtering implementation is clean and efficient. However, there's an indentation inconsistency in the closure.
Fix the indentation to match the rest of the file:
private static function filter_notices_by_type( array $notices, array $allowed_types ): array { if ( ! empty( $allowed_types ) ) { $notices = array_filter( $notices, function ( $notice ) use ( $allowed_types ) { - return in_array( $notice['type'], $allowed_types, true ); + return in_array( $notice['type'], $allowed_types, true ); } ); } return $notices; }src/admin/components/AdminNotice.vue (2)
65-68: Add validation for the context prop.Consider adding prop validation to ensure only valid context values are passed. This helps prevent potential issues with invalid contexts being sent to the API.
context: { type: String, default: 'all', + validator: function(value) { + return ['all', 'global', 'local'].includes(value) + } }
Line range hint
1-90: Consider splitting into smaller, more focused components.The component currently handles multiple responsibilities. Consider breaking it down into smaller, more maintainable components:
- A container component for data fetching and state management
- A presentation component for displaying individual notices
- A separate component for slide controls
This would improve:
- Code maintainability
- Component reusability
- Testing capabilities
Example structure:
<!-- AdminNoticeContainer.vue --> <template> <admin-notice-display :notices="notices" :loading="loading" @hide="hideNotice" @action="handleAction" /> </template> <!-- AdminNoticeDisplay.vue --> <template> <div class="notice dokan-admin-notices-wrap"> <!-- Notice display logic --> <slide-controls v-if="notices.length > 1" /> </div> </template> <!-- SlideControls.vue --> <template> <div class="slide-notice"> <!-- Slide control logic --> </div> </template>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
includes/Admin/Notices/Helper.php(3 hunks)includes/Assets.php(2 hunks)includes/REST/AdminNoticeController.php(3 hunks)src/admin/components/AdminNotice.vue(2 hunks)src/admin/notice/App.vue(1 hunks)src/admin/notice/main.js(1 hunks)webpack.config.js(1 hunks)
🔇 Additional comments (10)
src/admin/notice/App.vue (1)
7-7: Verify API endpoint integration.
The component uses the "admin" endpoint. Let's verify this endpoint exists and handles the global context correctly.
✅ Verification successful
API endpoint integration is properly implemented
The AdminNoticeController correctly implements the "admin" endpoint at /notices/admin with proper REST route registration. The endpoint:
- Handles the global context parameter as required
- Includes appropriate permission checks
- Returns notices via the Helper::dokan_get_admin_notices method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API endpoint registration matching the admin endpoint
rg -A 5 "register_rest_route.*admin.*notice"
# Search for notice-related API handlers
ast-grep --pattern 'class AdminNoticeController {
$$$
public function $_(WP_REST_Request $_) {
$$$
}
$$$
}'
Length of output: 168
Script:
#!/bin/bash
# Search for API route registrations with broader pattern
rg -A 5 "register_rest_route.*admin"
# Search for notice-related files
fd -e php notice
# Search for notice-related API handlers with a more general pattern
ast-grep --pattern 'function $_(WP_REST_Request $_) {
$$$
}'
# Look for REST API related configuration
rg -A 5 "rest_api_init"
Length of output: 1230
Script:
#!/bin/bash
# Check the content of AdminNoticeController.php
cat includes/REST/AdminNoticeController.php
# Check if Manager.php registers the admin notice routes
rg -A 10 "register_rest_routes.*function" includes/REST/Manager.php
# Look for notice-related route registrations in the REST Manager
rg "notice" includes/REST/Manager.php
Length of output: 2470
src/admin/notice/main.js (1)
6-11: Verify WordPress admin integration points.
Let's ensure this component properly integrates with WordPress admin pages and doesn't conflict with other plugins.
✅ Verification successful
Let me gather more information about the admin notice integration by checking the PHP templates and script registration.
Based on the results, I can now provide a final response since I have enough information to verify the WordPress admin integration:
Vue instance and mount point integration looks correct
The implementation properly integrates with WordPress admin by:
- Using a unique mount point ID
dokan-admin-noticesthat doesn't conflict with other plugins - Properly registering scripts via
wp_enqueue_script()with dependencies - Having proper template integration in
templates/admin-notice-dependencies.php - Following WordPress admin notice markup conventions
The only other Vue instances found are in separate components that mount to different elements, so there are no conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential conflicts with other plugins using the same mount point
rg -l "dokan-admin-notices" --type html
rg -l "dokan-admin-notices" --type php
# Check for other Vue instances that might conflict
rg "new Vue\(" --type js
# Verify proper script registration
rg -A 5 "wp_enqueue_script.*dokan.*notice" --type php
Length of output: 1086
Script:
#!/bin/bash
# Check the admin notice template content
cat templates/admin-notice-dependencies.php
# Check how the notice manager initializes the mount point
cat includes/Admin/Notices/Manager.php
# Check script dependencies and registration
rg "dokan-admin-notice-js|dokan-promo-notice-js" includes/Assets.php -A 10
# Check if there are any other plugins using Vue in admin
rg "Vue\.use|createApp" --type js
Length of output: 142259
includes/REST/AdminNoticeController.php (2)
8-8: LGTM!
The WP_REST_Request import is properly added and organized with other WordPress core imports.
39-39: Verify permission callback implementation for security.
The endpoint uses check_permission callback inherited from DokanRESTAdminController. Let's verify its implementation to ensure proper access control.
✅ Verification successful
Permission callback implementation verified and secure
The check_permission method in DokanRESTAdminController properly restricts access to users with manage_woocommerce capability, which is appropriate for admin-level REST endpoints. This is a secure implementation as it ensures only store administrators can access these notice endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the parent class implementation of check_permission
ast-grep --pattern 'class DokanRESTAdminController {
$$$
public function check_permission() {
$$$
}
$$$
}'
Length of output: 120
Script:
#!/bin/bash
# Let's try to find the DokanRESTAdminController class file first
fd -e php DokanRESTAdminController
# Then search for check_permission method across PHP files
rg -U "function check_permission" --type php -A 5
Length of output: 582
webpack.config.js (2)
19-19: LGTM! Entry point addition follows established patterns.
The new entry point for admin notices is properly configured and maintains consistency with existing Vue-based admin features.
19-19: Verify the referenced notice implementation file exists.
Let's ensure the referenced file and its dependencies are properly structured.
✅ Verification successful
The notice implementation file and its dependencies exist and are properly structured
The verification confirms:
- Entry point
main.jsexists at the correct path - Vue component structure is present with
App.vue - The component contains notice-related code with
AdminNoticereferences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and basic structure of the admin notice implementation
# Check if the entry file exists
echo "Checking for main.js..."
fd --type f "main.js" "src/admin/notice"
# Verify the Vue component structure
echo "Checking for related Vue components..."
fd --type f ".*\.vue$" "src/admin/notice"
# Look for notice-related imports/exports
echo "Checking notice-related code..."
rg -l "AdminNotice" "src/admin/notice"
Length of output: 409
includes/Admin/Notices/Helper.php (1)
27-38: LGTM! Verify scope usage across the codebase.
The implementation is solid with good backward compatibility and proper filter usage.
Let's verify the notice scope usage across the codebase:
✅ Verification successful
Implementation verified and properly used across the codebase
The changes are correctly implemented and used:
- REST endpoint properly passes the context parameter to the helper method
- Default parameter 'all' maintains backward compatibility
- Scope types are well-defined in the static mapping array
- The only usage is in the REST controller which correctly handles the parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to dokan_get_admin_notices to ensure they're updated with the new parameter
ast-grep --pattern 'dokan_get_admin_notices($$$)'
# Search for filter usage to understand how the scope might be modified
rg "apply_filters.*dokan_admin_notices_scope"
Length of output: 254
Script:
#!/bin/bash
# Let's search for actual usage of the method to verify if the new parameter is being used correctly
rg "dokan_get_admin_notices" -A 2
# Let's also check for any class references to understand the context better
rg "scope_type_mapping" -A 2
Length of output: 1646
src/admin/components/AdminNotice.vue (1)
Line range hint 1-1: Verify component usage and API endpoint compatibility.
Let's verify the integration points for this enhanced component.
✅ Verification successful
Component and API endpoint changes are compatible
The verification shows that both the component usage and API endpoint implementation are properly aligned:
- The API endpoint in
AdminNoticeController.phpcorrectly handles thecontextparameter and passes it to the Helper class - The Helper class has proper scope type mapping for different contexts (
global,local,all) - Current component usage shows two patterns:
- Explicit context usage:
<AdminNotice context="global" ...> - Default usage:
<AdminNotice>which will default to 'all' context
- Explicit context usage:
The changes are backward compatible as the context parameter is optional in both the component and the API endpoint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify component usage and API endpoint compatibility
# Check for component usage with context prop
echo "Checking component usage..."
rg -l "AdminNotice" --type vue
# Check API endpoint implementation
echo "Checking API endpoint implementation..."
rg -l "dokan_get_admin_notices" --type php
# Check for any hardcoded notice contexts
echo "Checking for hardcoded contexts..."
rg -i "context.*['\"](?:all|global|local)['\"]" --type vue --type php
Length of output: 512
Script:
#!/bin/bash
# Let's check the content of the files we found and also search for Vue files correctly
# Check Vue component usage (using fd to find .vue files)
echo "=== Checking Vue component usage ==="
fd -e vue -x rg -l "AdminNotice"
# Check the implementation of the notice API endpoint
echo -e "\n=== Checking API endpoint implementation ==="
cat includes/REST/AdminNoticeController.php
# Check notice helper implementation
echo -e "\n=== Checking notice helper implementation ==="
cat includes/Admin/Notices/Helper.php
# Check for context parameter in both Vue and PHP files
echo -e "\n=== Checking for context parameter usage ==="
fd -e vue -e php -x rg -l "context.*['\"](?:all|global|local)['\"]"
Length of output: 9222
Script:
#!/bin/bash
# Let's check the actual implementation in App.vue and AdminNotice usage
# Check the notice App implementation
echo "=== Checking notice App.vue implementation ==="
cat src/admin/notice/App.vue
# Check how AdminNotice is used in other components
echo -e "\n=== Checking AdminNotice usage in other components ==="
fd -e vue -x rg -A 2 -B 2 "<AdminNotice"
Length of output: 2147
includes/Assets.php (2)
539-543: LGTM! Script registration follows best practices.
The script registration:
- Uses proper dependencies (jQuery, Vue vendor)
- Implements cache busting through filemtime
- Follows the same pattern as other scripts in the file
✅ Verification successful
Script registration is correct and files are properly structured
The verification confirms:
- Source file exists at correct location:
src/admin/notice/main.js - Entry point is properly configured in webpack.config.js
- File structure follows the project's build conventions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the admin notice script file exists and is properly built
# Test: Check if the JS file exists at the expected location
fd -t f "dokan-admin-notice.js$" assets/js/
# Test: Check if the file is included in build process
rg "dokan-admin-notice" webpack.config.js
Length of output: 141
Script:
#!/bin/bash
# Verify the source file exists and check its content
fd -t f "main.js$" src/admin/notice/
# Check if there are any imports of dokan-vue-vendor in the source file
rg -A 3 "import.*dokan-vue-vendor" src/admin/notice/main.js
# Check if the built file exists in assets
fd -t f "dokan-admin-notice.js$" assets/
Length of output: 168
38-38: Verify script dependencies for admin notices.
The script enqueue looks good, but since it shares the same localization data as dokan-promo-notice-js, we should verify if it needs any additional dependencies or localized data specific to admin notices.
✅ Verification successful
Let me gather more information about the script registration and dependencies.
Let me check the script registration in the Assets class to understand the complete context.
Based on the code analysis, I can now generate the final response:
Script dependencies and localization are properly configured
The script dokan-admin-notice-js is correctly registered with:
- Dependencies on jQuery and Vue vendor
- Proper version control using file modification time
- Shared localization data with
dokan-promo-notice-jswhich includes necessary REST API endpoints and asset URLs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the admin notice script has any unique dependencies or localization needs
# Test: Search for any unique admin notice related dependencies or localization
rg -A 5 "dokan-admin-notice" src/
Length of output: 1619
Script:
#!/bin/bash
# Check script registration and dependencies
rg -B 5 "wp_enqueue_script.*dokan-admin-notice-js" --type php
# Check if there are any localization data being passed
rg "wp_localize_script.*dokan-admin-notice" --type php
# Look for the script file registration
rg "wp_register_script.*dokan-admin-notice" --type php
Length of output: 497
Script:
#!/bin/bash
# Check the complete Assets class for script registration
cat includes/Assets.php
Length of output: 60909
osmansufy
added
the
Needs: Dev Review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/admin/components/AdminNotice.vue (2)
65-68: Add prop validation and documentation for the scope prop.Consider adding:
- JSDoc documentation to explain the purpose and expected values of the scope prop
- Prop validation to ensure only valid scope values are passed
+/** + * @prop {String} scope - Filters notices by scope (e.g., 'global', 'local') + * Valid values: '', 'global', 'local' + */ scope: { type: String, default: '', + validator: function(value) { + return ['', 'global', 'local'].includes(value) + } }
Line range hint
65-97: Add watcher for scope changes.When the scope prop changes, the notices should be re-fetched to reflect the new filtering.
export default { // ... existing code ... + watch: { + scope: { + handler(newScope, oldScope) { + if (newScope !== oldScope) { + this.fetch(); + } + } + } + }, // ... existing code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
includes/Admin/Notices/Helper.php(2 hunks)includes/REST/AdminNoticeController.php(3 hunks)src/admin/components/AdminNotice.vue(2 hunks)src/admin/notice/App.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- includes/Admin/Notices/Helper.php
- includes/REST/AdminNoticeController.php
- src/admin/notice/App.vue
🔇 Additional comments (2)
src/admin/components/AdminNotice.vue (2)
Line range hint 90-97: Previous review comment about error handling is still applicable.
The fetch implementation still needs error handling and loading state management as highlighted in the previous review.
65-68: Verify consistent usage of the scope parameter.
Let's ensure the scope parameter is being used consistently across all instances of the AdminNotice component.
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
includes/REST/AdminNoticeController.php (1)
107-117: Enhance notice filtering robustness and efficiencyThe filter implementation could be improved for better robustness and efficiency:
- Add validation for notice array structure
- Consider using array_column for more efficient filtering
- Add type hints for better code maintainability
Apply this diff to improve the implementation:
- private static function filter_notices_by_scope( array $notices, string $allowed_scope = '' ): array { + private static function filter_notices_by_scope( array $notices, string $allowed_scope = '' ): array { if ( empty( $allowed_scope ) ) { return $notices; } - return array_filter( - $notices, function ( $notice ) use ( $allowed_scope ) { - return $notice['scope'] === $allowed_scope; - } - ); + // Validate notice structure + $notices = array_filter( $notices, function ( $notice ) { + return is_array( $notice ) && isset( $notice['scope'] ); + } ); + + // Filter by scope using array_column for better performance + $scopes = array_column( $notices, 'scope' ); + return array_intersect_key( $notices, array_flip( + array_keys( $scopes, $allowed_scope, true ) + ) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
includes/REST/AdminNoticeController.php(4 hunks)src/admin/components/AdminNotice.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/admin/components/AdminNotice.vue
🔇 Additional comments (2)
includes/REST/AdminNoticeController.php (2)
8-8: LGTM!
The WP_REST_Request import is correctly placed and necessary for the updated method signature.
69-85: Verify API integration with frontend components
Let's verify the integration with the Vue components mentioned in the summary.
✅ Verification successful
Let me generate additional verification scripts to check the frontend integration more thoroughly.
Let me check one more aspect to verify the component props definition.
Based on the verification results, I can now provide a final response:
API integration with frontend is properly implemented
The Vue component correctly handles the scope parameter:
- The component has a
scopeproperty defined - The API call properly appends the scope parameter to the URL:
?scope=${this.scope} - The endpoint integration matches the backend changes in AdminNoticeController.php
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check frontend integration with the new scope parameter
# Check Vue component usage of the scope parameter
echo "Checking Vue component integration..."
rg -A 5 "scope.*prop" --type vue
# Check API calls using the scope parameter
echo "Checking API calls..."
rg -A 5 "dokan.*notices.*scope" --type js
Length of output: 108945
Script:
#!/bin/bash
# Check Vue component props and API integration
# Check Vue component scope prop and usage
echo "Checking Vue component scope handling..."
rg -A 5 "scope.*=" src/admin/components/AdminNotice.vue
# Check API endpoint handling
echo "Checking API endpoint handling..."
rg -A 5 "dokan.*notices.*admin" src/admin/components/AdminNotice.vue
# Check if scope is passed in component initialization
echo "Checking component initialization..."
rg -A 5 "AdminNotice.*scope" src/admin
Length of output: 799
Script:
#!/bin/bash
# Check Vue component props definition
# Look for scope prop definition in AdminNotice component
echo "Checking component props..."
rg "props.*{" -A 10 src/admin/components/AdminNotice.vue
# Check for any other scope-related code
echo "Checking other scope usage..."
rg "scope" src/admin/components/AdminNotice.vue
Length of output: 534
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
includes/Upgrade/AdminNotice.php (1)
42-42: LGTM! The scope addition aligns with the global notice enhancement.The addition of
'scope' => 'global'is consistent with the PR's objective of enabling global admin notices. The change integrates well with the existing permission checks and upgrade process controls.Consider documenting the available scope values and their implications in the class-level PHPDoc, as this appears to be part of a larger architectural change involving REST API and frontend components.
includes/REST/AdminNoticeController.php (5)
42-42: Add @SInCE tag to document when the scope parameter was introducedAdd a
@sincetag to document the version when this parameter was introduced, helping with backward compatibility considerations.- 'description' => __( 'Notice context', 'dokan-lite' ), + 'description' => __( 'Notice context', 'dokan-lite' ), // @since x.x.x
69-71: Add type declarations for better code clarityConsider adding return type declarations and improving parameter type hints for better code clarity and maintainability.
- public function dokan_get_admin_notices( WP_REST_Request $request ) { + public function dokan_get_admin_notices( WP_REST_Request $request ): WP_REST_Response {
102-102: Replace DOKAN_SINCE placeholder with actual versionThe @SInCE tag contains a placeholder that needs to be replaced with the actual version number.
- * @since DOKAN_SINCE + * @since 3.7.0
115-117: Fix indentation in array_filter callbackThe indentation in the array_filter callback is inconsistent with the rest of the file. It uses tabs instead of spaces.
return array_filter( $notices, function ( $notice ) use ( $allowed_scope ) { - return $notice['scope'] === $allowed_scope; - } + return $notice['scope'] === $allowed_scope; + } );
110-112: Improve empty check for allowed_scopeThe current empty check might allow some edge cases through. Consider using a more specific check.
- if ( empty( $allowed_scope ) ) { + if ( ! is_string( $allowed_scope ) || '' === trim( $allowed_scope ) ) { return $notices; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
includes/REST/AdminNoticeController.php(4 hunks)includes/Upgrade/AdminNotice.php(1 hunks)src/admin/components/AdminNotice.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/admin/components/AdminNotice.vue
🔇 Additional comments (2)
includes/Upgrade/AdminNotice.php (2)
Line range hint 69-94: Robust security measures are maintained.
The upgrade process maintains strong security controls:
- Permission verification via
current_user_can - AJAX nonce verification
- Proper exception handling
- Prevention of concurrent upgrades
Line range hint 16-58: Verify integration with related components.
The show_notice method's new scope parameter should be properly handled by the related components.
Let's verify the integration points:
✅ Verification successful
Let me generate additional scripts to verify the scope parameter integration:
The scope parameter is properly integrated across components
The verification shows that the scope parameter is correctly handled:
-
The AdminNoticeController properly processes scope:
- Sets default scope as 'local' if not provided
- Has dedicated filter_notices_by_scope() method
- Applies scope filtering before returning notices
-
The changes in AdminNotice.php align with this implementation by adding the scope parameter to notice data structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the handling of the 'scope' parameter across related files
# Check if AdminNoticeController handles the scope parameter
echo "Checking AdminNoticeController for scope parameter handling..."
rg -A 5 "scope.*WP_REST_Request" "includes/REST"
# Check if Vue component accepts scope prop
echo "Checking Vue component for scope prop..."
rg -A 5 "scope.*prop" "src/admin/components"
# Check if the notice JS file uses the scope parameter
echo "Checking JS implementation..."
rg -A 5 "scope.*notice" "assets/js"
Length of output: 131497
Script:
#!/bin/bash
# Check Vue component for scope handling
echo "Checking Vue component for scope handling..."
ast-grep --pattern $'class AdminNotice {
$$$
scope: {
$$$
}
$$$
}'
# Check if scope is used in the template
echo "Checking template for scope usage..."
rg -A 5 'scope.*v-if|scope.*v-show' "src/admin/components"
# Check if scope is used in the notice rendering logic
echo "Checking notice rendering logic..."
rg -A 5 'scope.*notice' "includes/REST"
Length of output: 2039
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
includes/REST/AdminNoticeController.php (2)
40-49: LGTM: Well-structured parameter definition with one minor suggestionThe 'scope' parameter is well-defined with proper validation, sanitization, and documentation. Consider adding examples to the description to make it even clearer.
- 'description' => __( 'Choose notice scope: "local" displays only on Dokan pages, "global" displays across the entire site.', 'dokan-lite' ), + 'description' => __( 'Choose notice scope. Example: Use "local" for Dokan pages only (default), "global" for site-wide display.', 'dokan-lite' ),
77-85: Simplify and document the filtering logicThe filtering logic can be simplified and better documented.
// Filter notices by scope + // First filter by scope, then reset array keys to ensure sequential numeric indexing $filter_notices = array_filter( $notices, function ( $notice ) use ( $notice_scope ) { - return $notice['scope'] === ( $notice_scope ?? 'local' ); + return $notice['scope'] === $notice_scope; } ); $filter_notices = array_values( $filter_notices );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
includes/REST/AdminNoticeController.php(3 hunks)src/admin/components/AdminNotice.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/admin/components/AdminNotice.vue
🔇 Additional comments (1)
includes/REST/AdminNoticeController.php (1)
8-8: LGTM: Clean import addition
The WP_REST_Request import is properly added and necessary for type-hinting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
includes/REST/AdminNoticeController.php (2)
71-73: Consider using null coalescing operator for cleaner default handling.The current default value handling can be simplified.
- $notice_scope = $request->get_param( 'scope' ); - $notice_scope = ! empty( $notice_scope ) ? $notice_scope : 'local'; + $notice_scope = $request->get_param( 'scope' ) ?: 'local';
78-86: Simplify the filtering logic.The current implementation can be made more concise while maintaining readability.
- $filter_notices = array_filter( - $notices, - function ( $notice ) use ( $notice_scope ) { - $notice['scope'] = $notice['scope'] ?? 'local'; - - return $notice['scope'] === $notice_scope; - } - ); - $filter_notices = array_values( $filter_notices ); + $filter_notices = array_values(array_filter( + $notices, + fn($notice) => ($notice['scope'] ?? 'local') === $notice_scope + ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/REST/AdminNoticeController.php(3 hunks)
🔇 Additional comments (3)
includes/REST/AdminNoticeController.php (3)
8-8: LGTM: Import statement follows WordPress standards.
The addition of the WP_REST_Request import is appropriate for the type-hinting used in the updated method signature.
40-49: LGTM: Parameter definition follows best practices.
The 'scope' parameter is well-defined with:
- Clear description of valid values and their effects
- Proper enum validation
- Appropriate sanitization
71-88: Verify frontend handling of filtered notices.
The changes to notice filtering look good, but let's verify the frontend implementation.
✅ Verification successful
Frontend properly handles notice scope parameter
The changes to notice filtering are well-integrated with the frontend implementation:
- The AdminNotice.vue component accepts a
scopeprop with proper type definition - The component correctly passes the scope parameter to the API endpoint via
?scope=${this.scope} - The API endpoint properly handles the scope parameter with a default value of 'local'
The implementation shows proper coordination between frontend and backend for notice scope filtering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check frontend components for proper handling of notice scope
# Check Vue components for notice scope handling
rg -l "scope.*notice" --type vue
rg "scope.*prop" --type vue -A 5
# Check JS files for notice API calls
rg "notices/admin.*scope" --type js -A 5
Length of output: 168
Script:
#!/bin/bash
# Let's try different approaches to find frontend notice handling
# Look for notice-related files in common frontend directories
fd -e js -e jsx -e ts -e tsx notice
# Search for admin notice API calls and scope parameter
rg -t js "dokan_get_admin_notices|notices/admin"
# Search for notice-related components
rg -t js "AdminNotice|NoticeComponent"
# Look for files that might handle notices in assets
fd -e js notice assets/
Length of output: 133745
Script:
#!/bin/bash
# Let's check the AdminNotice component implementation and scope handling
# Look for AdminNotice component definition
ast-grep --pattern 'const AdminNotice = {
$$$
props: {
$$$
scope: $_
$$$
}
$$$
}'
# Search for scope parameter in API calls
rg -t js "dokan_get_admin_notices.*scope|notices/admin.*scope" -A 5
# Check for scope-related code in Vue components
fd -e vue . | xargs rg "scope.*notice" -l
Length of output: 217
Script:
#!/bin/bash
# Let's try a different approach to find the AdminNotice component and scope handling
# Look for AdminNotice component file
fd -e vue AdminNotice
# Search for the component content
rg -t js "name: ['\"]AdminNotice" -A 20
# Search for admin notice API calls in JS files
rg "dokan_get_admin_notices|notices/admin" -A 5
# Look for scope-related code in Vue files
fd -e vue . | xargs cat | rg "scope.*notice|notice.*scope"
Length of output: 4409
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
includes/REST/AdminNoticeController.php (1)
40-49: LGTM: Parameter definition follows WordPress REST API standardsThe 'scope' parameter is well-defined with proper validation, sanitization, and clear description. Consider adding the allowed values to the description for better API documentation.
- 'description' => __( 'Choose notice scope: "local" displays only on Dokan pages, "global" displays across the entire site.', 'dokan-lite' ), + 'description' => __( 'Choose notice scope. Allowed values: "local" (displays only on Dokan pages), "global" (displays across the entire site).', 'dokan-lite' ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/REST/AdminNoticeController.php(3 hunks)
🔇 Additional comments (2)
includes/REST/AdminNoticeController.php (2)
8-8: LGTM: Import statement is correctly placed
The WP_REST_Request import is properly added and necessary for the type-hinting.
77-84: LGTM: Notice filtering implementation is correct
The filtering logic is well-implemented:
- Properly uses array_filter with a closure
- Correctly handles undefined scope values
- Maintains array indexing with array_values
|
@getdokan/product, We may need to update the content of the Dokan data migration notice. |
shashwatahalder01
added
QA In Progress
and removed
Needs: Testing
|
@mrabbani vai, Kindly direct to change the text on Dokan Data Update Required announcement according to below. "Updating your Dokan data is required to continue functional operations." |
|
Issue: Clicking on upgrader button doesn't work if notice is set to global scope Please refer to the video attached https://jam.dev/c/db9e3cf0-3a63-4bbb-b705-afef35d1ce6a @mralaminahamed bhai |
|
@StalinDurjo vai, issue fix kore diachi. let check the update |
shashwatahalder01
added
QA approved
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
Release Notes
New Features
scopeprop to filter admin notices based on context.scopeparameter for REST API requests.Bug Fixes
Documentation
Chores