pro: route every Pro check through one case-insensitive getter - #8937
Conversation
Pro status was re-derived in seven places. Six compared userLevel to the literal 'pro' or 'expired' case-sensitively; the seventh, the post-purchase check in app_purchase.dart, lowercased first and also accepted subscriptionData.status == 'active'. account.dart had a raw comparison and a call to the isPro getter on adjacent lines. So a change in casing upstream would have shown a paying user the free tier and the data-cap widget while the purchase flow considered them Pro. userLevel comes from a Postgres enum today, so this is latent rather than live. UserDataProX.isPro is now the single derivation, folding case to match what the purchase flow always did, with isExpired alongside it. The six duplicates call it. common.dart imports and exports the extension so the canonical check is the default rather than something each caller re-implements. _userHasActivePurchase stays deliberately wider and is now documented as such: a subscription can read as active before the account level flips, and wrongly telling someone their payment failed is worse than being early. Narrowing it would create false failures; widening isPro to match would grant Pro UI to users the server considers free. It is expressed via isPro so the one intentional divergence is visible instead of accidental. platinum is a valid server-side level that pro_users treats as paid but the client still excludes. Left as-is since we do not issue it, with a test pinning the current behaviour so it fails loudly the day we do. Found while investigating Freshdesk #180658 and does NOT fix it. That user paid via Shepherd, so subscriptionData.status is empty and the server sent lowercase 'pro' -- neither divergence could fire for them. Their stale userLevel=expired is still unexplained. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVgb2MDpZ4wpH6fywKC2hE
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughEntitlement handling now exposes normalized Pro and expired flags, and consumers use them instead of comparing raw user-level strings. Expiration formatting, account flows, providers, purchase messaging, exports, and tests were updated. ChangesEntitlement status normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This pull request centralizes Pro/Expired entitlement checks by routing all client-side comparisons through a single, case-insensitive extension on UserDataModel, eliminating several duplicated (and previously inconsistent) userLevel comparisons across the app.
Changes:
- Updated
UserDataModelPro check to be case-insensitive and introduced a pairedisExpiredgetter. - Replaced duplicated
userLevel == 'pro'/'expired'comparisons in UI/providers/logic withisPro/isExpired. - Added focused unit tests covering casing, non-Pro levels, and the intentional exclusion of
platinum.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/core/extensions/user_data_test.dart | Adds unit coverage for isPro/isExpired behavior, including casing and platinum pin. |
| lib/features/account/account.dart | Replaces inline userLevel comparisons with canonical isPro / isExpired. |
| lib/core/utils/pro_utils.dart | Relies on exported extension (via common.dart) instead of importing user_data.dart directly. |
| lib/core/services/app_purchase.dart | Expresses the intentional “active subscription” divergence via isPro plus subscription status, and documents it. |
| lib/core/extensions/user_data.dart | Implements the canonical, case-insensitive isPro and adds isExpired based on a normalized _level. |
| lib/core/extensions/ref.dart | Updates Riverpod providers to use isPro / isExpired and imports the extension. |
| lib/core/extensions/plan.dart | Uses isExpired instead of comparing userLevel directly (extension available via common.dart). |
| lib/core/common/common.dart | Imports/exports user_data.dart so entitlement checks are consistently available via common.dart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/core/extensions/plan.dart (1)
53-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for normalized expiration dates.
toDate()now changes behavior for mixed-case levels such asExpired, but the supplied tests only exerciseisExpired; they do not verify thelastExpiredOnformatting path. Add a test with a mixed-case expired level and a validlastExpiredOn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/core/extensions/plan.dart` around lines 53 - 63, The expiration-date formatting path in the plan model lacks regression coverage for mixed-case expired levels. Add a test using an expired level such as “Expired” with a valid lastExpiredOn value, and assert that the normalized expiration date is formatted and returned with the expired translation. Keep the existing isExpired coverage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/core/extensions/plan.dart`:
- Around line 53-63: The expiration-date formatting path in the plan model lacks
regression coverage for mixed-case expired levels. Add a test using an expired
level such as “Expired” with a valid lastExpiredOn value, and assert that the
normalized expiration date is formatted and returned with the expired
translation. Keep the existing isExpired coverage intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a798227-9edf-4687-9b47-2b692d65f5df
📒 Files selected for processing (8)
lib/core/common/common.dartlib/core/extensions/plan.dartlib/core/extensions/ref.dartlib/core/extensions/user_data.dartlib/core/services/app_purchase.dartlib/core/utils/pro_utils.dartlib/features/account/account.darttest/core/extensions/user_data_test.dart
💤 Files with no reviewable changes (1)
- lib/core/utils/pro_utils.dart
Review feedback: routing plan.dart's expiry check through isExpired changed toDate() for mixed-case levels -- 'Expired' used to fall through and report the expiration date as though the plan were still active -- but the tests only exercised the isExpired getter, not the formatting path. lastExpiredOn and expiration are given distinct dates so the assertion proves which branch ran, and the expected string is derived the same way _formatDate derives it rather than hard-coded, since toDate() converts UTC to local and a fixed string would only hold in one timezone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVgb2MDpZ4wpH6fywKC2hE
|
@coderabbitai the nitpick was valid — fixed in 550ff17. You were right that Added
Two details worth noting for review:
Verified the test discriminates: reverting line 53 to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Commit summary and changed files =='
git show --stat --oneline --decorate --no-renames 550ff17a
echo
echo '== Commit patch =='
git show --format=fuller --no-ext-diff --unified=50 --no-renames 550ff17a -- \
lib/core/extensions/plan.dart \
test/core/extensions/plan_test.dart
echo
echo '== Source outline =='
ast-grep outline lib/core/extensions/plan.dart --items all --view expanded
echo
echo '== Test outline =='
ast-grep outline test/core/extensions/plan_test.dart --items all --view expanded
echo
echo '== Current relevant source and test sections =='
nl -ba lib/core/extensions/plan.dart | sed -n '1,130p'
nl -ba test/core/extensions/plan_test.dart | sed -n '1,260p'Length of output: 798
I couldn’t independently verify commit 🐇🔍 ✏️ Learnings added
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Problem
Pro status was re-derived in seven places, and they did not agree.
extensions/user_data.dartisProuserLevel == 'pro'extensions/ref.dartisUserProProviderextensions/ref.dartisUserExpiredProvider== 'expired'common/common.dart:158features/account/account.dart:213== 'expired'features/account/account.dart:356extensions/plan.dart:53== 'expired'services/app_purchase.dart:744proORsubscriptionData.status == 'active'The post-purchase check lowercased before comparing and accepted an active subscription. Everything else did neither.
account.darthad a raw comparison and a call to theisProgetter on adjacent lines:Consequence if casing ever changed upstream: the UI shows a paying user the free tier and the data-cap widget (
home.dart:172gatesDataUsage()on!isUserPro) while the purchase flow considers them Pro.userLevelcomes from a Postgres enum today, so this is latent, not live.Change
UserDataProX.isProis now the single derivation, withisExpiredalongside it:The six duplicates now call it.
common.dartimports and exports the extension, so the canonical check is the default rather than something each caller re-implements.The one intentional divergence is kept, and labelled
_userHasActivePurchasestays wider thanisPro. It gates post-purchase messaging — narrowing it would start telling paying users their payment failed; wideningisProto match would grant Pro UI to users the server considers free. So it is expressed viaisProand documented, making the divergence visible instead of accidental:platinum left alone, deliberately
platinumis a valid server-side level thatpro_userstreats as paid (users/delete.go:54:level == pro || level == platinum) but the client still excludes. We do not issue it, so widening entitlement here would be speculative. There is a test pinning the current behaviour so it fails loudly the day we do.This does not fix #180658
Found while investigating Freshdesk #180658 (China, Windows 9.1.17, paid via Shepherd, app kept showing
userLevel: expired), but neither divergence could have fired for that user:pro_server.purchasesandsubscriptionData.statusis empty — the subscription fallback is inertprofrom a Postgres enum — the casing difference is inertTheir stale
userLevel=expiredremains unexplained. Also ruled out along the way: thetype '_Map<String, dynamic>' is not a subtype of type 'String'error atflutter.log:1116, one millisecond before that payload, is theuser_failurescast fixed in #8929. It is onavailableServersProvider, whichhomeProviderdoes not depend on, so it cannot affectuserLevel. Same-millisecond co-occurrence, not causation.Testing
flutter test), including 10 new acrosstest/core/extensions/user_data_test.dart(7) andtest/core/extensions/plan_test.dart(3)_levelto case-sensitive fails exactly the two casing tests; revertingplan.dart:53touserLevel == 'expired'fails 2 of the 3toDate()tests. Both discriminate.toDate()'s expired branch is covered per review feedback — a mixed-caseExpiredpreviously fell through and reported the expiration date as though the plan were active. Expected dates are derived the way_formatDatederives them, not hard-coded, sincetoDate()converts UTC to local.flutter analyzeon every touched file: no new issues. The 4 remaining warnings are pre-existing — verified by analyzing the same files on stashed HEAD. Two imports this change made redundant were removed.userLevel == '…'comparisons remain outside the extension🤖 Generated with Claude Code
https://claude.ai/code/session_01RVgb2MDpZ4wpH6fywKC2hE
Summary by CodeRabbit
toDate()expired-branch behavior, including formatting andlastExpiredOn == 0handling.