Re-attach changesets to campaign that were owned by campaign
Created by: mrnugget
This fixes https://github.com/sourcegraph/sourcegraph/issues/13849 by re-opening changesets that were previously detached from the campaign.
See this Slack thread for a demo video that I recorded to show off this bug fix and what it fixes.
Thoughts on design
I'm not 100% happy with the code in the reconciler yet. I have the feeling that there's some design that makes the distinction between applyCampaign
-directed state transitions (e.g. closing a changeset when detaching it from a campaign, or, like here, re-opening a detached one) and user-directed updates (e.g. update title and commit of a PR) clearer and easier to handle.
At its heart this problem revolves around the fact that a subset of the desired changeset state is included in the changeset spec and another set is not (since we don't allow closing in the spec). If we include our internal-state-directives in the changeset specs, they'd need to be in the schema and we'd need to create new changeset specs every time we want to make a change. That seems odd.
What we could do is to make this idea of internal-state-directives clearer by bundling them in a single column, for example, or treating all of them the same way. Not sure yet.
However, I think the code is fine to merge as it is. It's just something I keep thinking about.
Caveat
As it stands, this implementation could lead to changesets being re-opened after the user closed them on the code host. In practice, though, this won't currently happen, since the reconciler is only kicked off on user action, in which case we do want to reopen the changesets.
In order to avoid that, we could add another boolean on the changesets, reopening bool
, that, just like closing
, would cause the reconciler to reopen the changeset only on applyCampaign
.
Merge request reports
Activity
Created by: sourcegraph-bot
Notifying subscribers in CODENOTIFY files for diff 894e9c90bcce44b79d215faf18d02b351c6d483a...d45bdee30e50f96b76a5bb08ab8954ac11b62771.
Notify File(s) @eseliger enterprise/internal/campaigns/reconciler.go
enterprise/internal/campaigns/reconciler_test.go
enterprise/internal/campaigns/service_apply_campaign.go
enterprise/internal/campaigns/service_apply_campaign_test.go
enterprise/internal/campaigns/store_changesets.go
enterprise/internal/campaigns/store_changesets_test.go
enterprise/internal/campaigns/testing/changeset_source.goCreated by: codecov[bot]
Codecov Report
Merging #14099 into main will decrease coverage by
22.44%
. The diff coverage isn/a
.@@ Coverage Diff @@ ## main #14099 +/- ## =========================================== - Coverage 51.79% 29.34% -22.45% =========================================== Files 1524 713 -811 Lines 77703 20901 -56802 Branches 6991 6366 -625 =========================================== - Hits 40244 6134 -34110 + Misses 33833 14722 -19111 + Partials 3626 45 -3581
Flag Coverage Δ #go ?
#integration 30.44% <ø> (+0.09%)
#storybook ?
#typescript 29.34% <ø> (-22.00%)
#unit 19.47% <ø> (-14.45%)
Impacted Files Coverage Δ shared/src/util/jsonc.ts 0.00% <0.00%> (-100.00%)
shared/src/backend/errors.ts 0.00% <0.00%> (-100.00%)
shared/src/util/wordHelpers.ts 0.00% <0.00%> (-100.00%)
shared/src/settings/settings.ts 0.00% <0.00%> (-100.00%)
shared/src/util/createRecord.ts 0.00% <0.00%> (-100.00%)
shared/src/search/parser/tokens.ts 0.00% <0.00%> (-100.00%)
shared/src/util/rxjs/repeatUntil.ts 0.00% <0.00%> (-100.00%)
shared/src/api/client/api/content.ts 0.00% <0.00%> (-100.00%)
shared/src/api/client/types/hover.ts 0.00% <0.00%> (-100.00%)
shared/src/util/memoizeObservable.ts 0.00% <0.00%> (-100.00%)
... and 998 more