Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit 78622cb

Browse files
authored
chore/enterpriseportal: only use 'revoke' verb for licenses (#63407)
There's a confusing notion of "archived license" that really means "archived subscription", which is problematic because "can an archived subscription, have valid licenses, in a world where revoked licenses exist?" IMO archiving a subscription should immediately and permanently revoke all its associated licenses, per discussion in https://github.com/sourcegraph/sourcegraph/pull/63330#discussion_r1645333457. This means we can remove all notion of "archived license" - when looking at licenses, they're only revoked, or not revoked. ⚠️ These RPCs are not used anywhere yet so this is a safe breaking change. ## Test plan CI
1 parent 31da9c2 commit 78622cb

File tree

5 files changed

+365
-370
lines changed

5 files changed

+365
-370
lines changed

cmd/enterprise-portal/internal/dotcomdb/dotcomdb.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -426,20 +426,28 @@ func (r *Reader) ListEnterpriseSubscriptionLicenses(
426426
limit: pageSize,
427427
}
428428
var args []any
429+
var hasRevokedFilter bool
429430
for _, filter := range filters {
430431
switch filter.GetFilter().(type) {
431432
case *subscriptionsv1.ListEnterpriseSubscriptionLicensesFilter_SubscriptionId:
432433
conds.addWhere(fmt.Sprintf("licenses.product_subscription_id = $%d", len(args)+1))
433434
args = append(args,
434435
strings.TrimPrefix(filter.GetSubscriptionId(), subscriptionsv1.EnterpriseSubscriptionIDPrefix))
435-
case *subscriptionsv1.ListEnterpriseSubscriptionLicensesFilter_IsArchived:
436-
if filter.GetIsArchived() {
437-
conds.addWhere("subscriptions.archived_at IS NOT NULL")
436+
case *subscriptionsv1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked:
437+
hasRevokedFilter = true
438+
// We treat subscription archived and revoked license the same.
439+
if filter.GetIsRevoked() {
440+
conds.addWhere("(subscriptions.archived_at IS NOT NULL OR licenses.revoked_at IS NOT NULL)")
438441
} else {
439442
conds.addWhere("subscriptions.archived_at IS NULL")
443+
conds.addWhere("licenses.revoked_at IS NULL")
440444
}
441445
}
442446
}
447+
if !hasRevokedFilter {
448+
conds.addWhere("subscriptions.archived_at IS NULL")
449+
conds.addWhere("licenses.revoked_at IS NULL")
450+
}
443451

444452
query := newLicensesQuery(conds, r.opts)
445453
rows, err := r.db.Query(ctx, query, args...)

cmd/enterprise-portal/internal/dotcomdb/dotcomdb_test.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
381381
name: "no filters",
382382
filters: nil,
383383
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {
384-
assert.Len(t, licenses, mock.createdLicenses) // return all results
384+
// Only unarchived subscriptions
385+
assert.Len(t, licenses, mock.createdLicenses-mock.archivedSubscriptions)
385386
},
386387
}, {
387388
name: "filter by subscription ID",
@@ -415,8 +416,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
415416
SubscriptionId: mock.targetSubscriptionID,
416417
},
417418
}, {
418-
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsArchived{
419-
IsArchived: false,
419+
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked{
420+
IsRevoked: false,
420421
},
421422
}},
422423
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {
@@ -430,8 +431,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
430431
}, {
431432
name: "filter by is archived",
432433
filters: []*v1.ListEnterpriseSubscriptionLicensesFilter{{
433-
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsArchived{
434-
IsArchived: true,
434+
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked{
435+
IsRevoked: true,
435436
},
436437
}},
437438
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {
@@ -440,8 +441,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
440441
}, {
441442
name: "filter by not archived",
442443
filters: []*v1.ListEnterpriseSubscriptionLicensesFilter{{
443-
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsArchived{
444-
IsArchived: false,
444+
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked{
445+
IsRevoked: false,
445446
},
446447
}},
447448
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {

cmd/enterprise-portal/internal/subscriptionsservice/v1.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -238,11 +238,6 @@ func (s *handlerV1) ListEnterpriseSubscriptionLicenses(ctx context.Context, req
238238

239239
// Validate filters
240240
filters := req.Msg.GetFilters()
241-
if len(filters) == 0 {
242-
// TODO: We may want to allow filter-less usage in the future
243-
return nil, connect.NewError(connect.CodeInvalidArgument,
244-
errors.New("at least one filter is required"))
245-
}
246241
for _, filter := range filters {
247242
// TODO: Implement additional filtering as needed
248243
switch f := filter.GetFilter().(type) {
@@ -259,8 +254,6 @@ func (s *handlerV1) ListEnterpriseSubscriptionLicenses(ctx context.Context, req
259254
errors.New(`invalid filter: "subscription_id"" provided but is empty`),
260255
)
261256
}
262-
case *subscriptionsv1.ListEnterpriseSubscriptionLicensesFilter_IsArchived:
263-
// Nothing to validate
264257
}
265258
}
266259

0 commit comments

Comments
 (0)