MR-9: Add membership expiry warning banner for members#7
Open
zhickson wants to merge 13 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a dismissible expiry banner feature: plugin bootstrap inclusion, admin settings to enable/disable and set days (1–90, default 7), default options, rendering and eligibility logic, a view with dismissal persisted in sessionStorage, and helper functions to determine soonest expiring subscription. Changes
Sequence DiagramsequenceDiagram
participant Browser as User Browser
participant WP as WordPress
participant Hook as wp_body_open / wp_footer
participant Banner as Expiry Banner Module
participant Meta as User Subscriptions
participant View as Expiry Banner View
participant Session as sessionStorage
Browser->>WP: Request frontend page
WP->>Hook: Trigger wp_body_open / wp_footer
Hook->>Banner: memberful_wp_render_expiry_banner()
Banner->>Banner: Run eligibility checks (admin, connection, logged-in, enabled)
alt Not eligible
Banner-->>Hook: No output
else Eligible
Banner->>Meta: Fetch user's subscriptions
Meta-->>Banner: Subscription data
Banner->>Banner: Determine soonest expiring subscription within threshold
alt No expiry found
Banner-->>Hook: No output
else Expiry found
Banner->>View: Render banner HTML
View-->>Browser: Inject banner HTML
Browser->>Session: Check dismissal flag
alt Dismissed
Browser->>View: Hide banner
else Show banner
Browser->>Browser: Display banner
Browser->>Session: Set dismissal flag on dismiss
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wordpress/wp-content/plugins/memberful-wp/src/options.php (1)
27-28: Minor style inconsistency:falsevsFALSE.The existing options in this array use uppercase
FALSE/TRUE(e.g., lines 20, 26), while these new entries use lowercasefalse. Functionally identical in PHP, but worth aligning for consistency.🔧 Suggested diff
- 'memberful_expiry_banner_enabled' => false, + 'memberful_expiry_banner_enabled' => FALSE,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/src/options.php` around lines 27 - 28, The new option entries 'memberful_expiry_banner_enabled' and 'memberful_expiry_banner_days' use lowercase boolean literal false; update 'memberful_expiry_banner_enabled' to use the uppercase PHP boolean literal FALSE to match the surrounding options style (e.g., other entries using TRUE/FALSE) so the array remains stylistically consistent.wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php (1)
19-21:asyncanddeferattributes are no-ops on inline scripts.These attributes only have effect on
<script>elements with asrcattribute. On inline scripts they are silently ignored by browsers. Removing them avoids confusion.🔧 Suggested diff
-<script async defer> +<script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php` around lines 19 - 21, The inline script in expiry-banner.php is using async and defer on a script tag that has no src (they're no-ops); remove the async and defer attributes from the <script> element wrapping the IIFE that references memberful-expiry-banner and the sessionStorage key "memberful_expiry_banner_dismissed" so the tag is a plain inline <script> and behavior remains identical (keep the IIFE, event listener on .memberful-expiry-banner__dismiss, and sessionStorage logic unchanged).wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php (1)
10-17: Skipping admins — consider documenting the rationale or using a filter.Users with
manage_optionsare unconditionally excluded (line 15). If an admin also has a Memberful membership (e.g., for testing), they will never see the banner. This is likely intentional, but it's not overridable. Consider making this filterable for edge cases, or at least adding a brief inline comment explaining why admins are excluded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php` around lines 10 - 17, The code unconditionally skips users with current_user_can('manage_options') inside memberful_wp_render_expiry_banner which prevents admins from ever seeing the expiry banner; update the logic to be overridable by introducing a filter (e.g. call apply_filters('memberful_wp_show_expiry_to_admins', false) or similar) and use its result to decide whether to return, or if you prefer minimal change add a clear inline comment above the current_user_can check explaining why admins are excluded and that it is intentional; make sure the filter name and the current_user_can('manage_options') check are referenced so callers can opt into showing the banner to admins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php`:
- Around line 129-155: The current loop over $subscriptions incorrectly picks
the earliest expires_at inside the threshold even if that subscription is
long-ago expired; change the selection to prefer a non-expired subscription:
inside the foreach tracking $expires_at (from
memberful_wp_parse_expiry_timestamp) compute $is_expired = ($expires_at - $now)
< 0 and then maintain two candidates—$best_active (non-expired, choose the one
with smallest expires_at) and $best_expired (expired, choose the one with
largest expires_at = most recently expired). Update $best_active when a
non-expired subscription has an earlier expires_at than the current
$best_active; update $best_expired when an expired subscription has a later
expires_at than the current $best_expired. After the loop set $soonest to
$best_active if present, otherwise to $best_expired, preserving existing
calculation of days_remaining and is_expired using $now and DAY_IN_SECONDS.
In `@wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php`:
- Around line 16-18: The fixed .memberful-expiry-banner overlaps page content
because it uses position:fixed without offsetting the document flow; update the
display logic that renders .memberful-expiry-banner to also add/remove a
matching top offset to the page (e.g., set document.body.style.paddingTop =
banner.offsetHeight + 'px' when the banner is shown and clear it on dismiss) or
insert a spacer element of height equal to banner.offsetHeight immediately
before the main content; ensure this adjustment is applied after layout (or on
window resize) and is removed when the .memberful-expiry-banner is dismissed so
focus targets and skip-links are not hidden.
---
Nitpick comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php`:
- Around line 10-17: The code unconditionally skips users with
current_user_can('manage_options') inside memberful_wp_render_expiry_banner
which prevents admins from ever seeing the expiry banner; update the logic to be
overridable by introducing a filter (e.g. call
apply_filters('memberful_wp_show_expiry_to_admins', false) or similar) and use
its result to decide whether to return, or if you prefer minimal change add a
clear inline comment above the current_user_can check explaining why admins are
excluded and that it is intentional; make sure the filter name and the
current_user_can('manage_options') check are referenced so callers can opt into
showing the banner to admins.
In `@wordpress/wp-content/plugins/memberful-wp/src/options.php`:
- Around line 27-28: The new option entries 'memberful_expiry_banner_enabled'
and 'memberful_expiry_banner_days' use lowercase boolean literal false; update
'memberful_expiry_banner_enabled' to use the uppercase PHP boolean literal FALSE
to match the surrounding options style (e.g., other entries using TRUE/FALSE) so
the array remains stylistically consistent.
In `@wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php`:
- Around line 19-21: The inline script in expiry-banner.php is using async and
defer on a script tag that has no src (they're no-ops); remove the async and
defer attributes from the <script> element wrapping the IIFE that references
memberful-expiry-banner and the sessionStorage key
"memberful_expiry_banner_dismissed" so the tag is a plain inline <script> and
behavior remains identical (keep the IIFE, event listener on
.memberful-expiry-banner__dismiss, and sessionStorage logic unchanged).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php (1)
147-173:⚠️ Potential issue | 🟠 MajorSubscription selection still picks the earliest
expires_at, preferring old-expired subscriptions over active ones.The threshold guard at line 158 skips subscriptions with
$expires_at > $threshold_timestamp(a future value), but past-expired subscriptions — even ones from years ago — pass through and compete in the selection at line 162. The$expires_at < $soonest['expires_at']comparison will then choose the oldest expired subscription over an active one expiring in, say, 6 days.The fix is to maintain two candidates —
$best_active(non-expired, prefer soonest-to-expire) and$best_expired(expired, prefer most recently expired) — and after the loop assign$soonestto$best_activeif present, otherwise$best_expired.🐛 Proposed fix for the subscription selection logic
$now = time(); $threshold_timestamp = $now + ( $days_threshold * DAY_IN_SECONDS ); - $soonest = null; + $best_active = null; + $best_expired = null; foreach ( $subscriptions as $subscription ) { if ( empty( $subscription['expires_at'] ) ) { continue; } $expires_at = memberful_wp_parse_expiry_timestamp( $subscription['expires_at'] ); if ( empty( $expires_at ) ) { continue; } if ( $expires_at > $threshold_timestamp ) { continue; } - if ( null === $soonest || $expires_at < $soonest['expires_at'] ) { - $seconds_remaining = $expires_at - $now; - $is_expired = $seconds_remaining < 0; - $days_remaining = $is_expired ? 0 : (int) ceil( $seconds_remaining / DAY_IN_SECONDS ); - - $soonest = array( - 'expires_at' => $expires_at, - 'days_remaining' => $days_remaining, - 'is_expired' => $is_expired, - ); - } + $seconds_remaining = $expires_at - $now; + $is_expired = $seconds_remaining < 0; + + if ( ! $is_expired ) { + // Prefer the non-expired subscription that expires soonest. + if ( null === $best_active || $expires_at < $best_active['expires_at'] ) { + $best_active = array( + 'expires_at' => $expires_at, + 'days_remaining' => (int) ceil( $seconds_remaining / DAY_IN_SECONDS ), + 'is_expired' => false, + ); + } + } else { + // Fall back to the most recently expired subscription. + if ( null === $best_expired || $expires_at > $best_expired['expires_at'] ) { + $best_expired = array( + 'expires_at' => $expires_at, + 'days_remaining' => 0, + 'is_expired' => true, + ); + } + } } - return $soonest; + return null !== $best_active ? $best_active : $best_expired;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php` around lines 147 - 173, The current loop over $subscriptions (using memberful_wp_parse_expiry_timestamp) picks the absolute earliest expires_at (so very old expired subs beat active ones); change the selection to track two candidates: $best_active and $best_expired. For each subscription (after the existing empty/threshold checks and computing $seconds_remaining/$is_expired/$days_remaining), if !$is_expired consider it for $best_active and choose the one with the smallest expires_at (soonest-to-expire); if $is_expired consider it for $best_expired and choose the one with the largest expires_at (most recently expired). After the loop set $soonest = $best_active ?: $best_expired and keep the same shape (expires_at, days_remaining, is_expired).
🧹 Nitpick comments (1)
wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php (1)
20-21:resizeandorientationchangelisteners are registered even when the banner has already been dismissed.When
r()(the dismissed-check) returnstrue,n.style.displayis set to"none"and the bump style is zeroed wheni()fires. However, the event listeners are still attached and will keep callingi()on every resize/orientation change for the lifetime of the page, even though the banner is never shown again. Giveni()readsn.style.displayand will always find"none", the computed margin is always 0 and the calls are no-ops — but they're wasteful.♻️ Suggested refinement
-if(r())n.style.display="none";else{var a=n.querySelector(".memberful-expiry-banner__dismiss");a&&a.addEventListener("click",(function(){s(),n.style.display="none",i()}))}window.addEventListener("resize",i),window.addEventListener("orientationchange",i),i() +if(r()){n.style.display="none";}else{var a=n.querySelector(".memberful-expiry-banner__dismiss");a&&a.addEventListener("click",(function(){s();n.style.display="none";i();}));window.addEventListener("resize",i);window.addEventListener("orientationchange",i);}i();By moving the event listener registration inside the
elsebranch, they are only attached when the banner is actually visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php` around lines 20 - 21, The resize and orientationchange listeners are always registered even when the banner is dismissed; update the code so the window.addEventListener calls for "resize" and "orientationchange" that reference the i() handler are moved into the else branch (the branch where r() returns false and the banner is potentially visible), and ensure the dismiss handler (a click on a) still calls i() after setting n.style.display="none"; optionally remove those listeners when s() runs by calling removeEventListener with the same i reference to avoid leftover handlers—target the n, i(), r(), s(), and a symbols when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php`:
- Line 19: The inline <script> block in expiry-banner.php currently includes
async and defer attributes which have no effect for inline code; remove the
async and defer attributes from the script tag that defines the expiry banner
logic (the inline <script> in expiry-banner.php) so the tag is just a plain
<script> block, ensuring no misleading attributes remain.
---
Duplicate comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php`:
- Around line 147-173: The current loop over $subscriptions (using
memberful_wp_parse_expiry_timestamp) picks the absolute earliest expires_at (so
very old expired subs beat active ones); change the selection to track two
candidates: $best_active and $best_expired. For each subscription (after the
existing empty/threshold checks and computing
$seconds_remaining/$is_expired/$days_remaining), if !$is_expired consider it for
$best_active and choose the one with the smallest expires_at
(soonest-to-expire); if $is_expired consider it for $best_expired and choose the
one with the largest expires_at (most recently expired). After the loop set
$soonest = $best_active ?: $best_expired and keep the same shape (expires_at,
days_remaining, is_expired).
---
Nitpick comments:
In `@wordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php`:
- Around line 20-21: The resize and orientationchange listeners are always
registered even when the banner is dismissed; update the code so the
window.addEventListener calls for "resize" and "orientationchange" that
reference the i() handler are moved into the else branch (the branch where r()
returns false and the banner is potentially visible), and ensure the dismiss
handler (a click on a) still calls i() after setting n.style.display="none";
optionally remove those listeners when s() runs by calling removeEventListener
with the same i reference to avoid leftover handlers—target the n, i(), r(),
s(), and a symbols when making this change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.phpwordpress/wp-content/plugins/memberful-wp/views/expiry-banner.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php (1)
149-180:⚠️ Potential issue | 🟠 MajorSubscription selection logic still prioritises old-expired subscriptions over active ones.
As noted in previous review, this logic selects the subscription with the earliest
expires_atamong all subscriptions within the threshold. This means an old expired subscription (e.g., from 2 years ago) would be selected over an active subscription expiring soon.The fix should:
- Prioritise non-expired subscriptions within the threshold (pick the soonest to expire)
- Only fall back to the most recently expired subscription if no active-within-threshold subscription exists
🐛 Proposed fix to prioritise active subscriptions
$now = time(); $threshold_timestamp = $now + ( $days_threshold * DAY_IN_SECONDS ); - $soonest = null; + $best_active = null; + $best_expired = null; $expiring_subscriptions_count = 0; $active_subscriptions_count = 0; foreach ( $subscriptions as $subscription ) { if ( empty( $subscription['expires_at'] ) ) { ++$active_subscriptions_count; continue; } $expires_at = memberful_wp_parse_expiry_timestamp( $subscription['expires_at'] ); if ( empty( $expires_at ) ) { ++$active_subscriptions_count; continue; } if ( $expires_at > $threshold_timestamp ) { ++$active_subscriptions_count; continue; } ++$expiring_subscriptions_count; + $is_expired = ( $expires_at - $now ) < 0; - if ( null === $soonest || $expires_at < $soonest['expires_at'] ) { - $seconds_remaining = $expires_at - $now; - $is_expired = $seconds_remaining < 0; - $days_remaining = $is_expired ? 0 : (int) ceil( $seconds_remaining / DAY_IN_SECONDS ); - - $soonest = array( - 'expires_at' => $expires_at, - 'days_remaining' => $days_remaining, - 'is_expired' => $is_expired, - ); + if ( $is_expired ) { + // Track most recently expired (largest expires_at among expired) + if ( null === $best_expired || $expires_at > $best_expired['expires_at'] ) { + $best_expired = array( 'expires_at' => $expires_at ); + } + } else { + // Track soonest to expire (smallest expires_at among non-expired) + if ( null === $best_active || $expires_at < $best_active['expires_at'] ) { + $best_active = array( 'expires_at' => $expires_at ); + } } } + // Prefer active subscription, fall back to most recently expired + $soonest_candidate = $best_active ?? $best_expired; + + if ( null === $soonest_candidate ) { + return null; + } + + $expires_at = $soonest_candidate['expires_at']; + $seconds_remaining = $expires_at - $now; + $is_expired = $seconds_remaining < 0; + $days_remaining = $is_expired ? 0 : (int) ceil( $seconds_remaining / DAY_IN_SECONDS ); + + $soonest = array( + 'expires_at' => $expires_at, + 'days_remaining' => $days_remaining, + 'is_expired' => $is_expired, + ); + if ( null === $soonest ) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php` around lines 149 - 180, The current selection in the foreach over $subscriptions uses only expires_at to pick $soonest and therefore can choose a long-ago expired subscription over a soon-to-expire active one; update the comparison logic after computing $is_expired to prefer non-expired items: when $soonest is null set it normally; otherwise if the current item is not expired and $soonest['is_expired'] is true, replace $soonest; if both have the same is_expired value then if not expired pick the smaller expires_at (so the soonest-to-expire active subscription) but if both expired pick the larger expires_at (the most recently expired) before assigning to $soonest (use the existing $expires_at, $soonest['expires_at'], and $soonest['is_expired'] symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php`:
- Around line 31-33: The early-return condition uses is_admin(), which detects
admin pages not admin users, so replace or augment that check to exclude
administrator users by testing capabilities instead (e.g., use
current_user_can('manage_options') or user_can( get_current_user_id(),
'manage_options') ) alongside memberful_wp_is_connected_to_site() and
is_user_logged_in(); update the conditional in expiry_banner.php (the block
containing is_admin(), memberful_wp_is_connected_to_site(), is_user_logged_in())
so that administrator-capability-holders are returned early and thus do not see
the banner.
---
Duplicate comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php`:
- Around line 149-180: The current selection in the foreach over $subscriptions
uses only expires_at to pick $soonest and therefore can choose a long-ago
expired subscription over a soon-to-expire active one; update the comparison
logic after computing $is_expired to prefer non-expired items: when $soonest is
null set it normally; otherwise if the current item is not expired and
$soonest['is_expired'] is true, replace $soonest; if both have the same
is_expired value then if not expired pick the smaller expires_at (so the
soonest-to-expire active subscription) but if both expired pick the larger
expires_at (the most recently expired) before assigning to $soonest (use the
existing $expires_at, $soonest['expires_at'], and $soonest['is_expired']
symbols).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
Run ID: 25050aa2-6722-4e80-bd78-9fcdc3202a63
📒 Files selected for processing (1)
wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php
Comment on lines
+31
to
+33
| if ( is_admin() || ! memberful_wp_is_connected_to_site() || ! is_user_logged_in() ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
is_admin() checks page context, not user role—admin users will still see the banner on front-end pages.
Per the PR objectives, administrator users should be excluded from seeing the banner. However, is_admin() only returns true when viewing admin pages (e.g., /wp-admin/), not when an administrator user is viewing front-end pages.
To exclude administrator users, add a capability check:
🐛 Proposed fix to exclude admin users
- if ( is_admin() || ! memberful_wp_is_connected_to_site() || ! is_user_logged_in() ) {
+ if ( is_admin() || ! memberful_wp_is_connected_to_site() || ! is_user_logged_in() || current_user_can( 'manage_options' ) ) {
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php` around lines
31 - 33, The early-return condition uses is_admin(), which detects admin pages
not admin users, so replace or augment that check to exclude administrator users
by testing capabilities instead (e.g., use current_user_can('manage_options') or
user_can( get_current_user_id(), 'manage_options') ) alongside
memberful_wp_is_connected_to_site() and is_user_logged_in(); update the
conditional in expiry_banner.php (the block containing is_admin(),
memberful_wp_is_connected_to_site(), is_user_logged_in()) so that
administrator-capability-holders are returned early and thus do not see the
banner.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php (2)
31-33:⚠️ Potential issue | 🟠 MajorUse capability-based admin exclusion on Line 31.
is_admin()checks request context, not whether the current user is an admin, so admin users on front-end pages can still see the banner.🐛 Proposed fix
- if ( is_admin() || ! memberful_wp_is_connected_to_site() || ! is_user_logged_in() ) { + if ( is_admin() || ! memberful_wp_is_connected_to_site() || ! is_user_logged_in() || current_user_can( 'manage_options' ) ) { return; }#!/bin/bash set -euo pipefail FILE="wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php" echo "Inspecting eligibility guard around Line 31..." nl -ba "$FILE" | sed -n '24,38p' echo echo "Verifying admin capability exclusion is present..." rg -n "is_admin\(|is_user_logged_in\(|current_user_can\(" "$FILE" -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php` around lines 31 - 33, The early-return guard uses is_admin() (request-context) which lets admin users on the front-end still see the banner; update the condition to check the current user's capability instead. Replace the is_admin() check with a capability check using current_user_can('manage_options') (or another appropriate capability for your admin role) while keeping the other checks memberful_wp_is_connected_to_site() and is_user_logged_in() intact so the line becomes a single short-circuit return that returns when the user has the admin capability or the site isn’t connected or the user isn’t logged in.
145-180:⚠️ Potential issue | 🟠 MajorPrefer active-soonest expiry; only fall back to most-recent expired.
Current selection can choose a very old expired subscription and incorrectly show an expired-state banner even when another active subscription expires soon.
🐛 Proposed fix
- $soonest = null; + $best_active = null; + $best_expired = null; $expiring_subscriptions_count = 0; $active_subscriptions_count = 0; @@ - ++$expiring_subscriptions_count; - - if ( null === $soonest || $expires_at < $soonest['expires_at'] ) { - $seconds_remaining = $expires_at - $now; - $is_expired = $seconds_remaining < 0; - $days_remaining = $is_expired ? 0 : (int) ceil( $seconds_remaining / DAY_IN_SECONDS ); - - $soonest = array( - 'expires_at' => $expires_at, - 'days_remaining' => $days_remaining, - 'is_expired' => $is_expired, - ); - } + ++$expiring_subscriptions_count; + + $seconds_remaining = $expires_at - $now; + $is_expired = $seconds_remaining < 0; + $days_remaining = $is_expired ? 0 : (int) ceil( $seconds_remaining / DAY_IN_SECONDS ); + $candidate = array( + 'expires_at' => $expires_at, + 'days_remaining' => $days_remaining, + 'is_expired' => $is_expired, + ); + + if ( $is_expired ) { + if ( null === $best_expired || $expires_at > $best_expired['expires_at'] ) { + $best_expired = $candidate; // most recently expired + } + continue; + } + + if ( null === $best_active || $expires_at < $best_active['expires_at'] ) { + $best_active = $candidate; // soonest active expiry + } } + $soonest = null !== $best_active ? $best_active : $best_expired; + if ( null === $soonest ) { return null; }Also applies to: 182-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php` around lines 145 - 180, The current loop that builds $soonest can pick an old expired subscription; change the selection logic in the foreach over $subscriptions (the block that updates $expiring_subscriptions_count and $soonest) so you first compute $is_expired for each candidate (using $expires_at and $now) and prefer a non-expired candidate with the earliest expires_at (smallest timestamp) over any expired one; only if there are no non-expired candidates fall back to the most-recent expired (largest expires_at). Update the comparison that sets $soonest to consider three cases: initialize when $soonest is null, replace if candidate is non-expired while $soonest is expired or has a later expires_at, or if both are expired replace only when candidate.expires_at is greater (more recent); keep increments of $expiring_subscriptions_count and $active_subscriptions_count unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php`:
- Around line 31-33: The early-return guard uses is_admin() (request-context)
which lets admin users on the front-end still see the banner; update the
condition to check the current user's capability instead. Replace the is_admin()
check with a capability check using current_user_can('manage_options') (or
another appropriate capability for your admin role) while keeping the other
checks memberful_wp_is_connected_to_site() and is_user_logged_in() intact so the
line becomes a single short-circuit return that returns when the user has the
admin capability or the site isn’t connected or the user isn’t logged in.
- Around line 145-180: The current loop that builds $soonest can pick an old
expired subscription; change the selection logic in the foreach over
$subscriptions (the block that updates $expiring_subscriptions_count and
$soonest) so you first compute $is_expired for each candidate (using $expires_at
and $now) and prefer a non-expired candidate with the earliest expires_at
(smallest timestamp) over any expired one; only if there are no non-expired
candidates fall back to the most-recent expired (largest expires_at). Update the
comparison that sets $soonest to consider three cases: initialize when $soonest
is null, replace if candidate is non-expired while $soonest is expired or has a
later expires_at, or if both are expired replace only when candidate.expires_at
is greater (more recent); keep increments of $expiring_subscriptions_count and
$active_subscriptions_count unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2b2c1b7-67f2-4e35-9ab4-41a9eda7451a
📒 Files selected for processing (1)
wordpress/wp-content/plugins/memberful-wp/src/expiry_banner.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a new membership expiry banner that helps members avoid interruptions by warning them before access lapses.
When enabled, logged-in members will see a clear banner if their membership has expired, expires today, or is due to expire soon. The warning window is configurable in plugin settings, so site owners can choose how early to remind members.
What's included
1-90days, default7).role/aria-live) and extension hooks for custom implementations.Scope and behaviour
Test plan
7) and save.Summary by CodeRabbit