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

Commit

Permalink
chore/enterpriseportal: only use 'revoke' verb for licenses (#63407)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bobheadxi authored Jun 20, 2024
1 parent 31da9c2 commit 78622cb
Show file tree
Hide file tree
Showing 5 changed files with 365 additions and 370 deletions.
14 changes: 11 additions & 3 deletions cmd/enterprise-portal/internal/dotcomdb/dotcomdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,20 +426,28 @@ func (r *Reader) ListEnterpriseSubscriptionLicenses(
limit: pageSize,
}
var args []any
var hasRevokedFilter bool
for _, filter := range filters {
switch filter.GetFilter().(type) {
case *subscriptionsv1.ListEnterpriseSubscriptionLicensesFilter_SubscriptionId:
conds.addWhere(fmt.Sprintf("licenses.product_subscription_id = $%d", len(args)+1))
args = append(args,
strings.TrimPrefix(filter.GetSubscriptionId(), subscriptionsv1.EnterpriseSubscriptionIDPrefix))
case *subscriptionsv1.ListEnterpriseSubscriptionLicensesFilter_IsArchived:
if filter.GetIsArchived() {
conds.addWhere("subscriptions.archived_at IS NOT NULL")
case *subscriptionsv1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked:
hasRevokedFilter = true
// We treat subscription archived and revoked license the same.
if filter.GetIsRevoked() {
conds.addWhere("(subscriptions.archived_at IS NOT NULL OR licenses.revoked_at IS NOT NULL)")
} else {
conds.addWhere("subscriptions.archived_at IS NULL")
conds.addWhere("licenses.revoked_at IS NULL")
}
}
}
if !hasRevokedFilter {
conds.addWhere("subscriptions.archived_at IS NULL")
conds.addWhere("licenses.revoked_at IS NULL")
}

query := newLicensesQuery(conds, r.opts)
rows, err := r.db.Query(ctx, query, args...)
Expand Down
15 changes: 8 additions & 7 deletions cmd/enterprise-portal/internal/dotcomdb/dotcomdb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
name: "no filters",
filters: nil,
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {
assert.Len(t, licenses, mock.createdLicenses) // return all results
// Only unarchived subscriptions
assert.Len(t, licenses, mock.createdLicenses-mock.archivedSubscriptions)
},
}, {
name: "filter by subscription ID",
Expand Down Expand Up @@ -415,8 +416,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
SubscriptionId: mock.targetSubscriptionID,
},
}, {
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsArchived{
IsArchived: false,
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked{
IsRevoked: false,
},
}},
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {
Expand All @@ -430,8 +431,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
}, {
name: "filter by is archived",
filters: []*v1.ListEnterpriseSubscriptionLicensesFilter{{
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsArchived{
IsArchived: true,
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked{
IsRevoked: true,
},
}},
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {
Expand All @@ -440,8 +441,8 @@ func TestListEnterpriseSubscriptionLicenses(t *testing.T) {
}, {
name: "filter by not archived",
filters: []*v1.ListEnterpriseSubscriptionLicensesFilter{{
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsArchived{
IsArchived: false,
Filter: &v1.ListEnterpriseSubscriptionLicensesFilter_IsRevoked{
IsRevoked: false,
},
}},
expect: func(t *testing.T, licenses []*dotcomdb.LicenseAttributes) {
Expand Down
7 changes: 0 additions & 7 deletions cmd/enterprise-portal/internal/subscriptionsservice/v1.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,11 +238,6 @@ func (s *handlerV1) ListEnterpriseSubscriptionLicenses(ctx context.Context, req

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

Expand Down
Loading

0 comments on commit 78622cb

Please sign in to comment.