Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design a GraphQL API that would allow us to show either the "Update campaign" or "Create campaign" button on the apply page #12468

Closed
mrnugget opened this issue Jul 24, 2020 · 9 comments
Assignees
Labels
batch-changes Issues related to Batch Changes
Milestone

Comments

@mrnugget
Copy link
Contributor

We need to way to tell whether the application of the given campaign spec on the apply page would update an existing campaign or create a new campaign.

Quoting from our Campaigns sync doc:

When the user runs src campaign apply, it prints a URL like /{users,organizations}/$NAMESPACE_NAME/campaigns/apply?spec=$CAMPAIGN_SPEC_ID. That page shows something slightly different…

  • if the apply would create a new campaign: displays the spec and a "Create campaign" button. This uses the createCampaign mutation to ensure that if another campaign with the same namespace+name was created after the user loaded the page, that it would error instead of clobbering that unintentionally.
  • if the apply would update an existing campaign: displays a short summary of the existing campaign (namespace+name, link, and something like ), shows the new campaign spec (no delta) [that is the changeset specs], and an "Update campaign" button. This uses the applyCampaign mutation with ensureCampaign to ensure that the "Update campaign" button in fact updates the intended campaign and does not clobber another campaign in case the existing one is deleted and another one with the same namespace+name is created after the user loads the page.
    Runs in the namespace, URL: /users/abc/campaigns/apply?specID

Reply to this ticket with the proposed API

@eseliger
Copy link
Member

This API will be obsolete once we have the delta API in place, so I already deprecated it, although it's not even implemented yet.
Getting the campaign that is targeted by the given campaignSpec will return the campaign if it already exists.

diff --git a/cmd/frontend/graphqlbackend/schema.graphql b/cmd/frontend/graphqlbackend/schema.graphql
index 564d6653b4..35fe964b4a 100755
--- a/cmd/frontend/graphqlbackend/schema.graphql
+++ b/cmd/frontend/graphqlbackend/schema.graphql
@@ -1184,6 +1184,15 @@ type Query {
         viewerCanAdminister: Boolean
     ): CampaignConnection!
 
+    # Gets the campaign that is targetted by the given campaign spec. Returns null, when the targetted campaign doesn't yet exist.
+    # This is the case when no match was found for the given namespace + campaign name combination.
+    # DEPRECATED: This API is only used to determine whether a campaign will be created or updated by running the applyCampaign
+    # mutation. Once the delta API is in place, this API is not required anymore and should NOT be used by anyone.
+    campaignForSpec(campaignSpec: ID!): Campaign
+        @deprecated(
+            reason: "This API is going to disappear in a future version of Sourcegraph. It acts as a lightweight version of the upcoming delta API."
+        )
+
     # Looks up a repository by either name or cloneURL.
     repository(
         # Query the repository by name, for example "github.com/gorilla/mux".

@mrnugget
Copy link
Contributor Author

Looks good to me! But wouldn't it be easier for the frontend if this was an attribute on the campaignSpec? Something like campaignSpec.appliesToCampaign: Campaign? Then you'd only have to load the CampaignSpec and can show the button immediately.

@eseliger
Copy link
Member

That's actually a good idea. That field could even stay useful, so it might not need the immediate deprecation, I've updated the patch:

diff --git a/cmd/frontend/graphqlbackend/schema.graphql b/cmd/frontend/graphqlbackend/schema.graphql
index 564d6653b4..6f1c86e18b 100755
--- a/cmd/frontend/graphqlbackend/schema.graphql
+++ b/cmd/frontend/graphqlbackend/schema.graphql
@@ -659,6 +659,9 @@ type CampaignSpec implements Node {
 
     # When true, the viewing user can apply this spec.
     viewerCanAdminister: Boolean!
+
+    # The campaign this spec will update when applied. Is null, when the campaign doesn't yet exist.
+    appliesToCampaign: Campaign
 }
 
 # A user (identified either by username or email address) with its repository permission.

@mrnugget
Copy link
Contributor Author

mrnugget commented Aug 3, 2020

Thanks! I know I came up with it, but now I'm wondering: is applicableToCampaign better than appliesToCampaign? @LawnGnome some native-speaker thoughts on this?

@eseliger
Copy link
Member

eseliger commented Aug 3, 2020

I think I'd prefer the latter, if grammar says yes - because it makes more clear that this WILL happen, while applicableToCampaign just conveys that it would work - if you wanted to. But let's defer to @LawnGnome :)

@mrnugget
Copy link
Contributor Author

mrnugget commented Aug 3, 2020

while applicableToCampaign just conveys that it would work

But isn't that correct? You don't have to apply the campaign spec when you preview it.

@eseliger
Copy link
Member

eseliger commented Aug 3, 2020

Yes, but my understanding was that when you apply, it will apply (applies) to that campaign. applicable to me more conveyed that that campaign in general would be compatible to be updated by this spec, but doesn't as strong underline that there will always be only that one specific campaign that is applicable. But maybe I'm interpreting things where no meaning exists 😁

@LawnGnome
Copy link
Contributor

You both know I'm Australian, right? Some people would argue I don't speak proper English.

I'd go with applies, though, for the same reasons @eseliger said.

@mrnugget
Copy link
Contributor Author

mrnugget commented Aug 7, 2020

Closing this because it's done! The API implementation of this is covered in #12469 but can also be tackled separately.

@mrnugget mrnugget closed this as completed Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
batch-changes Issues related to Batch Changes
Projects
None yet
Development

No branches or pull requests

3 participants