Implement per-plan role mappings feature #2
Conversation
- Introduced functionality to assign specific WordPress roles to subscription plans, allowing for more granular control over member permissions. - Added UI elements in the admin settings for enabling per-plan roles and mapping them to subscription plans. - Updated role decision logic to consider per-plan mappings when determining user roles based on their subscriptions. - Ensured mutual exclusivity between broad access and specific plan access in post visibility settings. This update enhances the flexibility of role management within the Memberful plugin.
WalkthroughAdds per-plan WordPress role mappings across backend, UI, and role-decision logic; introduces storage, admin handling, URL helper, and new view. Updates advanced settings to enable/disable per-plan roles and map plans to roles, with bulk user-role updates. Adjusts post access control fallback and enforces mutual exclusivity between “any subscriber” and specific plans in UI and save logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant WP as WP Admin
participant Handler as memberful_wp_plan_role_mappings
participant Roles as roles.php (mappings)
participant Users as WP Users
Admin->>WP: Open Plan Role Mappings page
Admin->>WP: Submit mappings form
WP->>Handler: Validate nonce, read POST
Handler->>Roles: Set use_per_plan_roles(flag)
alt Per-plan enabled
Handler->>Roles: Validate & save plan→role mappings
Handler->>Roles: Update all user roles with mappings
Roles->>Users: Iterate users, decide role per mappings
else Per-plan disabled
Handler->>Roles: Clear plan→role mappings
Handler->>Roles: Update all user roles (generic logic)
end
Handler-->>WP: Status + redirect
WP-->>Admin: Success message
sequenceDiagram
autonumber
actor WP as WP Core
participant RD as Memberful_Wp_User_Role_Decision
participant Roles as roles.php (feature flag + mappings)
WP->>RD: role_for_user(user)
RD->>Roles: Check use_per_plan_roles()
alt Per-plan enabled AND user has active subscriptions
RD->>Roles: Get all plan→role mappings
RD->>RD: Collect user’s plan IDs
RD->>RD: Find first mapped role
alt Any mapped role found
RD-->>WP: mapped role
else None mapped
RD-->>WP: inactive role
end
else Per-plan disabled or no active subs
RD-->>WP: role via existing active/inactive logic
end
sequenceDiagram
autonumber
actor Visitor
participant WP as WP Frontend
participant ACL as memberful_can_user_access_post
Visitor->>WP: Request post
WP->>ACL: Check access
alt No plan/product restrictions
alt Post viewable by any subscriber AND user has a subscription
ACL-->>WP: grant
else Otherwise
ACL-->>WP: grant (no restrictions)
end
else Restrictions present
ACL-->>WP: evaluate registered/plan/product intersection
end
WP-->>Visitor: Serve or restrict
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on November 2
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| <select name="plan_role_mappings[<?php echo esc_attr( $plan_id ); ?>]"> | ||
| <option value=""><?php _e( 'No specific role (use default)', 'memberful' ); ?></option> | ||
| <?php foreach( $available_roles as $role => $role_name ): ?> | ||
| <option value="<?php echo esc_attr( $role ); ?>" <?php selected( $current_mappings[ $plan_id ], $role ); ?>> |
There was a problem hiding this comment.
Bug: Undefined Array Key Access in Role Mapping
Accessing $current_mappings[$plan_id] directly without checking for key existence can cause PHP notices. This occurs when a subscription plan lacks a role mapping, leading to an undefined array key access within the selected() function call.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wordpress/wp-content/plugins/memberful-wp/src/admin.php (1)
383-428: Add nonce verification before processing POST data.The POST block starting at Line 383 processes form submissions without nonce verification. This is a security vulnerability that could allow CSRF attacks.
Apply this diff to add nonce verification at the start of the POST block:
if ( ! empty( $_POST ) ) { + if ( ! memberful_wp_valid_nonce( 'memberful_options' ) ) { + return; + } + if ( isset( $_POST['role_mappings']['active_customer'] ) && array_key_exists( $_POST['role_mappings']['active_customer'], $allowed_roles ) ) {
🧹 Nitpick comments (3)
wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php (1)
74-78: Consider implementing explicit role priority logic.When a user has multiple plans with different role mappings, the code returns the first role found (line 78). This could lead to non-deterministic behavior if plan order changes or if WordPress internally reorders subscription data.
Consider implementing explicit priority logic (e.g., by role hierarchy or plan priority) to ensure consistent role assignment.
wordpress/wp-content/plugins/memberful-wp/src/admin.php (1)
703-726: Consider refactoring duplicated logic.The logic in Lines 703-726 is very similar to Lines 408-428 in
memberful_wp_advanced_settings(). Both blocks handle per-plan role mappings validation, saving, and user role updates. Consider extracting this common logic into a shared helper function to improve maintainability.Example refactor:
/** * Process and save per-plan role mappings from POST data * @param bool $use_per_plan_roles Whether per-plan roles are enabled * @param array $allowed_roles Allowed WordPress roles * @param array $subscription_plans Available subscription plans * @return bool Whether the save was successful */ function memberful_wp_save_plan_role_mappings( $use_per_plan_roles, $allowed_roles, $subscription_plans ) { memberful_wp_set_use_per_plan_roles( $use_per_plan_roles ); if ( $use_per_plan_roles && isset( $_POST['plan_role_mappings'] ) ) { $new_mappings = array(); foreach ( $_POST['plan_role_mappings'] as $plan_id => $role ) { $plan_id = intval( $plan_id ); $role = sanitize_text_field( $role ); if ( ! empty( $role ) && array_key_exists( $role, $allowed_roles ) && isset( $subscription_plans[ $plan_id ] ) ) { $new_mappings[ $plan_id ] = $role; } } update_option( 'memberful_plan_role_mappings', $new_mappings ); memberful_wp_update_all_user_roles_with_plan_mappings(); return true; } else { update_option( 'memberful_plan_role_mappings', array() ); return true; } }Then both functions could call this helper instead of duplicating the logic.
wordpress/wp-content/plugins/memberful-wp/src/roles.php (1)
58-72: Remove or deprecate unused plan role mapping functions
- These two functions aren’t referenced anywhere internally and rely on separate get_option()/update_option() calls (race‐condition prone).
- Either remove them (or mark as @deprecated for the next major version), or document their concurrency limitations and direct users to the bulk‐update workflow in admin.php.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
wordpress/wp-content/plugins/memberful-wp/src/acl.php(1 hunks)wordpress/wp-content/plugins/memberful-wp/src/admin.php(4 hunks)wordpress/wp-content/plugins/memberful-wp/src/metabox.php(1 hunks)wordpress/wp-content/plugins/memberful-wp/src/options.php(1 hunks)wordpress/wp-content/plugins/memberful-wp/src/roles.php(1 hunks)wordpress/wp-content/plugins/memberful-wp/src/urls.php(1 hunks)wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php(2 hunks)wordpress/wp-content/plugins/memberful-wp/views/acl_selection.php(2 hunks)wordpress/wp-content/plugins/memberful-wp/views/advanced_settings.php(2 hunks)wordpress/wp-content/plugins/memberful-wp/views/plan_role_mappings.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
wordpress/wp-content/plugins/memberful-wp/views/advanced_settings.php (1)
wordpress/wp-content/plugins/memberful-wp/src/core-ext.php (1)
memberful_wp_nonce_field(15-17)
wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php (1)
wordpress/wp-content/plugins/memberful-wp/src/roles.php (2)
memberful_wp_use_per_plan_roles(86-88)memberful_wp_get_all_plan_role_mappings(78-80)
wordpress/wp-content/plugins/memberful-wp/views/plan_role_mappings.php (2)
wordpress/wp-content/plugins/memberful-wp/src/core-ext.php (2)
memberful_wp_render(62-66)memberful_wp_nonce_field(15-17)wordpress/wp-content/plugins/memberful-wp/src/urls.php (1)
memberful_wp_plugin_plan_role_mappings_url(103-105)
wordpress/wp-content/plugins/memberful-wp/src/roles.php (2)
wordpress/wp-content/plugins/memberful-wp/src/user/map.php (1)
fetch_user_ids_of_all_mapped_members(260-266)wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php (2)
Memberful_Wp_User_Role_Decision(3-85)ensure_user_role_is_correct(4-8)
wordpress/wp-content/plugins/memberful-wp/src/admin.php (5)
wordpress/wp-content/plugins/memberful-wp/src/entities.php (1)
memberful_subscription_plans(32-34)wordpress/wp-content/plugins/memberful-wp/src/roles.php (6)
memberful_wp_get_all_plan_role_mappings(78-80)memberful_wp_use_per_plan_roles(86-88)memberful_wp_update_customer_roles(32-41)memberful_wp_set_use_per_plan_roles(94-96)memberful_wp_update_all_user_roles_with_plan_mappings(101-113)memberful_wp_roles_that_can_be_mapped_to(3-11)wordpress/wp-content/plugins/memberful-wp/vendor/reporting.php (2)
Memberful_Wp_Reporting(12-143)report(22-26)wordpress/wp-content/plugins/memberful-wp/src/core-ext.php (2)
memberful_wp_valid_nonce(11-13)memberful_wp_render(62-66)wordpress/wp-content/plugins/memberful-wp/src/urls.php (1)
memberful_wp_plugin_plan_role_mappings_url(103-105)
🪛 PHPMD (2.15.0)
wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php
68-68: Avoid unused local variables such as '$subscription_data'. (undefined)
(UnusedLocalVariable)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
wordpress/wp-content/plugins/memberful-wp/views/acl_selection.php (2)
26-26: LGTM!The addition of the CSS class
memberful-subscription-planenables JavaScript selection and mutual exclusivity logic.
51-87: LGTM!The mutual exclusivity logic between "any active subscriber" and specific plans is correctly implemented. The synchronization functions properly handle:
- Disabling specific plans when broad access is selected
- Unchecking broad access when specific plans are selected
- Initial state synchronization on page load
wordpress/wp-content/plugins/memberful-wp/src/urls.php (1)
103-105: LGTM!The new URL helper follows the established pattern of other subpage helpers and correctly delegates to
memberful_wp_plugin_settings_urlwith theplan_role_mappingssubpage parameter.wordpress/wp-content/plugins/memberful-wp/src/options.php (1)
19-20: LGTM!The new option keys for per-plan role mappings are correctly added with appropriate defaults (empty array and FALSE).
wordpress/wp-content/plugins/memberful-wp/src/acl.php (1)
329-336: LGTM!The fallback access logic correctly handles posts with no specific plan or product restrictions:
- Grants access to users with active subscriptions when
viewable_by_any_subscriberis true- Grants open access when no restrictions exist
The implementation aligns with the mutual exclusivity enforcement in the metabox save logic.
wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php (1)
49-52: LGTM!The feature flag check correctly gates per-plan role logic and only applies to active users.
wordpress/wp-content/plugins/memberful-wp/views/advanced_settings.php (3)
29-79: LGTM!The Per-Plan Roles UI section is well-structured with:
- Clear toggle for enabling the feature
- Conditional display of mappings when enabled
- Appropriate handling of empty subscription plans
- Consistent use of WordPress escaping and translation functions
84-96: LGTM!The JavaScript toggle logic correctly switches between the simple role mapping table and the per-plan mappings UI based on the checkbox state, providing a clear user experience.
68-68: Guard against undefined array index.The inline comparison
$current_mappings[$plan_id]may trigger an undefined index notice if the plan has no mapping. Useisset()or the null coalescing operator to provide a safe default.Apply this diff:
- <option value="<?php echo esc_attr( $role ); ?>" <?php echo ( isset($current_mappings[$plan_id]) && $current_mappings[$plan_id] === $role ) ? 'selected="selected"' : '' ?>> + <option value="<?php echo esc_attr( $role ); ?>" <?php selected( isset($current_mappings[$plan_id]) ? $current_mappings[$plan_id] : '', $role ); ?>>Likely an incorrect or invalid review comment.
wordpress/wp-content/plugins/memberful-wp/views/plan_role_mappings.php (1)
1-80: LGTM!The per-plan role mappings view is well-structured with:
- Clear page heading and description
- Enable toggle with appropriate description
- Conditional mappings table with proper escaping
- Client-side toggle functionality
The implementation aligns with WordPress coding standards and the plugin's existing patterns.
wordpress/wp-content/plugins/memberful-wp/src/admin.php (2)
379-381: LGTM!The variable initialization properly retrieves subscription plans, current mappings, and the per-plan feature flag using the appropriate helper functions.
445-447: LGTM!The vars array correctly includes the new variables needed for the per-plan roles UI in the advanced settings view.
wordpress/wp-content/plugins/memberful-wp/src/roles.php (3)
78-80: LGTM!The function correctly retrieves all plan role mappings with an appropriate default value.
86-96: LGTM!The feature flag accessor functions are correctly implemented with an appropriate default value.
101-113: LGTM!The batch updater correctly retrieves all mapped members and updates their roles. The early return optimisation for empty user lists is good. Whilst this could be slow with many users, it's acceptable for an admin operation that runs after configuration changes.
| wp_redirect( memberful_wp_plugin_plan_role_mappings_url() ); | ||
| return; |
There was a problem hiding this comment.
Add exit after redirect to prevent further execution.
Line 728 performs a redirect but doesn't exit, which means the script continues to execute and render the view. This can lead to unexpected behaviour and wasted resources.
Apply this diff:
wp_redirect( memberful_wp_plugin_plan_role_mappings_url() );
- return;
+ exit;
}🤖 Prompt for AI Agents
In wordpress/wp-content/plugins/memberful-wp/src/admin.php around lines 728 to
729, the code calls wp_redirect(...) and returns but does not stop execution;
after performing the redirect you must immediately terminate script execution to
prevent further rendering or logic from running. Modify the flow so that after
the wp_redirect(...) call you call exit(); (or die();) to stop execution
(keeping the existing return/remove as appropriate) so no additional output or
processing occurs after the redirect.
| // Enforce mutual exclusivity at save time: if specific plans are chosen, do not allow broad access | ||
| if ( !empty($subscription_plan_ids) ) { | ||
| $viewable_by_anybody_subscribed_to_a_plan = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify mutual exclusivity is also enforced for term metadata.
The enforcement of mutual exclusivity between specific plans and broad access is correct for posts. However, the memberful_wp_save_term_metadata function (lines 161-189) does not appear to implement the same logic. Ensure consistent behavior across posts and terms.
Run the following script to confirm that term metadata save lacks this enforcement:
🏁 Script executed:
#!/bin/bash
# Description: Check if term metadata save enforces mutual exclusivity
rg -n -A 5 -B 2 'memberful_wp_save_term_metadata' wordpress/wp-content/plugins/memberful-wp/src/metabox.php | grep -A 5 'viewable_by_anybody_subscribed_to_a_plan'Length of output: 0
🏁 Script executed:
#!/bin/bash
rg -n 'function memberful_wp_save_term_metadata' -A 20 -B 2 wordpress/wp-content/plugins/memberful-wp/src/metabox.phpLength of output: 1610
Enforce mutual exclusivity in term metadata save
In memberful_wp_save_term_metadata (metabox.php lines 179–181), insert the same check as in post saves so that if $subscription_plan_ids is non-empty you force $viewable_by_anybody_subscribed_to_a_plan = false before calling memberful_wp_set_term_available_to_anybody_subscribed_to_a_plan. This ensures specific-plan ACLs override broad-access.
🤖 Prompt for AI Agents
In wordpress/wp-content/plugins/memberful-wp/src/metabox.php around lines
119-122, the term metadata save path needs the same mutual-exclusivity guard as
post saves: before calling
memberful_wp_set_term_available_to_anybody_subscribed_to_a_plan inside
memberful_wp_save_term_metadata, check if $subscription_plan_ids is non-empty
and, if so, set $viewable_by_anybody_subscribed_to_a_plan = false; then proceed
to call memberful_wp_set_term_available_to_anybody_subscribed_to_a_plan so
specific-plan ACLs override broad-access.
| private function role_for_user_with_plan_mappings( $current_subscriptions ) { | ||
| $plan_mappings = memberful_wp_get_all_plan_role_mappings(); | ||
|
|
||
| // Find the highest priority role based on user's subscriptions | ||
| $assigned_roles = array(); | ||
|
|
||
| foreach ( $current_subscriptions as $plan_id => $subscription_data ) { | ||
| if ( isset( $plan_mappings[ $plan_id ] ) ) { | ||
| $assigned_roles[] = $plan_mappings[ $plan_id ]; | ||
| } | ||
| } | ||
|
|
||
| // If user has multiple plans with different roles, we need to determine priority | ||
| if ( ! empty( $assigned_roles ) ) { | ||
| // For now, we'll use the first role found | ||
| // In the future, this could be enhanced with role priority logic | ||
| return $assigned_roles[0]; | ||
| } | ||
|
|
||
| // Fallback when per-plan roles are enabled but no mapping found | ||
| // Use the inactive role to avoid granting broad access unintentionally | ||
| return $this->inactive_role; | ||
| } |
There was a problem hiding this comment.
Reconsider the fallback role for unmapped plans.
When per-plan roles are enabled but no mapping is found for a user's active subscriptions, the code falls back to $this->inactive_role (line 83). This seems incorrect because the user has active subscriptions and should not receive the inactive role.
Consider using $this->active_role instead, or maintaining the user's current role to avoid unintentionally downgrading active members.
Apply this diff to use the active role as fallback:
- // Fallback when per-plan roles are enabled but no mapping found
- // Use the inactive role to avoid granting broad access unintentionally
- return $this->inactive_role;
+ // Fallback when per-plan roles are enabled but no mapping found
+ // Use the active role since user has active subscriptions
+ return $this->active_role;🧰 Tools
🪛 PHPMD (2.15.0)
68-68: Avoid unused local variables such as '$subscription_data'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php around
lines 62 to 84, the fallback for users with active subscriptions but no per-plan
mapping currently returns $this->inactive_role which wrongly downgrades active
members; change the fallback to return $this->active_role (or preserve the
user's current role) so active subscribers aren't assigned the inactive role,
and ensure any tests or callers expecting inactive_role are updated accordingly.
| // Find the highest priority role based on user's subscriptions | ||
| $assigned_roles = array(); | ||
|
|
||
| foreach ( $current_subscriptions as $plan_id => $subscription_data ) { |
There was a problem hiding this comment.
Remove unused variable.
The variable $subscription_data is declared but never used in the loop. Remove it to clean up the code.
Apply this diff:
- foreach ( $current_subscriptions as $plan_id => $subscription_data ) {
+ foreach ( $current_subscriptions as $plan_id => $_subscription_data ) {
if ( isset( $plan_mappings[ $plan_id ] ) ) {
$assigned_roles[] = $plan_mappings[ $plan_id ];
}📝 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.
| foreach ( $current_subscriptions as $plan_id => $subscription_data ) { | |
| foreach ( $current_subscriptions as $plan_id => $_subscription_data ) { | |
| if ( isset( $plan_mappings[ $plan_id ] ) ) { | |
| $assigned_roles[] = $plan_mappings[ $plan_id ]; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
68-68: Avoid unused local variables such as '$subscription_data'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In wordpress/wp-content/plugins/memberful-wp/src/user/role_decision.php around
line 68, the foreach declares $subscription_data but never uses it; replace the
loop to iterate keys only (e.g. use foreach (array_keys($current_subscriptions)
as $plan_id) ) so the unused variable is removed and intent is clear.
| <select name="plan_role_mappings[<?php echo esc_attr( $plan_id ); ?>]"> | ||
| <option value=""><?php _e( 'No specific role (use default)', 'memberful' ); ?></option> | ||
| <?php foreach( $available_roles as $role => $role_name ): ?> | ||
| <option value="<?php echo esc_attr( $role ); ?>" <?php selected( $current_mappings[ $plan_id ], $role ); ?>> |
There was a problem hiding this comment.
Guard against undefined array index.
The selected() function uses $current_mappings[$plan_id] directly, which may trigger an undefined index notice if the plan has no mapping. Provide a safe default value.
Apply this diff:
- <option value="<?php echo esc_attr( $role ); ?>" <?php selected( $current_mappings[ $plan_id ], $role ); ?>>
+ <option value="<?php echo esc_attr( $role ); ?>" <?php selected( isset($current_mappings[$plan_id]) ? $current_mappings[$plan_id] : '', $role ); ?>>📝 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.
| <option value="<?php echo esc_attr( $role ); ?>" <?php selected( $current_mappings[ $plan_id ], $role ); ?>> | |
| <option value="<?php echo esc_attr( $role ); ?>" <?php selected( isset($current_mappings[$plan_id]) ? $current_mappings[$plan_id] : '', $role ); ?>> |
🤖 Prompt for AI Agents
In wordpress/wp-content/plugins/memberful-wp/views/plan_role_mappings.php around
line 49 the code passes $current_mappings[$plan_id] directly to selected(),
which can raise an undefined index notice when no mapping exists; guard by using
a safe default (e.g. use isset() or the null coalescing operator to get
$current_mappings[$plan_id] ?? '' or similar) and pass that safe value into
selected() so the function always receives a defined value.
1 participant
This update enhances the flexibility of role management within the Memberful plugin.
Summary by CodeRabbit