More faster user query count #201
Conversation
|
Warning Rate limit exceeded@superdav42 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 26 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. 📒 Files selected for processing (1)
WalkthroughReworks customer user role limiting to avoid per-role get_users() counts by using count_users() avail_roles data and treating 0 as unlimited. Maintains existing enable/disable checks and preserves roles in frontend/admin-logged-out contexts. Adds PHPUnit tests covering frontend visitors, admin without login, and over-limit behavior on a test site. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor WP as WordPress Core
participant L as Customer_User_Role_Limits
participant S as Site Limitations
participant U as Users API
WP->>L: filter_editable_roles(roles)
alt No limitations module enabled OR super admin OR frontend/no user context
L-->>WP: return roles (unchanged)
else Limits apply
L->>S: get per-role limits
L->>U: count_users()
U-->>L: { avail_roles: {role: count, ...} }
loop For each role
alt Limit enabled and number > 0 and count >= number
L->>L: unset role
else Limit disabled or unlimited or under limit
L->>L: keep role
end
end
L-->>WP: return filtered roles
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning, 2 inconclusive)❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inc/limits/class-customer-user-role-limits.php (1)
96-112: Harden against non-object $limit to avoid TypeError in property_exists().$users_limitation->{$role} may be absent or null; property_exists() requires object|string.
Use an object check:
if (is_object($limit) && property_exists($limit, 'enabled') && $limit->enabled) { // ... } else { unset($roles[$role]); }
🧹 Nitpick comments (4)
inc/limits/class-customer-user-role-limits.php (1)
103-105: Memoize count_users() per request to cut redundant work.Static caching avoids repeated queries if the filter runs multiple times in one request.
- if ( ! isset($user_count)) { - $user_count = count_users(); - } + static $user_count = null; + if (null === $user_count) { + $user_count = count_users(); + }tests/WP_Ultimo/Limits/Customer_User_Role_Limits_Test.php (3)
29-41: Don’t create a site in every test; move site creation to a class-level setup and target the correct blog when clearing meta.Current setUp() leaks sites across tests and clears meta on the current blog, not the test site.
@@ - // Create a test site - self::$test_site = wu_create_site( - [ - 'title' => 'Test Site', - 'domain' => 'test-site5.example.com', - 'template_id' => 1, - 'type' => Site_Type::CUSTOMER_OWNED, - ] - ); - // Remove any pre-existing site limitations - $blog_id = get_current_blog_id(); - delete_metadata('blog', $blog_id, 'wu_limitations'); + // Remove any pre-existing site limitations on current and test blog + delete_metadata('blog', get_current_blog_id(), 'wu_limitations'); + if (self::$test_site) { + delete_metadata('blog', self::$test_site->get_id(), 'wu_limitations'); + }Add a class-level setup once (place after the property declaration):
+ public static function wpSetUpBeforeClass($factory): void { + self::$test_site = wu_create_site( + [ + 'title' => 'Test Site', + 'domain' => 'test-site5.example.com', + 'template_id' => 1, + 'type' => Site_Type::CUSTOMER_OWNED, + ] + ); + }
47-53: Use wpTearDownAfterClass() so cleanup reliably runs.tear_down_after_class() may not be invoked depending on the PHPUnit/WP versions in CI.
- public static function tear_down_after_class() { - parent::tear_down_after_class(); + public static function wpTearDownAfterClass(): void { + parent::wpTearDownAfterClass();
55-73: Add coverage for “users module enabled” on frontend/anonymous to lock in the fast-path behavior.The current tests pass because the module is disabled; they won’t catch regressions where a frontend visitor still triggers count logic.
Add tests like:
public function test_frontend_visitor_returns_original_when_users_module_enabled(): void { $instance = Customer_User_Role_Limits::get_instance(); wp_set_current_user(0); if (function_exists('set_current_screen')) { set_current_screen('front'); } $roles = ['subscriber' => ['name' => 'Subscriber']]; // Enable users module with some limit add_metadata('blog', get_current_blog_id(), 'wu_limitations', [ 'users' => [ 'enabled' => true, 'limit' => ['subscriber' => ['enabled' => true, 'number' => 1]], ], ], true); $this->assertSame($roles, $instance->filter_editable_roles($roles)); } public function test_admin_not_logged_in_returns_original_when_users_module_enabled(): void { $instance = Customer_User_Role_Limits::get_instance(); wp_set_current_user(0); if (function_exists('set_current_screen')) { set_current_screen('dashboard'); } add_metadata('blog', get_current_blog_id(), 'wu_limitations', [ 'users' => [ 'enabled' => true, 'limit' => ['subscriber' => ['enabled' => true, 'number' => 1]], ], ], true); $this->assertSame(['subscriber' => ['name' => 'Subscriber']], $instance->filter_editable_roles(['subscriber' => ['name' => 'Subscriber']])); }Also applies to: 75-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
inc/limits/class-customer-user-role-limits.php(1 hunks)tests/WP_Ultimo/Limits/Customer_User_Role_Limits_Test.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/WP_Ultimo/Limits/Customer_User_Role_Limits_Test.php (3)
inc/objects/class-limitations.php (1)
Limitations(24-489)inc/functions/site.php (1)
wu_create_site(135-160)inc/limits/class-customer-user-role-limits.php (2)
Customer_User_Role_Limits(21-150)filter_editable_roles(85-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: cypress (8.4, chrome)
- GitHub Check: cypress (8.2, firefox)
- GitHub Check: cypress (7.4, firefox)
- GitHub Check: cypress (8.0, chrome)
- GitHub Check: PHP 8.3
- GitHub Check: PHP 8.1
- GitHub Check: PHP 8.2
- GitHub Check: PHP 8.4
fixes #120
Summary by CodeRabbit
Bug Fixes
Performance
Tests