Skip to content

fix: change override condition of tier #1547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ravverma wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: change override condition of tier #1547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/spacecat-shared-tier-client/src/tier-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,9 @@ class TierClient {
if (existing.entitlement) {
const currentTier = existing.entitlement.getTier();

// If currentTier doesn't match with given tier and is not PAID, update it
if (currentTier !== tier && currentTier !== ENTITLEMENT_TIERS.PAID) {
// Only upgrade the tier when the customer is becoming PAID; otherwise keep as-is.
// Tiers: PRE_ONBOARD, PLG, FREE_TRIAL, PAID.
if (tier === ENTITLEMENT_TIERS.PAID && currentTier !== ENTITLEMENT_TIERS.PAID) {
Copy link
Copy Markdown
Member

@solaris007 solaris007 Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: silent no-op with misleading return.

When this guard is false, the method still returns existing.entitlement from below, whose getTier() does NOT match the tier argument the caller passed. Callers in spacecat-api-service (PLG/LLMO onboarding) and spacecat-fulfillment-worker have no way to detect their requested tier was ignored: no log, no error, no flag on the return object.

Fix (pick one):

  • Add this.log.warn({ orgId, currentTier, requestedTier: tier }, 'Tier change skipped; createEntitlement only upgrades to PAID') in an else branch, OR
  • Return { entitlement, siteEnrollment, tierChanged: boolean } so callers can branch on it.

Important (separate finding, same line): the hard-coded === ENTITLEMENT_TIERS.PAID couples the library to today's 4-tier ladder. When ENTERPRISE or any variant is added, this predicate is wrong at every call site. Prefer a tier-ordinal comparison or an explicit transition-policy map.

existing.entitlement.setTier(tier);
await existing.entitlement.save();
}
Expand Down
12 changes: 6 additions & 6 deletions packages/spacecat-shared-tier-client/test/tier-client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ describe('TierClient', () => {
});
});

it('should upgrade PLG entitlement to FREE_TRIAL (PLG is not PAID, so tier updates)', async () => {
it('should NOT change tier when upgrading PLG to FREE_TRIAL (only PAID upgrades are applied)', async () => {
Copy link
Copy Markdown
Member

@solaris007 solaris007 Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: the createEntitlement describe block has zero tests with currentTier = PAID.

The entire invariant this PR enforces ("protect PAID from being overwritten") is unverified. Please add, each asserting setTier/save are NOT called and the returned entitlement's getTier() still returns PAID:

  • PAID -> PAID
  • PAID -> PLG
  • PAID -> FREE_TRIAL
  • PAID -> PRE_ONBOARD

Also Important: consider replacing these two renamed tests with a parameterised forEach over the full 16-cell transition table (4 tiers x 4 targets). Future tier additions (e.g. ENTERPRISE) become a one-row change instead of N new it blocks.

Minor: the stale JSDoc at tier-client.js:131-137 still says "If entitlement exists with different tier, updates the tier" and needs updating to reflect the PAID-only rule.

const mockPlgEntitlement = {
...mockEntitlement,
getTier: () => 'PLG',
Expand All @@ -500,8 +500,8 @@ describe('TierClient', () => {

const result = await tierClient.createEntitlement('FREE_TRIAL');

expect(mockPlgEntitlement.setTier).to.have.been.calledWith('FREE_TRIAL');
expect(mockPlgEntitlement.save).to.have.been.called;
expect(mockPlgEntitlement.setTier).to.not.have.been.called;
expect(mockPlgEntitlement.save).to.not.have.been.called;
expect(result).to.deep.equal({
entitlement: mockPlgEntitlement,
siteEnrollment: mockSiteEnrollment,
Expand Down Expand Up @@ -550,7 +550,7 @@ describe('TierClient', () => {
});
});

it('should upgrade PRE_ONBOARD entitlement to PLG (PRE_ONBOARD is not PAID, so tier updates)', async () => {
it('should NOT change tier when upgrading PRE_ONBOARD to PLG (only PAID upgrades are applied)', async () => {
const mockPreOnboardEntitlement = {
...mockEntitlement,
getTier: () => 'PRE_ONBOARD',
Expand All @@ -564,8 +564,8 @@ describe('TierClient', () => {

const result = await tierClient.createEntitlement('PLG');

expect(mockPreOnboardEntitlement.setTier).to.have.been.calledWith('PLG');
expect(mockPreOnboardEntitlement.save).to.have.been.called;
expect(mockPreOnboardEntitlement.setTier).to.not.have.been.called;
expect(mockPreOnboardEntitlement.save).to.not.have.been.called;
expect(result).to.deep.equal({
entitlement: mockPreOnboardEntitlement,
siteEnrollment: mockSiteEnrollment,
Expand Down
Loading