Skip to content
This repository has been archived by the owner on Feb 5, 2018. It is now read-only.

Add option --milestone to open_issue command #95

Closed
wants to merge 11 commits into from
Closed

Add option --milestone to open_issue command #95

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
419a073
added option to include milestone on issue, no tests yet
tarebyte Nov 6, 2014
7b2bf9b
only add milestone to the issue if it exists, fail the creation of th…
tarebyte Nov 6, 2014
c5fe543
convert hashes to string for body
tarebyte Nov 6, 2014
f6af4b4
convert body to string as well
tarebyte Nov 6, 2014
c92ea82
parse string back into JSON
tarebyte Nov 6, 2014
720cba8
retry
tarebyte Nov 7, 2014
d964df7
undo changes to other files
tarebyte Nov 7, 2014
dd60ad6
convert returns to string
tarebyte Nov 7, 2014
b4da552
symbolize keys
tarebyte Nov 7, 2014
f99aea2
refactor for clarity and functionality
tarebyte Nov 12, 2014
4019073
update milestone detection to fit ClientDecorator Pattern
tarebyte Nov 19, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/teachers_pet/actions/create_student_teams.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'active_support/core_ext/hash/keys'

module TeachersPet
module Actions
class CreateStudentTeams < Base
Expand All @@ -21,7 +23,8 @@ def create_student_teams
if team
puts "Team @#{org_login}/#{team_name} already exists."
else
team = self.client.create_team(org_login, team_name)
team = JSON.parse(self.client.create_team(org_login, team_name))
team.symbolize_keys!
end
self.client.add_users_to_team(org_login, team, usernames)
end
Expand Down
8 changes: 8 additions & 0 deletions lib/teachers_pet/actions/open_issue.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ def read_info
@issue = {
title: self.options[:title],
options: {
milestone: self.options[:milestone],
labels: self.options[:labels]
}
}
Expand Down Expand Up @@ -42,6 +43,13 @@ def create
next
end

if @issue[:options][:milestone].nil?
if self.client.milestone("#{@organization}/#{repo_name}", @issue[:options][:milestone]).nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

A little hard to understand what these conditionals are doing... mind adding inline comments, or (better yet) moving them into a method with a descriptive name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it is a convoluted.

The code I have actually won't work because it runs the conditional only if the milestone is nil.

What do you think about something like this?

# If the a milestone is an option, make sure the milestone is on GitHub
if @issue[:options][:milestone]
  unless milestone_exists_on_github?(self.client, "#{@organization}/#{repo_name}", @issue[:options][:milestone])
    puts " --> Milestone not found, skipping '#{repo_name}'"
    next
  end
end

def milestone_exists_on_github?(client, repo, milestone)
  !client.milestone(repo, milestone).nil?
end

puts " --> Milestone not found, skipping '#{repo_name}'"
next
end
end

# Create the issue with octokit
self.client.create_issue("#{@organization}/#{repo_name}", @issue[:title], @issue[:body], @issue[:options])
end
Expand Down
1 change: 1 addition & 0 deletions lib/teachers_pet/commands/open_issue.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class Cli
option :title, desc: "The title of the issue to be created"
option :body, banner: 'PATH', desc: "The path to the file containing the issue body (.txt or .md)"
option :labels, banner: 'LABEL1,LABEL2'
option :milestone, desc: "The milestone you would like to associate with the issue", type: :numeric
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason the CI breaks when all I add is this option

https://travis-ci.org/tarebyte/teachers_pet/jobs/40206454

Update: I created a bug elsewhere and didn't realize it. This is no longer true

Copy link
Contributor

Choose a reason for hiding this comment

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

Where would someone find the milestone "number"? Doesn't seem that it's reflected in the URL, e.g. https://github.com/scala/scala/milestones/on-hold.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see anywhere in the UI or the URL where the milestones are listed, maybe I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to move away from prompts, but one solution would be that if they include --milestone without a value, it could list the milestones for the repository and ask them to choose which to use.

This leads to another question, which is how would the milestones get created across the repositories in the first place. Do we need an create_milestone command? This feels like 🐢s all the way down, and while I appreciate all the work you've put into this, I'm questioning the value relative to this extra required effort.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a problem, if nothing else I had good learning experience out of it, and I can extract out the json code I wrote and make that into a new PR 😃.

I don't know how often educators use milestones in students repos. My guess would be not very often. Just by talking to educators the last few days it seems like it's a very rare case.


students_option
common_options
Expand Down
4 changes: 2 additions & 2 deletions spec/commands/create_student_teams_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def stub_owners_only
}.to_json).to_return(body: {
id: i,
name: student
})
}.to_json)

# Checks for existing team members
# TODO No need to retrieve members for a new team
Expand Down Expand Up @@ -62,7 +62,7 @@ def stub_owners_only
}.to_json).to_return(body: {
id: 1,
name: 'studentteam1'
})
}.to_json)

# Checks for existing team members
# TODO No need to retrieve members for a new team
Expand Down
22 changes: 18 additions & 4 deletions spec/commands/open_issue_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
include CommandHelpers

context 'through CLI' do
def common_test(labels='')
def common_test(labels='', milestone=nil)
issue_title = "Issue Test"
request_stubs = []

Expand All @@ -16,6 +16,10 @@ def common_test(labels='')
student_usernames.each do |username|
# Action checks that repos exist already
request_stubs << stub_request(:get, "https://testteacher:[email protected]/repos/testorg/#{username}-testrepo")

unless milestone
request_stubs << stub_get_json("https://testteacher:[email protected]/repos/testorg/#{username}-testrepo/milestones/", number: milestone)
end
end

# create the issue in each repo
Expand All @@ -26,10 +30,15 @@ def common_test(labels='')
team_id = st[:id]
end
end
issue_body = File.read(issue_fixture_path).gsub("\n", "\\n")
labels_list = labels.split(",").map(&:strip).to_s.delete(' ')
issue_body = File.read(issue_fixture_path)
labels_list = labels.split(",").map(&:strip)
request_stubs << stub_request(:post, "https://testteacher:[email protected]/repos/testorg/#{username}-testrepo/issues").
with(body: "{\"labels\":#{labels_list},\"title\":\"#{issue_title}\",\"body\":\"#{issue_body}\"}").
with(body: {
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems a lot better to me than trying to type out the raw string yourself.

This convention is also being used in create_student_teams_spec.rb

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

milestone: milestone,
labels: labels_list,
title: issue_title,
body: issue_body
}.to_json).
to_return(status: 201)
end

Expand All @@ -40,6 +49,7 @@ def common_test(labels='')
title: issue_title,
body: issue_fixture_path,
labels: labels,
milestone: milestone,

students: students_list_fixture_path,

Expand All @@ -59,5 +69,9 @@ def common_test(labels='')
it "open issue with labels" do
common_test('bug, feature')
end

it "open issue with milestone" do
common_test('bug, feature', 1)
end
end
end