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

check for disabled issues in issue view #240

Merged
merged 3 commits into from
Jan 22, 2020
Merged

Conversation

vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Jan 21, 2020

closes #226

this PR adds another graphql query to gh issue view to ensure that a repo actually has issues
enabled before trying to query issues.

~/s/testing $ gh issue view 16
the 'vilmibm/testing' repository has disabled issues

@vilmibm vilmibm requested a review from mislav January 21, 2020 21:51
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.

Thanks for handling this!

This smells to me like a bug in our API: if a repository has issues disabled, how come it lets us query specific issues from within it? But in any case, we have to work around it.

command/issue.go Outdated
@@ -215,6 +215,14 @@ func issueView(cmd *cobra.Command, args []string) error {
return err
}

issuesEnabled, err := api.HasIssuesEnabled(apiClient, baseRepo)
Copy link
Contributor

@mislav mislav Jan 22, 2020

Choose a reason for hiding this comment

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

This looks good! But I'm wondering whether we can avoid making this additional query every time? Using GraphQL, we can request extra information in existing queries.

For instance, this is done right before an issueFromArg(). In that call, we call api.IssueByNumber() to check if an issue exists. Within that call, we could add the hasIssuesEnabled field to repository and return an error if it returned false. That way we save ourselves an extra round trip to the server.

If this idea doesn't seem clear to you, let me know and we can try doing this together!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's clear. i have an impulse to have more clear and specific API helper functions but in retrospect that goes against the spirit of graphql :( i can retool for that.

@vilmibm
Copy link
Contributor Author

vilmibm commented Jan 22, 2020

ok, ready for a re-review @mislav

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.

Fantastic! Thank you for following up 🎉

@vilmibm vilmibm merged commit 5965698 into master Jan 22, 2020
@mislav mislav deleted the disabled-issue-preview branch January 22, 2020 23:00
mislav pushed a commit that referenced this pull request Jan 23, 2020
check for disabled issues in issue view
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.

Block issue preview if Issues disabled
2 participants