enhance: Add analytics cache modifier for seller analytics filter. #2529
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant SP as ServiceProvider
participant CM as CacheKeyModifier
participant WC as WooCommerce Analytics
SP->>CM: Register Service
CM->>CM: Setup Entities
CM->>CM: Register Hooks
WC->>CM: Request Analytics
CM->>CM: Apply Seller Filter
CM-->>WC: Return Filtered Query Args
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (3)
includes/Analytics/Reports/SellerDataModifier.php (3)
12-22: Replace version placeholders or define dedicated constantsCurrently, the doc tags use
DOKAN_SINCE. Please replace it with the actual plugin version or define a constant for better clarity.
39-59: Consider extracting entity definitionsDefining these analytics entities in a single array is straightforward. For scalability, consider extracting them into constants or a dedicated config to streamline maintainability.
99-110: Add coverage for seller filteringConsider writing unit or integration tests to ensure the seller filtering logic behaves correctly for valid, invalid, and edge-case seller IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
includes/Analytics/Reports/BaseQueryFilter.php(1 hunks)includes/Analytics/Reports/SellerDataModifier.php(1 hunks)includes/DependencyManagement/Providers/AnalyticsServiceProvider.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- includes/Analytics/Reports/BaseQueryFilter.php
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (4)
includes/DependencyManagement/Providers/AnalyticsServiceProvider.php (1)
27-27: Add new serviceThis new entry ensures that the
SellerDataModifieris managed by the container. The approach is consistent, looks clean, and integrates well with your existing architecture.includes/Analytics/Reports/SellerDataModifier.php (3)
23-30: Constructor hooking is acceptableCalling
setup_entities()andregister_hooks()directly in the constructor ensures the hooks are registered upon instantiation. This is fine for now, but if performance or flexibility become issues, consider lazy hooking.
68-72: Hooks are registered correctlyAttaching each entity’s filter via the WordPress hook API is implemented properly.
88-97: Verify usage of Stock\QueryFilter for vendor ID retrievalConfirm that calling
Stock\QueryFilter::classto retrieve the seller ID is the intended approach. If a more dedicated or generic method to retrieve the seller ID exists, consider using it for clarity.✅ Verification successful
Stock\QueryFilter is the correct choice for vendor ID retrieval
The implementation is appropriate as Stock\QueryFilter extends BaseQueryFilter, inheriting proper permission checks, and is specifically designed for stock analytics context. The usage is consistent with other stock-related analytics components in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for other potential definitions of get_vendor_id across the codebase rg -A 5 'function get_vendor_id'Length of output: 1965
Script:
#!/bin/bash # Check Stock\QueryFilter's implementation and inheritance ast-grep --pattern 'class QueryFilter extends $_' includes/Analytics/Reports/Stock/QueryFilter.php cat includes/Analytics/Reports/Stock/QueryFilter.phpLength of output: 1637
MdAsifHossainNadim
added
Needs: Dev Review
MdAsifHossainNadim
changed the title
MdAsifHossainNadim
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
includes/Analytics/Reports/CacheKeyModifier.php (6)
7-11: Specify the “@SInCE” version.The docblock comments reference
DOKAN_SINCE, but the actual plugin version is not specified. Please replaceDOKAN_SINCEwith the correct version number to enhance clarity and maintain consistent documentation references.
27-30: Consider deferring hook registration.Registering hooks in the constructor may be too early for some scenarios, potentially leading to race conditions if other services aren’t yet loaded. Consider deferring the hook registration to a dedicated
initmethod triggered after the plugin is fully initialized.
73-75: Add error handling for unrecognized entities.If
$this->entitiescontains strings that don’t match known analytics entities, the filter registration could fail silently. Consider adding checks or logging to provide feedback when encountering unrecognized entities.
92-92: Avoid direct container calls in the method body.Directly calling
dokan()->get_container()->get(...)in this method couples the code to the container. For improved testability and maintainability, inject the needed dependencies into the constructor instead of resolving them at call time.
99-99: Clarify usage of theseller_idarray key.Adding
'seller_id'to$argsmight conflict with existing keys or reserved parameters in analytics queries. Consider using a more descriptive key (e.g.,'dokan_seller_id') to avoid potential collisions.
112-114: Enhance seller validation.Currently,
is_valid_seller_idchecks only for a numeric value greater than zero. If needed, extend validation logic to ensure the user exists and has the required seller capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/Analytics/Reports/CacheKeyModifier.php(1 hunks)includes/DependencyManagement/Providers/AnalyticsServiceProvider.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/DependencyManagement/Providers/AnalyticsServiceProvider.php
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (1)
includes/Analytics/Reports/CacheKeyModifier.php (1)
21-21: Ensure backward compatibility with typed properties.Typed properties (
protected array $entities;) require PHP 7.4 or higher. If the plugin aims to support older PHP versions, consider removing the type declaration or providing a polyfill. Otherwise, document the required PHP version.
mrabbani
added
👍 Dev Review Done
and removed
Needs: Dev Review
shashwatahalder01
added
QA In Progress
and removed
Needs: Testing
shashwatahalder01
added
QA approved
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Analytics Reports
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
Chores
BaseQueryFilterRefactor