PM-24535 Web premium upgrade path for archive #16854
Conversation
| premiumOptions?: { | ||
| /** When true, the premium badge will show on the filter for non-premium users. */ | ||
| showPremiumBadge?: true; | ||
| /** Action to be called instead of applying the filter. */ | ||
| blockFilterAction?: () => Promise<void>; | ||
| }; |
There was a problem hiding this comment.
I tried to make this somewhat reusable if it were to ever apply to other filters, but I'm not supper happy with the configuration options here. I'm open to ideas!
There was a problem hiding this comment.
The only piece I'm hung up on is the name showPremiumBadge implies that the badge will always show, even though its only shown for non-premium. Maybe showBadgeForNonPremium though that is now pretty verbose.
There was a problem hiding this comment.
Verbose but does eliminate any confusion which is important.
Updated! 84d3c01
|
Great job! No new security vulnerabilities introduced in this pull request |
nick-livefront
force-pushed
the
vault/pm-24535/web-premium-archive
branch
from
1c1f901 to
759126f
Compare
|
Claude finished @nick-livefront's task —— View job Code Review CompleteI've reviewed this PR implementing the premium upgrade path for the archive feature. Here are my findings: SummaryThis PR successfully implements premium upgrade paths for the archive feature, showing appropriate badges and prompts for non-premium users. The implementation generally follows established patterns and addresses previous review feedback well. FindingsFinding 1: PR description quality is excellent - clear objectives, comprehensive screenshots/videos, and proper Jira tracking. Finding 2: Observable pattern implementation follows ADR-0003 correctly with Finding 3: The refactoring mentioned in the last commit (a55201e) using Finding 4: Good use of Angular signals for Finding 5: Premium badge implementation is properly abstracted through Finding 6: Tailwind CSS classes properly use
💭 Finding 8: The 💭 Finding 9: In vault-cipher-row.component.ts:193-197, the logic for hiding attachments when archived AND non-premium is good, but the comment could be more explicit about the business rule: "Users who lose premium should still access attachments on their archived items." ♻️ Finding 10: The 🎨 Finding 11: Consider extracting the premium check logic that's repeated in multiple places. For example, vault-cipher-row.component.ts has several guards like 💭 Finding 12: The service implementation in default-cipher-archive.service.ts:60-78 exposes multiple related observables ( Positive Observations👍 Great job addressing previous review feedback, especially:
👍 The video demonstrations in the PR description clearly show both user flows (no premium vs lost premium with archived items). 👍 Comprehensive i18n string additions with clear, user-friendly messaging (messages.json additions). Recommendations
ConclusionThis is solid work that implements the premium upgrade path thoughtfully. The main concern is test coverage, which should be addressed before merging. The code quality is good, follows established patterns, and the previous review feedback has been well incorporated. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16854 +/- ##
==========================================
- Coverage 41.20% 41.20% -0.01%
==========================================
Files 3543 3543
Lines 101912 101941 +29
Branches 15282 15287 +5
==========================================
+ Hits 41991 42002 +11
- Misses 58155 58174 +19
+ Partials 1766 1765 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrmccannon
left a comment
jrmccannon
left a comment
There was a problem hiding this comment.
Looks good. Just a few small suggestions and a question.
apps/web/src/app/vault/components/vault-items/vault-cipher-row.component.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/vault/components/vault-items/vault-items.component.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/vault/individual-vault/vault-filter/components/vault-filter.component.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/vault/individual-vault/vault-filter/components/vault-filter.component.ts
Outdated
Show resolved
Hide resolved
nick-livefront
force-pushed
the
vault/pm-24535/web-premium-archive
branch
from
27635f1 to
abb1e9d
Compare
|
@jrmccannon @shane-melton I had to rebase as the base branch was squashed in, ready for review now! |
shane-melton
left a comment
shane-melton
left a comment
There was a problem hiding this comment.
Looks good to me! Just one non-blocking comment regarding the name of the premiumOptions.
| premiumOptions?: { | ||
| /** When true, the premium badge will show on the filter for non-premium users. */ | ||
| showPremiumBadge?: true; | ||
| /** Action to be called instead of applying the filter. */ | ||
| blockFilterAction?: () => Promise<void>; | ||
| }; |
There was a problem hiding this comment.
The only piece I'm hung up on is the name showPremiumBadge implies that the badge will always show, even though its only shown for non-premium. Maybe showBadgeForNonPremium though that is now pretty verbose.
nick-livefront
dismissed stale reviews from shane-melton and jrmccannon
via
84d3c01
|
|
Archive has been put on a temporary hold - I'm moving this to a draft for the time being. |
nick-livefront
dismissed stale reviews from shane-melton and jrmccannon
via
385f5fa
|
@jrmccannon @shane-melton This is back ready for review now that Archive has a path forward. No functionality changes since your last review, only coming up to date with I actually refactored a change I don't know why I changed regarding the feature flag observable - a55201e |
…35/web-premium-archive
…the underlying logic
jrmccannon
left a comment
jrmccannon
left a comment
There was a problem hiding this comment.
Looks good from AC perspective
🎟️ Tracking
PM-24535
📔 Objective
Add steps for the user to upgrade to if they do not have access to premium:
📸 Screenshots
no-premium.mov
lost-premium.mov
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes