Draft for campaigns schema #10931
Draft for campaigns schema #10931
Conversation
| type CampaignPreview { | ||
| # URL where this preview can be looked at in the UI. | ||
| url: String! | ||
| unchanged(first: Int, after: String): ExternalChangesetConnection! |
There was a problem hiding this comment.
In the UI, the user doesn't really know if they are looking at an ExternalChangeset or a Patch. To the user, it feels like they are looking at an "unpublished changeset" or a "published changeset". That is the distinction that is apparent to them.
Should we make added here return an ExternalChangesetConnection as well? What is the benefit of having it return a PatchConnection?
There was a problem hiding this comment.
added patches won't be published while still in preview, so we really only have the patches.
I just saw I made a mistake here, changed needs to be PatchConnection as well.
(Mirroring the logic from here)
There was a problem hiding this comment.
(This is not urgent.)
Even though they won't be published, it still is useful for the caller to know "What will the new changesets for these patches look like? What will their title be?"
(Gotta run, will write more soon.)
There was a problem hiding this comment.
I figured we could construct that from the patch + the description/title properties from the Preview type, so we don't have to implement a PreviewExternalChangeset or something, that doesn't have URLs to the changeset on the codehost, when they don't exist yet.
There was a problem hiding this comment.
I think this CampaignUpdatePreview type should be more comprehensive/explicit. What you said relies on the frontend to implement some of the update preview logic, right?
Also, it's possible to have unchanged or deleted patches (that are not published yet), so for consistency and to avoid confusion, those fields should have the same type as added.
I think we should have the following fields in this type:
unchanged: ChangesetOrChangesetPreviewConnection!
# where ChangesetOrChangesetPreviewConnection is union of ExternalChangeset | ChangesetPreview, and ChangesetPreview is used instead of Patch everywhere except when uploading patches
changed: ChangesetDeltaConnection!
# where ChangesetDeltaConnection is like { old: ChangesetOrChangesetPreview, new: ChangesetOrChangesetPreview, isTitleChanged: Boolean!, isDescriptionChanged: Boolean!, isPatchChanged: Boolean!, willPerformExternalOperation: Boolean! }
deleted: ChangesetOrChangesetPreviewConnection!
added: ChangesetOrChangesetPreviewConnection!
WDYT?
There was a problem hiding this comment.
I didn't want to align the API too closely with how we render stuff, but I guess that doesn't really matter.
Agree that your proposed change is good. @mrnugget that would align with the change of making a Changeset interface. Should we pull that out?
And @sqs: Did you intentionally make old and new nullable? In my understanding we would always have both
There was a problem hiding this comment.
@eseliger: old and new being nullable was not intentional.
Although I guess we could just have a single updates: ChangesetDeltaConnection! field in CampaignUpdatePreview that would cover all of the cases. That would have the nice property of letting us put add'l information on the other fields that is relevant, such as being able to know "for this deletion, is it actually going to close something on the code host, or just delete something on Sourcegraph that isn't yet published?"
In that case, old and new should be nullable to indicate whether it was added or deleted or changed.
I don't think this single updates field is necessary, though.
There was a problem hiding this comment.
Yeah, makes sense in a way, but then the logic in determining what actually happens in the update is at the clients end again, so I guess to make it more accessible to API consumers, we can leave it as added, deleted,.. if you agree.
I'll update the schema with the proposed changes now
There was a problem hiding this comment.
Agree that your proposed change is good. @mrnugget that would align with the change of making a Changeset interface. Should we pull that out?
Are you referring to the changes I'm making for repository permissions? And then adding a third implementation of that interface, a ChangesetPreview?
You can take a look at the changes I have in https://github.com/sourcegraph/sourcegraph/pull/11071/files#diff-eb11139d7a9813ee0180fcb7dcdcd52f and then pull what's necessary into this proposal here.
Does that work?
There was a problem hiding this comment.
Done!
@mrnugget yes I do. That should work, I'll copy that over, once this PR is approved otherwise
Co-authored-by: Quinn Slack <[email protected]>
| # Patch ID is a UUID, base64 encoded. That is, so the patch IDs can't be guessed. | ||
| union PatchInputOrID = PatchInput | PatchIDInput | ||
|
|
||
| interface Changeset { |
There was a problem hiding this comment.
Todo: copy over from @mrnugget's PR
No description provided.