From 3f545652d57f091d7931d65be4a73ef450b5a677 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 27 Jul 2026 13:37:49 +0000 Subject: [PATCH] Use business campaign rules instead of status errors Introduce accessreview.Campaign with draft/deletable predicates and operation-specific client errors. Drop errUnlessDraftCampaign and status-to-sentinel switches in the service layer. Signed-off-by: Cursor Agent Co-authored-by: Bryan FRIMIN --- pkg/accessreview/campaign.go | 59 ++++++++++++++ pkg/accessreview/campaign_service.go | 79 ++++++------------- pkg/accessreview/errors.go | 65 ++++++++++----- pkg/accessreview/errors_test.go | 32 +++++--- .../v1/access_review_campaign_resolvers.go | 33 ++++---- 5 files changed, 163 insertions(+), 105 deletions(-) create mode 100644 pkg/accessreview/campaign.go diff --git a/pkg/accessreview/campaign.go b/pkg/accessreview/campaign.go new file mode 100644 index 000000000..5d1793469 --- /dev/null +++ b/pkg/accessreview/campaign.go @@ -0,0 +1,59 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package accessreview + +import ( + "go.probo.inc/probo/pkg/coredata" + "go.probo.inc/probo/pkg/gid" +) + +// Campaign is the business view of a loaded access review campaign record. +type Campaign struct { + ID gid.GID + status coredata.AccessReviewCampaignStatus +} + +func NewCampaignFromCoredata(record *coredata.AccessReviewCampaign) Campaign { + return Campaign{ + ID: record.ID, + status: record.Status, + } +} + +func (c Campaign) IsDraft() bool { + return c.status == coredata.AccessReviewCampaignStatusDraft +} + +func (c Campaign) IsDeletable() bool { + return c.IsDraft() || c.status == coredata.AccessReviewCampaignStatusCancelled +} + +func (c Campaign) IsPendingActions() bool { + return c.status == coredata.AccessReviewCampaignStatusPendingActions +} + +func (c Campaign) IsCompleted() bool { + return c.status == coredata.AccessReviewCampaignStatusCompleted +} + +func (c Campaign) IsCancelled() bool { + return c.status == coredata.AccessReviewCampaignStatusCancelled +} diff --git a/pkg/accessreview/campaign_service.go b/pkg/accessreview/campaign_service.go index c7721e5ac..b7c8ac192 100644 --- a/pkg/accessreview/campaign_service.go +++ b/pkg/accessreview/campaign_service.go @@ -158,8 +158,9 @@ func (s *Service) UpdateCampaign( return fmt.Errorf("cannot load campaign: %w", err) } - if err := errUnlessDraftCampaign(campaign); err != nil { - return err + biz := NewCampaignFromCoredata(campaign) + if !biz.IsDraft() { + return NewCampaignNotDraftError(biz.ID) } if req.Name != nil && *req.Name != nil { @@ -209,18 +210,9 @@ func (s *Service) DeleteCampaign( return fmt.Errorf("cannot load campaign: %w", err) } - if campaign.Status != coredata.AccessReviewCampaignStatusDraft && - campaign.Status != coredata.AccessReviewCampaignStatusCancelled { - switch campaign.Status { - case coredata.AccessReviewCampaignStatusInProgress: - return NewCampaignInProgressError(campaign.ID) - case coredata.AccessReviewCampaignStatusPendingActions: - return NewCampaignPendingActionsError(campaign.ID) - case coredata.AccessReviewCampaignStatusCompleted: - return NewCampaignCompletedError(campaign.ID) - default: - return NewCampaignInProgressError(campaign.ID) - } + biz := NewCampaignFromCoredata(campaign) + if !biz.IsDeletable() { + return NewCampaignNotDeletableError(biz.ID) } if err := campaign.Delete(ctx, conn, scope); err != nil { @@ -250,8 +242,9 @@ func (s *Service) AddCampaignSource( return fmt.Errorf("cannot load campaign: %w", err) } - if err := errUnlessDraftCampaign(campaign); err != nil { - return err + biz := NewCampaignFromCoredata(campaign) + if !biz.IsDraft() { + return NewCampaignNotDraftError(biz.ID) } if err := source.LoadByID(ctx, conn, scope, req.AccessReviewSourceID); err != nil { if errors.Is(err, coredata.ErrResourceNotFound) { @@ -297,8 +290,9 @@ func (s *Service) RemoveCampaignSource( return fmt.Errorf("cannot load campaign: %w", err) } - if err := errUnlessDraftCampaign(campaign); err != nil { - return err + biz := NewCampaignFromCoredata(campaign) + if !biz.IsDraft() { + return NewCampaignNotDraftError(biz.ID) } campaignSource := &coredata.AccessReviewCampaignSource{} @@ -406,8 +400,9 @@ func (s *Service) StartCampaign( return fmt.Errorf("cannot load campaign: %w", err) } - if err := errUnlessDraftCampaign(campaign); err != nil { - return err + biz := NewCampaignFromCoredata(campaign) + if !biz.IsDraft() { + return NewCampaignNotDraftError(biz.ID) } var campaignSources coredata.AccessReviewCampaignSources @@ -460,19 +455,9 @@ func (s *Service) CloseCampaign( return fmt.Errorf("cannot load campaign: %w", err) } - if campaign.Status != coredata.AccessReviewCampaignStatusPendingActions { - switch campaign.Status { - case coredata.AccessReviewCampaignStatusInProgress: - return NewCampaignInProgressError(campaign.ID) - case coredata.AccessReviewCampaignStatusCompleted: - return NewCampaignCompletedError(campaign.ID) - case coredata.AccessReviewCampaignStatusCancelled: - return NewCampaignCancelledError(campaign.ID) - case coredata.AccessReviewCampaignStatusDraft: - return NewCampaignInProgressError(campaign.ID) - default: - return NewCampaignInProgressError(campaign.ID) - } + biz := NewCampaignFromCoredata(campaign) + if !biz.IsPendingActions() { + return NewCampaignNotPendingActionsError(biz.ID) } entries := coredata.AccessReviewEntries{} @@ -596,12 +581,13 @@ func (s *Service) CancelCampaign( return fmt.Errorf("cannot load campaign: %w", err) } - if campaign.Status == coredata.AccessReviewCampaignStatusCompleted { - return NewCampaignCompletedError(campaign.ID) + biz := NewCampaignFromCoredata(campaign) + if biz.IsCompleted() { + return NewCampaignCompletedError(biz.ID) } - if campaign.Status == coredata.AccessReviewCampaignStatusCancelled { - return NewCampaignCancelledError(campaign.ID) + if biz.IsCancelled() { + return NewCampaignCancelledError(biz.ID) } now := time.Now() @@ -752,22 +738,3 @@ func (s *Service) CountCampaignsForOrganizationID( return count, nil } - -func errUnlessDraftCampaign(campaign *coredata.AccessReviewCampaign) error { - if campaign.Status == coredata.AccessReviewCampaignStatusDraft { - return nil - } - - switch campaign.Status { - case coredata.AccessReviewCampaignStatusInProgress: - return NewCampaignInProgressError(campaign.ID) - case coredata.AccessReviewCampaignStatusPendingActions: - return NewCampaignPendingActionsError(campaign.ID) - case coredata.AccessReviewCampaignStatusCompleted: - return NewCampaignCompletedError(campaign.ID) - case coredata.AccessReviewCampaignStatusCancelled: - return NewCampaignCancelledError(campaign.ID) - default: - return NewCampaignInProgressError(campaign.ID) - } -} diff --git a/pkg/accessreview/errors.go b/pkg/accessreview/errors.go index ee87b1859..3fd1f389b 100644 --- a/pkg/accessreview/errors.go +++ b/pkg/accessreview/errors.go @@ -28,11 +28,12 @@ import ( ) var ( - ErrCampaignMissingSources = errors.New("access review campaign missing scope sources") - ErrCampaignInProgress = errors.New("access review campaign in progress") - ErrCampaignPendingActions = errors.New("access review campaign pending actions") - ErrCampaignCompleted = errors.New("access review campaign completed") - ErrCampaignCancelled = errors.New("access review campaign cancelled") + ErrCampaignMissingSources = errors.New("access review campaign missing scope sources") + ErrCampaignNotDraft = errors.New("access review campaign not draft") + ErrCampaignNotDeletable = errors.New("access review campaign not deletable") + ErrCampaignNotPendingActions = errors.New("access review campaign not pending actions") + ErrCampaignCompleted = errors.New("access review campaign completed") + ErrCampaignCancelled = errors.New("access review campaign cancelled") ) type ( @@ -40,11 +41,15 @@ type ( CampaignID gid.GID } - CampaignInProgressError struct { + CampaignNotDraftError struct { CampaignID gid.GID } - CampaignPendingActionsError struct { + CampaignNotDeletableError struct { + CampaignID gid.GID + } + + CampaignNotPendingActionsError struct { CampaignID gid.GID } @@ -72,28 +77,46 @@ func (e *CampaignMissingSourcesError) Is(target error) bool { return target == ErrCampaignMissingSources } -func NewCampaignInProgressError(campaignID gid.GID) error { - return &CampaignInProgressError{CampaignID: campaignID} +func NewCampaignNotDraftError(campaignID gid.GID) error { + return &CampaignNotDraftError{CampaignID: campaignID} } -func (e *CampaignInProgressError) Error() string { - return fmt.Sprintf("access review campaign %q is in progress", e.CampaignID) +func (e *CampaignNotDraftError) Error() string { + return fmt.Sprintf("access review campaign %q is not in draft", e.CampaignID) } -func (e *CampaignInProgressError) Is(target error) bool { - return target == ErrCampaignInProgress +func (e *CampaignNotDraftError) Is(target error) bool { + return target == ErrCampaignNotDraft } -func NewCampaignPendingActionsError(campaignID gid.GID) error { - return &CampaignPendingActionsError{CampaignID: campaignID} +func NewCampaignNotDeletableError(campaignID gid.GID) error { + return &CampaignNotDeletableError{CampaignID: campaignID} } -func (e *CampaignPendingActionsError) Error() string { - return fmt.Sprintf("access review campaign %q is pending actions", e.CampaignID) +func (e *CampaignNotDeletableError) Error() string { + return fmt.Sprintf( + "access review campaign %q cannot be deleted unless it is draft or cancelled", + e.CampaignID, + ) } -func (e *CampaignPendingActionsError) Is(target error) bool { - return target == ErrCampaignPendingActions +func (e *CampaignNotDeletableError) Is(target error) bool { + return target == ErrCampaignNotDeletable +} + +func NewCampaignNotPendingActionsError(campaignID gid.GID) error { + return &CampaignNotPendingActionsError{CampaignID: campaignID} +} + +func (e *CampaignNotPendingActionsError) Error() string { + return fmt.Sprintf( + "access review campaign %q cannot be closed unless it is pending actions", + e.CampaignID, + ) +} + +func (e *CampaignNotPendingActionsError) Is(target error) bool { + return target == ErrCampaignNotPendingActions } func NewCampaignCompletedError(campaignID gid.GID) error { @@ -101,7 +124,7 @@ func NewCampaignCompletedError(campaignID gid.GID) error { } func (e *CampaignCompletedError) Error() string { - return fmt.Sprintf("access review campaign %q is completed", e.CampaignID) + return fmt.Sprintf("access review campaign %q is already completed", e.CampaignID) } func (e *CampaignCompletedError) Is(target error) bool { @@ -113,7 +136,7 @@ func NewCampaignCancelledError(campaignID gid.GID) error { } func (e *CampaignCancelledError) Error() string { - return fmt.Sprintf("access review campaign %q is cancelled", e.CampaignID) + return fmt.Sprintf("access review campaign %q is already cancelled", e.CampaignID) } func (e *CampaignCancelledError) Is(target error) bool { diff --git a/pkg/accessreview/errors_test.go b/pkg/accessreview/errors_test.go index 05a112f67..701831a22 100644 --- a/pkg/accessreview/errors_test.go +++ b/pkg/accessreview/errors_test.go @@ -52,27 +52,39 @@ func TestCampaignClientErrors(t *testing.T) { sentinel: accessreview.ErrCampaignMissingSources, }, { - name: "in progress", - err: accessreview.NewCampaignInProgressError(campaignID), - wantText: fmt.Sprintf("access review campaign %q is in progress", campaignID), - sentinel: accessreview.ErrCampaignInProgress, + name: "not draft", + err: accessreview.NewCampaignNotDraftError(campaignID), + wantText: fmt.Sprintf("access review campaign %q is not in draft", campaignID), + sentinel: accessreview.ErrCampaignNotDraft, }, { - name: "pending actions", - err: accessreview.NewCampaignPendingActionsError(campaignID), - wantText: fmt.Sprintf("access review campaign %q is pending actions", campaignID), - sentinel: accessreview.ErrCampaignPendingActions, + name: "not deletable", + err: accessreview.NewCampaignNotDeletableError(campaignID), + wantText: fmt.Sprintf( + "access review campaign %q cannot be deleted unless it is draft or cancelled", + campaignID, + ), + sentinel: accessreview.ErrCampaignNotDeletable, + }, + { + name: "not pending actions", + err: accessreview.NewCampaignNotPendingActionsError(campaignID), + wantText: fmt.Sprintf( + "access review campaign %q cannot be closed unless it is pending actions", + campaignID, + ), + sentinel: accessreview.ErrCampaignNotPendingActions, }, { name: "completed", err: accessreview.NewCampaignCompletedError(campaignID), - wantText: fmt.Sprintf("access review campaign %q is completed", campaignID), + wantText: fmt.Sprintf("access review campaign %q is already completed", campaignID), sentinel: accessreview.ErrCampaignCompleted, }, { name: "cancelled", err: accessreview.NewCampaignCancelledError(campaignID), - wantText: fmt.Sprintf("access review campaign %q is cancelled", campaignID), + wantText: fmt.Sprintf("access review campaign %q is already cancelled", campaignID), sentinel: accessreview.ErrCampaignCancelled, }, } diff --git a/pkg/server/api/console/v1/access_review_campaign_resolvers.go b/pkg/server/api/console/v1/access_review_campaign_resolvers.go index 7578d175b..84059aa52 100644 --- a/pkg/server/api/console/v1/access_review_campaign_resolvers.go +++ b/pkg/server/api/console/v1/access_review_campaign_resolvers.go @@ -761,13 +761,7 @@ func (r *mutationResolver) UpdateAccessReviewCampaign(ctx context.Context, input return nil, gqlutils.NotFound(ctx, err) } - if errorx.AnyOf( - err, - accessreview.ErrCampaignInProgress, - accessreview.ErrCampaignPendingActions, - accessreview.ErrCampaignCompleted, - accessreview.ErrCampaignCancelled, - ) { + if errorx.AnyOf(err, accessreview.ErrCampaignNotDraft) { return nil, gqlutils.Invalid(ctx, err) } @@ -793,6 +787,10 @@ func (r *mutationResolver) DeleteAccessReviewCampaign(ctx context.Context, input return nil, gqlutils.NotFound(ctx, err) } + if errors.Is(err, accessreview.ErrCampaignNotDeletable) { + return nil, gqlutils.Invalid(ctx, err) + } + r.logger.ErrorCtx(ctx, "cannot delete access review campaign", log.Error(err)) return nil, gqlutils.Internal(ctx) @@ -815,10 +813,7 @@ func (r *mutationResolver) StartAccessReviewCampaign(ctx context.Context, input if errorx.AnyOf( err, accessreview.ErrCampaignMissingSources, - accessreview.ErrCampaignInProgress, - accessreview.ErrCampaignPendingActions, - accessreview.ErrCampaignCompleted, - accessreview.ErrCampaignCancelled, + accessreview.ErrCampaignNotDraft, ) { return nil, gqlutils.Invalid(ctx, err) } @@ -842,6 +837,10 @@ func (r *mutationResolver) CloseAccessReviewCampaign(ctx context.Context, input campaign, err := r.accessReview.CloseCampaign(ctx, scope, input.AccessReviewCampaignID) if err != nil { + if errors.Is(err, accessreview.ErrCampaignNotPendingActions) { + return nil, gqlutils.Invalid(ctx, err) + } + r.logger.ErrorCtx(ctx, "cannot close access review campaign", log.Error(err)) return nil, gqlutils.Internal(ctx) @@ -895,13 +894,7 @@ func (r *mutationResolver) AddAccessReviewCampaignSource(ctx context.Context, in return nil, gqlutils.NotFound(ctx, err) } - if errorx.AnyOf( - err, - accessreview.ErrCampaignInProgress, - accessreview.ErrCampaignPendingActions, - accessreview.ErrCampaignCompleted, - accessreview.ErrCampaignCancelled, - ) { + if errors.Is(err, accessreview.ErrCampaignNotDraft) { return nil, gqlutils.Invalid(ctx, err) } @@ -931,6 +924,10 @@ func (r *mutationResolver) RemoveAccessReviewCampaignSource(ctx context.Context, }, ) if err != nil { + if errors.Is(err, accessreview.ErrCampaignNotDraft) { + return nil, gqlutils.Invalid(ctx, err) + } + r.logger.ErrorCtx(ctx, "cannot remove scope source from access review campaign", log.Error(err)) return nil, gqlutils.Internal(ctx)