campaigns: Support changeset filtering

[ryanslade]

This adds support for filtering by State, ReviewState and CheckState

The columns are updated when we sync or receive a webhook as these are the moment we get input about external state.

We may need to add a new enum value, UNKNOWN to each of the GraphQL enums so that we can filter to display those states.

The migration adds new columns and types but leaves state as UNKNOWN until next sync or webhook. I think this is fine considering we don't have any customers with large numbers of changesets.

I'll update the resolvers to use the new columns in another PR.

Part of: #8462

[codecov]

Codecov Report

Merging #8848 into master will decrease coverage by 0.01%.
The diff coverage is 44.94%.

@@            Coverage Diff             @@
##           master    #8848      +/-   ##
==========================================
- Coverage   41.63%   41.62%   -0.02%     
==========================================
  Files        1314     1314              
  Lines       70680    70822     +142     
  Branches     6554     6556       +2     
==========================================
+ Hits        29425    29477      +52     
- Misses      38577    38656      +79     
- Partials     2678     2689      +11

Impacted Files	Coverage Δ
web/src/enterprise/campaigns/detail/backend.ts	10.9% <ø> (ø)	⬆️
...b/src/enterprise/campaigns/detail/CampaignTabs.tsx	72.22% <ø> (ø)	⬆️
...rprise/campaigns/detail/changesets/presentation.ts	100% <ø> (ø)	⬆️
enterprise/internal/campaigns/service.go	35.41% <0%> (-0.25%)	⬇️
cmd/frontend/graphqlbackend/campaigns.go	0% <0%> (ø)	⬆️
...rc/enterprise/campaigns/detail/CampaignDetails.tsx	52.28% <0%> (ø)	⬆️
...erprise/internal/campaigns/resolvers/changesets.go	58.74% <100%> (+1.27%)	⬆️
enterprise/internal/campaigns/syncer.go	68.46% <100%> (+0.24%)	⬆️
...nterprise/internal/campaigns/resolvers/resolver.go	36.96% <11.11%> (-1.48%)	⬇️
enterprise/internal/campaigns/webhooks.go	22.22% <60%> (+0.69%)	⬆️
... and 4 more

[mrnugget]

Happy to see that updating the state based on syncing data and webhook events is as easy as this.

Couple of thoughts:

• I don't think we should use enums in the database for this. Especially not if we use a bail-out value like UNKNOWN. I would simply use a nullable text column and make sure we handle the null value properly.
• If we could get away without using UNKNOWN, that would be great. ChangesetReviewStatePending is already a good default value. As is ChangesetStateOpen for State().
• Can we already use these values in the State(), ReviewState(), CheckState() methods on ChangesetResolver?

[eseliger]

[ryanslade]

Currently our GraphQL and code enums do not match up

# The state of a Changeset Review
enum ChangesetReviewState {
    APPROVED
    CHANGES_REQUESTED
    PENDING
}

# The state of continuous integration checks on a changeset
enum ChangesetCheckState {
    PENDING
    PASSED
    FAILED
}

// ChangesetReviewState constants.
const (
	ChangesetReviewStateApproved         ChangesetReviewState = "APPROVED"
	ChangesetReviewStateChangesRequested ChangesetReviewState = "CHANGES_REQUESTED"
	ChangesetReviewStatePending          ChangesetReviewState = "PENDING"
	ChangesetReviewStateCommented        ChangesetReviewState = "COMMENTED"
	ChangesetReviewStateDismissed        ChangesetReviewState = "DISMISSED"
)

// ChangesetCheckState constants.
type ChangesetCheckState string

const (
	ChangesetCheckStateUnknown ChangesetCheckState = "UNKNOWN"
	ChangesetCheckStatePending ChangesetCheckState = "PENDING"
	ChangesetCheckStatePassed  ChangesetCheckState = "PASSED"
	ChangesetCheckStateFailed  ChangesetCheckState = "FAILED"
)

For CheckState if our Go type is UNKNOWN we return nil to the API. Do we want to allow filtering to UNKNOWN check states?

Review state currently works since we don't currently return any values that don't exist in the GraphQL API but it does make me a bit nervous that it could break in future.

@eseliger @mrnugget Thoughts?

[ryanslade]

I had an idea for how to handle review state. Once all the logic of computing state is moved to update time then the code at the resolver layer will just be reading the value from a field and we can decide at that point what to do with values that don't exist in graohql. At least then that logic is isolated to the resolver layer. It's still not totally type safe but I think it's more explicit that what's happening now

[ryanslade]

PTAL

@mrnugget I'll do the resolver changes in another PR as this one is already pretty big

[mrnugget]

Ah, sorry, I had "Approve" selected but meant: changes requested. Take a look at the comments.

[mrnugget]

If I understand it correctly: the UI in here completes this feature, right? If so, please add a changelog entry :)

[mrnugget]

Approving. Discussed comments etc. on Slack call.