Skip to content

Implementing issue template GraphQL API call#2539

Merged
mislav merged 5 commits intocli:trunkfrom
divyaramanathan:issue-create-template
Feb 17, 2021
Merged

Implementing issue template GraphQL API call#2539
mislav merged 5 commits intocli:trunkfrom
divyaramanathan:issue-create-template

Conversation

@divyaramanathan
Copy link
Contributor

@divyaramanathan divyaramanathan commented Dec 3, 2020

closes #2361, fixes #875

In this PR, the following changes were made:

  • Added new IssueTemplate type and necessary API calls
  • Added the required IssueTemplates query
  • Attempted to modify 'create.go' to use this API

}

func IssueTemplates(client *Client, repo *Repository) ([]IssueTemplate, error) {
type response struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could simplify/shorten this GQL request using the functionality provided by github.com/shurcooL/githubv4 so that the response struct also acts as the query. Many of the queries in api/queries_repo.go use this style. MilestoneByNumber is a short simple one that might be good to copy from.

return nil, err
}

if !resp.Repository.HasIssuesEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that this check is outside the scope of responsibilities for this function. If this api call is being made then the caller should be responsible for making sure the repository has issue enabled.

Comment on lines +197 to +221
// overwrites previous templateContent
var templates []api.IssueTemplate
templates, err = api.IssueTemplates(apiClient, repo)
templateContent = templates[0].Body
if err != nil {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the discussion in the issue I think this API call should replace the call to prShared.FindTemplates on line 108.

I am going to cc @mislav since it was his his comment about using the API both when -R is provided and when it is not which makes me think that is the desired approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @samcoe for the initial review! You are right that this call needs to happen earlier.

@divyaramanathan You will want to invoke the new API logic just before TemplateSurvey(), since that is where the user will be presented with a list of templates to choose between. Also, since the new API approach replaces looking up templates from the filesystem, you can remove the old filesystem lookups from this file.

@samcoe
Copy link
Contributor

samcoe commented Dec 8, 2020

This is great progress so far. The API code looks good, just need to hook up the API response to the survey code properly.

mislav
mislav previously requested changes Dec 9, 2020
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@divyaramanathan Thanks so much for taking this on! I'm really looking forward to improving templates this way.

Comment on lines +197 to +221
// overwrites previous templateContent
var templates []api.IssueTemplate
templates, err = api.IssueTemplates(apiClient, repo)
templateContent = templates[0].Body
if err != nil {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @samcoe for the initial review! You are right that this call needs to happen earlier.

@divyaramanathan You will want to invoke the new API logic just before TemplateSurvey(), since that is where the user will be presented with a list of templates to choose between. Also, since the new API approach replaces looking up templates from the filesystem, you can remove the old filesystem lookups from this file.

@zaboyle
Copy link
Contributor

zaboyle commented Dec 11, 2020

@mislav @samcoe thanks so much for your awesome feedback!! @divyaramanathan and I made some changes to reflect this behavior:

  • addressed 2 suggestions from @samcoe to shorten/simplify GQL call
  • removed references to old filesystem lookups for issue templates
  • modified TemplateSurvey and selectTemplate logic to work with new IssueTemplate objects

We're still having some issues, mainly:

  1. the template titles aren't displaying properly in the terminal
    image

  2. it seems only 1 template is getting returned from our API call

We did, however, manage to grab this template and edit its contents:
image

We think we're very very close, and would love some help on how to debug this API call and terminal rendering (we tried Postman but need an auth key). Thanks so much!!

@vilmibm vilmibm self-requested a review December 18, 2020 22:11
@vilmibm
Copy link
Contributor

vilmibm commented Jan 20, 2021

@mislav to get tests working and add conditional for older enterprise versions

@mislav mislav force-pushed the issue-create-template branch from 1a5801a to 83bb1bf Compare February 10, 2021 17:23
@mislav mislav requested a review from samcoe February 10, 2021 17:24
@mislav mislav changed the title [WIP] #2361 Implementing issue template GraphQL API call Implementing issue template GraphQL API call Feb 10, 2021
@mislav
Copy link
Contributor

mislav commented Feb 10, 2021

I have updated this branch to have both issue create and pr create work with the new templates approach:

  • If issue templates API is available (e.g. on github.com and recent GHE versions), read the templates from API instead of from disk. This fixes issue create -R <repo> use, as well as having templates always working regardless of the current branch being checked out.
  • When an issue template was selected and the issue templates API is available, include the issueTemplate parameter in the final issue payload to enable template labels/assignees/projects to be applied. Fixes Creating an issue from a template doesn't apply template labels to newly created issue #875
    • ⚠️ Known issue: when "Continue in browser" was used, the selected template will not apply appropriate labels/assignees/projects. This is because the web interface requires a template filename for its ?template=... query param, whereas the API only provides us with the template name.
  • Fall back to reading templates from disk when the API isn't available.
  • "Legacy" GitHub templates are still being read from disk as before and applied if no regular template was found or selected, provided that -R wasn't used.

@divyaramanathan @zaboyle Thank you so much for getting started on this implementation! 🙏


if template != nil {
templateContent = string(template.Body())
if ok, _ := tpl.HasAPI(); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposing HasAPI() seems a bit odd and feels like details only the template manager should be aware of. What if, when doing the fetch of the templates, we don't set the template name if !HasAPI() that way we can just call template.Name() here.

@@ -0,0 +1,252 @@
package shared
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this direction. I think it would be good to add some unit tests to test these new functions directly since the top level command tests don't necessarily cover all the cases that are handled here.

This comment was marked as spam.

Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mislav Thanks for addressing the feedback. The changes look great. Appreciate your work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gh issue create -R doesn't show issue templates Creating an issue from a template doesn't apply template labels to newly created issue

6 participants