From 19c83f25410e197512284c9523333fed178dd130 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Sat, 27 Aug 2011 01:02:20 +0800 Subject: [PATCH] Refactored _issue_tracker_fields.html.haml by moving all field information into issue tracker models. This not only keeps the view DRY, but also streamlines the process of creating new issue trackers. Next step is to write a rails generator so that we can run 'rails generate issue_tracker NewIssueTracker'. Also, this commit contains a fix for the apps/edit page, where issue trackers were not being saved correctly. --- app/controllers/apps_controller.rb | 2 +- app/models/issue_trackers/fogbugz_tracker.rb | 18 ++++++++++++++++-- app/models/issue_trackers/github_issues_tracker.rb | 32 ++++++++++++++++++++++++++++++++ app/models/issue_trackers/github_tracker.rb | 21 --------------------- app/models/issue_trackers/lighthouse_tracker.rb | 14 ++++++++++++-- app/models/issue_trackers/mingle_tracker.rb | 23 +++++++++++++++++++++-- app/models/issue_trackers/pivotal_labs_tracker.rb | 9 +++++++-- app/models/issue_trackers/redmine_tracker.rb | 13 +++++++++++-- app/views/apps/_issue_tracker_fields.html.haml | 75 ++++++++++----------------------------------------------------------------- app/views/issue_trackers/github_body.txt.erb | 45 --------------------------------------------- app/views/issue_trackers/github_issues_body.txt.erb | 45 +++++++++++++++++++++++++++++++++++++++++++++ spec/controllers/apps_controller_spec.rb | 11 +++++++---- spec/factories/issue_tracker_factories.rb | 2 +- spec/models/issue_trackers/github_issues_tracker_spec.rb | 41 +++++++++++++++++++++++++++++++++++++++++ spec/models/issue_trackers/github_tracker_spec.rb | 41 ----------------------------------------- 15 files changed, 204 insertions(+), 188 deletions(-) create mode 100644 app/models/issue_trackers/github_issues_tracker.rb delete mode 100644 app/models/issue_trackers/github_tracker.rb delete mode 100644 app/views/issue_trackers/github_body.txt.erb create mode 100644 app/views/issue_trackers/github_issues_body.txt.erb create mode 100644 spec/models/issue_trackers/github_issues_tracker_spec.rb delete mode 100644 spec/models/issue_trackers/github_tracker_spec.rb diff --git a/app/controllers/apps_controller.rb b/app/controllers/apps_controller.rb index 1fc824c..1cbf4ff 100644 --- a/app/controllers/apps_controller.rb +++ b/app/controllers/apps_controller.rb @@ -46,7 +46,7 @@ class AppsController < InheritedResources::Base protected def initialize_subclassed_issue_tracker if params[:app][:issue_tracker_attributes] && tracker_type = params[:app][:issue_tracker_attributes][:type] - if IssueTracker.subclasses.map(&:to_s).concat(["IssueTracker"]).include?(tracker_type.to_s) + if IssueTracker.subclasses.map(&:name).concat(["IssueTracker"]).include?(tracker_type) @app.issue_tracker = tracker_type.constantize.new(params[:app][:issue_tracker_attributes]) end end diff --git a/app/models/issue_trackers/fogbugz_tracker.rb b/app/models/issue_trackers/fogbugz_tracker.rb index bbf5125..fffa9ef 100644 --- a/app/models/issue_trackers/fogbugz_tracker.rb +++ b/app/models/issue_trackers/fogbugz_tracker.rb @@ -1,9 +1,23 @@ class IssueTrackers::FogbugzTracker < IssueTracker Label = "fogbugz" - RequiredFields = %w(project_id account username password) + Fields = [ + [:project_id, { + :label => "Area Name" + }], + [:account, { + :label => "FogBugz URL", + :placeholder => "abc from http://abc.fogbugz.com/" + }], + [:username, { + :placeholder => "Username/Email for your account" + }], + [:password, { + :placeholder => "Password for your account" + }] + ] def check_params - if RequiredFields.detect {|f| self[f].blank? } + if Fields.detect {|f| self[f[0]].blank? } errors.add :base, 'You must specify your FogBugz Area Name, FogBugz URL, Username, and Password' end end diff --git a/app/models/issue_trackers/github_issues_tracker.rb b/app/models/issue_trackers/github_issues_tracker.rb new file mode 100644 index 0000000..e2c4a07 --- /dev/null +++ b/app/models/issue_trackers/github_issues_tracker.rb @@ -0,0 +1,32 @@ +class IssueTrackers::GithubIssuesTracker < IssueTracker + Label = "github" + Fields = [ + [:project_id, { + :label => "Account/Repository", + :placeholder => "errbit/errbit from https://github.com/errbit/errbit" + }], + [:username, { + :placeholder => "Your username on Github" + }], + [:api_token, { + :placeholder => "Your Github API Token" + }] + ] + + def check_params + if Fields.detect {|f| self[f[0]].blank? } + errors.add :base, 'You must specify your Github repository, username and API Token' + end + end + + def create_issue(err) + client = Octokit::Client.new(:login => username, :token => api_token) + issue = client.create_issue(project_id, issue_title(err), body_template.result(binding), options = {}) + err.update_attribute :issue_link, issue.html_url + end + + def body_template + @@body_template ||= ERB.new(File.read(Rails.root + "app/views/issue_trackers/github_issues_body.txt.erb").gsub(/^\s*/, '')) + end +end + diff --git a/app/models/issue_trackers/github_tracker.rb b/app/models/issue_trackers/github_tracker.rb deleted file mode 100644 index 0c2a8cd..0000000 --- a/app/models/issue_trackers/github_tracker.rb +++ /dev/null @@ -1,21 +0,0 @@ -class IssueTrackers::GithubTracker < IssueTracker - Label = "github" - RequiredFields = %w(project_id username api_token) - - def check_params - if RequiredFields.detect {|f| self[f].blank? } - errors.add :base, 'You must specify your Github repository, username and API token' - end - end - - def create_issue(err) - client = Octokit::Client.new(:login => username, :token => api_token) - issue = client.create_issue(project_id, issue_title(err), body_template.result(binding), options = {}) - err.update_attribute :issue_link, issue.html_url - end - - def body_template - @@body_template ||= ERB.new(File.read(Rails.root + "app/views/issue_trackers/github_body.txt.erb").gsub(/^\s*/, '')) - end -end - diff --git a/app/models/issue_trackers/lighthouse_tracker.rb b/app/models/issue_trackers/lighthouse_tracker.rb index 6ab64ae..bc3a8c3 100644 --- a/app/models/issue_trackers/lighthouse_tracker.rb +++ b/app/models/issue_trackers/lighthouse_tracker.rb @@ -1,9 +1,19 @@ class IssueTrackers::LighthouseTracker < IssueTracker Label = "lighthouseapp" - RequiredFields = %w(account api_token project_id) + Fields = [ + [:account, { + :placeholder => "abc from abc.lighthouseapp.com" + }], + [:api_token, { + :placeholder => "API Token for your account" + }], + [:project_id, { + :placeholder => "Lighthouse project" + }] + ] def check_params - if RequiredFields.detect {|f| self[f].blank? } + if Fields.detect {|f| self[f[0]].blank? } errors.add :base, 'You must specify your Lighthouseapp account, API token and Project ID' end end diff --git a/app/models/issue_trackers/mingle_tracker.rb b/app/models/issue_trackers/mingle_tracker.rb index 0c9a317..9997b57 100644 --- a/app/models/issue_trackers/mingle_tracker.rb +++ b/app/models/issue_trackers/mingle_tracker.rb @@ -1,9 +1,28 @@ class IssueTrackers::MingleTracker < IssueTracker Label = "mingle" - RequiredFields = %w(account project_id username password) + Fields = [ + [:account, { + :label => "Mingle URL", + :placeholder => "abc from http://abc.fogbugz.com/" + }], + [:project_id, { + :placeholder => "Mingle project" + }], + [:ticket_properties, { + :label => "Card Properties (comma separated key=value pairs)", + :placeholder => "card_type = Defect, defect_status = Open, priority = Essential" + }], + [:username, { + :label => "Sign-in name", + :placeholder => "Sign-in name for your account" + }], + [:password, { + :placeholder => "Password for your account" + }] + ] def check_params - if RequiredFields.detect {|f| self[f].blank? } or !ticket_properties_hash["card_type"] + if Fields.detect {|f| self[f[0]].blank? } or !ticket_properties_hash["card_type"] errors.add :base, 'You must specify your Mingle URL, Project ID, Card Type (in default card properties), Sign-in name, and Password' end end diff --git a/app/models/issue_trackers/pivotal_labs_tracker.rb b/app/models/issue_trackers/pivotal_labs_tracker.rb index 089eb4c..c33dba5 100644 --- a/app/models/issue_trackers/pivotal_labs_tracker.rb +++ b/app/models/issue_trackers/pivotal_labs_tracker.rb @@ -1,9 +1,14 @@ class IssueTrackers::PivotalLabsTracker < IssueTracker Label = "pivotal" - RequiredFields = %w(api_token project_id) + Fields = [ + [:api_token, { + :placeholder => "API Token for your account" + }], + [:project_id, {}] + ] def check_params - if RequiredFields.detect {|f| self[f].blank? } + if Fields.detect {|f| self[f[0]].blank? } errors.add :base, 'You must specify your Pivotal Tracker API token and Project ID' end end diff --git a/app/models/issue_trackers/redmine_tracker.rb b/app/models/issue_trackers/redmine_tracker.rb index efe8c90..9eeb134 100644 --- a/app/models/issue_trackers/redmine_tracker.rb +++ b/app/models/issue_trackers/redmine_tracker.rb @@ -1,9 +1,18 @@ class IssueTrackers::RedmineTracker < IssueTracker Label = "redmine" - RequiredFields = %w(account api_token project_id) + Fields = [ + [:account, { + :label => "Redmine URL", + :placeholder => "e.g. http://www.redmine.org/" + }], + [:api_token, { + :placeholder => "API Token for your account" + }], + [:project_id, {}] + ] def check_params - if RequiredFields.detect {|f| self[f].blank? } + if Fields.detect {|f| self[f[0]].blank? } errors.add :base, 'You must specify your Redmine URL, API token and Project ID' end end diff --git a/app/views/apps/_issue_tracker_fields.html.haml b/app/views/apps/_issue_tracker_fields.html.haml index 4f14d7c..ccbfd28 100644 --- a/app/views/apps/_issue_tracker_fields.html.haml +++ b/app/views/apps/_issue_tracker_fields.html.haml @@ -6,76 +6,21 @@ = label_tag :type_none, :for => label_for_attr(w, 'type_issuetracker'), :class => "label_radio none" do = w.radio_button :type, "IssueTracker", 'data-section' => 'none' (None) - = label_tag :type_github, :for => label_for_attr(w, 'type_githubtracker'), :class => "label_radio github" do - = w.radio_button :type, "GithubTracker", 'data-section' => 'github' - Github Issues - = label_tag :type_lighthouseapp, :for => label_for_attr(w, 'type_lighthousetracker'), :class => "label_radio lighthouseapp" do - = w.radio_button :type, "LighthouseTracker", 'data-section' => 'lighthouse' - Lighthouse - = label_tag :type_redmine, :for => label_for_attr(w, 'type_redminetracker'), :class => "label_radio redmine" do - = w.radio_button :type, "RedmineTracker", 'data-section' => 'redmine' - Redmine - = label_tag :type_pivotal, :for => label_for_attr(w, 'type_pivotallabstracker'), :class => "label_radio pivotal" do - = w.radio_button :type, "PivotalLabsTracker", 'data-section' => 'pivotal' - Pivotal Tracker - = label_tag :type_fogbugz, :for => label_for_attr(w, 'type_fogbugztracker'), :class => "label_radio fogbugz" do - = w.radio_button :type, "FogbugzTracker", 'data-section' => 'fogbugz' - FogBugz - = label_tag :type_mingle, :for => label_for_attr(w, 'type_mingletracker'), :class => "label_radio mingle" do - = w.radio_button :type, "MingleTracker", 'data-section' => 'mingle' - Mingle + - IssueTracker.subclasses.each do |tracker| + = label_tag "type_#{tracker::Label}:", :for => label_for_attr(w, "type_#{tracker.name.downcase.gsub(':','')}"), :class => "label_radio #{tracker::Label}" do + = w.radio_button :type, tracker.name, 'data-section' => tracker::Label + = tracker.name[/::(.*)Tracker/,1].titleize %div.tracker_params.none{:class => (w.object && !(w.object.class < IssueTracker)) ? 'chosen' : nil} %p When no issue tracker has been configured, you will be able to leave comments on errors. - %div.tracker_params.github{:class => w.object.is_a?(GithubTracker) ? 'chosen' : nil} - = w.label :project_id, "Account/Repository" - = w.text_field :project_id, :placeholder => "errbit/errbit from https://github.com/errbit/errbit" - = w.label :username, "Username" - = w.text_field :username, :placeholder => "Your username on Github" - = w.label :api_token, "Token" - = w.text_field :api_token, :placeholder => "Your API Token" - %div.tracker_params.lighthouse{:class => w.object.is_a?(LighthouseTracker) ? 'chosen' : nil} - = w.label :account, "Account" - = w.text_field :account, :placeholder => "abc from abc.lighthouseapp.com" - = w.label :api_token, "API token" - = w.text_field :api_token, :placeholder => "API Token for your account" - = w.label :project_id, "Project ID" - = w.text_field :project_id - %div.tracker_params.redmine{:class => w.object.is_a?(RedmineTracker) ? 'chosen' : nil} - = w.label :account, "Redmine URL" - = w.text_field :account, :placeholder => "like http://www.redmine.org/" - = w.label :api_token, "API token" - = w.text_field :api_token, :placeholder => "API Token for your account" - = w.label :project_id, "Project ID" - = w.text_field :project_id - %div.tracker_params.pivotal{:class => w.object.is_a?(PivotalLabsTracker) ? 'chosen' : nil} - = w.label :project_id, "Project ID" - = w.text_field :project_id - = w.label :api_token, "API token" - = w.text_field :api_token, :placeholder => "API Token for your account" - %div.tracker_params.fogbugz{:class => w.object.is_a?(FogbugzTracker) ? 'chosen' : nil} - = w.label :project_id, "Area Name" - = w.text_field :project_id - = w.label :account, "FogBugz URL" - = w.text_field :account, :placeholder => "abc from http://abc.fogbugz.com/" - = w.label :username, 'Username' - = w.text_field :username, :placeholder => 'Username/Email for your account' - = w.label :password, 'Password' - = w.password_field :password, :placeholder => 'Password for your account' - %div.tracker_params.mingle{:class => w.object.is_a?(MingleTracker) ? 'chosen' : nil} - = w.label :account, "Mingle URL" - = w.text_field :account, :placeholder => "http://mingle.yoursite.com/" - = w.label :project_id, "Project ID" - = w.text_field :project_id - = w.label :ticket_properties, "Card Properties (comma separated key=value pairs)" - = w.text_field :ticket_properties, :placeholder => "card_type = Defect, defect_status = Open, priority = Essential" - = w.label :username, 'Sign-in name' - = w.text_field :username, :placeholder => 'Sign-in name for your account' - = w.label :password, 'Password' - = w.password_field :password, :placeholder => 'Password for your account' + - IssueTracker.subclasses.each do |tracker| + %div.tracker_params{:class => (w.object.is_a?(tracker) ? 'chosen ' : '') << tracker::Label} + - tracker::Fields.each do |field, field_info| + = w.label field, field_info[:label] || field.to_s.titleize + = w.text_field field, :placeholder => field_info[:placeholder] .image_preloader - - %w(none github lighthouseapp redmine pivotal fogbugz mingle).each do |tracker| + - (IssueTracker.subclasses.map{|t| t::Label } << 'none').each do |tracker| = image_tag "#{tracker}_inactive.png" = image_tag "#{tracker}_create.png" diff --git a/app/views/issue_trackers/github_body.txt.erb b/app/views/issue_trackers/github_body.txt.erb deleted file mode 100644 index 8b3297f..0000000 --- a/app/views/issue_trackers/github_body.txt.erb +++ /dev/null @@ -1,45 +0,0 @@ -[See this exception on Errbit](<%= app_err_url err.app, err %> "See this exception on Errbit") -<% if notice = err.notices.first %> -# <%= notice.message %> # -## Summary ## -<% if notice.request['url'].present? %> - ### URL ### - [<%= notice.request['url'] %>](<%= notice.request['url'] %>)" -<% end %> -### Where ### -<%= notice.err.where %> - -### Occured ### -<%= notice.created_at.to_s(:micro) %> - -### Similar ### -<%= (notice.err.notices_count - 1).to_s %> - -## Params ## -``` -<%= pretty_hash(notice.params) %> -``` - -## Session ## -``` -<%= pretty_hash(notice.session) %> -``` - -## Backtrace ## -``` -<% for line in notice.backtrace %><%= line['number'] %>: <%= line['file'].sub(/^\[PROJECT_ROOT\]/, '') %> -> **<%= line['method'] %>** -<% end %> -``` - -## Environment ## - - -<% for key, val in notice.env_vars %> - - - - -<% end %> -
<%= key %>:<%= val %>
-<% end %> - diff --git a/app/views/issue_trackers/github_issues_body.txt.erb b/app/views/issue_trackers/github_issues_body.txt.erb new file mode 100644 index 0000000..8b3297f --- /dev/null +++ b/app/views/issue_trackers/github_issues_body.txt.erb @@ -0,0 +1,45 @@ +[See this exception on Errbit](<%= app_err_url err.app, err %> "See this exception on Errbit") +<% if notice = err.notices.first %> +# <%= notice.message %> # +## Summary ## +<% if notice.request['url'].present? %> + ### URL ### + [<%= notice.request['url'] %>](<%= notice.request['url'] %>)" +<% end %> +### Where ### +<%= notice.err.where %> + +### Occured ### +<%= notice.created_at.to_s(:micro) %> + +### Similar ### +<%= (notice.err.notices_count - 1).to_s %> + +## Params ## +``` +<%= pretty_hash(notice.params) %> +``` + +## Session ## +``` +<%= pretty_hash(notice.session) %> +``` + +## Backtrace ## +``` +<% for line in notice.backtrace %><%= line['number'] %>: <%= line['file'].sub(/^\[PROJECT_ROOT\]/, '') %> -> **<%= line['method'] %>** +<% end %> +``` + +## Environment ## + + +<% for key, val in notice.env_vars %> + + + + +<% end %> +
<%= key %>:<%= val %>
+<% end %> + diff --git a/spec/controllers/apps_controller_spec.rb b/spec/controllers/apps_controller_spec.rb index b1bb6fa..95c66ed 100644 --- a/spec/controllers/apps_controller_spec.rb +++ b/spec/controllers/apps_controller_spec.rb @@ -276,7 +276,7 @@ describe AppsController do IssueTracker.subclasses.each do |tracker_klass| context tracker_klass do it "should save tracker params" do - params = tracker_klass::RequiredFields.inject({}){|hash,f| hash[f] = "test_value"; hash } + params = tracker_klass::Fields.inject({}){|hash,f| hash[f[0]] = "test_value"; hash } params['ticket_properties'] = "card_type = defect" if tracker_klass == MingleTracker params['type'] = tracker_klass.to_s put :update, :id => @app.id, :app => {:issue_tracker_attributes => params} @@ -284,14 +284,17 @@ describe AppsController do @app.reload tracker = @app.issue_tracker tracker.should be_a(tracker_klass) - tracker_klass::RequiredFields.each do |required| - tracker.send(required.to_sym).should == 'test_value' + tracker_klass::Fields.each do |field, field_info| + case field + when :ticket_properties; tracker.send(field.to_sym).should == 'card_type = defect' + else tracker.send(field.to_sym).should == 'test_value' + end end end it "should show validation notice when sufficient params are not present" do # Leave out one required param - params = tracker_klass::RequiredFields[1..-1].inject({}){|hash,f| hash[f] = "test_value"; hash } + params = tracker_klass::Fields[1..-1].inject({}){|hash,f| hash[f[0]] = "test_value"; hash } params['type'] = tracker_klass.to_s put :update, :id => @app.id, :app => {:issue_tracker_attributes => params} diff --git a/spec/factories/issue_tracker_factories.rb b/spec/factories/issue_tracker_factories.rb index 928f0a3..d3c269e 100644 --- a/spec/factories/issue_tracker_factories.rb +++ b/spec/factories/issue_tracker_factories.rb @@ -20,7 +20,7 @@ Factory.define :mingle_tracker, :parent => :issue_tracker, :class => :mingle_tra e.ticket_properties 'card_type = Defect, defect_status = open, priority = essential' end -Factory.define :github_tracker, :parent => :issue_tracker, :class => :github_tracker do |e| +Factory.define :github_issues_tracker, :parent => :issue_tracker, :class => :github_issues_tracker do |e| e.project_id 'test_account/test_project' e.username 'test_username' end diff --git a/spec/models/issue_trackers/github_issues_tracker_spec.rb b/spec/models/issue_trackers/github_issues_tracker_spec.rb new file mode 100644 index 0000000..2e13e74 --- /dev/null +++ b/spec/models/issue_trackers/github_issues_tracker_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +describe GithubIssuesTracker do + it "should create an issue on Github Issues with err params, and set issue link for err" do + notice = Factory :notice + tracker = Factory :github_issues_tracker, :app => notice.err.app + err = notice.err + + number = 5 + @issue_link = "https://github.com/#{tracker.project_id}/issues/#{number}" + body = < 201, :headers => {'Location' => @issue_link}, :body => body ) + + err.app.issue_tracker.create_issue(err) + err.reload + + requested = have_requested(:post, "https://#{tracker.username}%2Ftoken:#{tracker.api_token}@github.com/api/v2/json/issues/open/#{tracker.project_id}") + WebMock.should requested.with(:headers => {'Content-Type' => 'application/x-www-form-urlencoded'}) + WebMock.should requested.with(:body => /title=%5Bproduction%5D%5Bfoo%23bar%5D%20FooError%3A%20Too%20Much%20Bar/) + WebMock.should requested.with(:body => /See%20this%20exception%20on%20Errbit/) + + err.issue_link.should == @issue_link + end +end + diff --git a/spec/models/issue_trackers/github_tracker_spec.rb b/spec/models/issue_trackers/github_tracker_spec.rb deleted file mode 100644 index a14508c..0000000 --- a/spec/models/issue_trackers/github_tracker_spec.rb +++ /dev/null @@ -1,41 +0,0 @@ -require 'spec_helper' - -describe GithubTracker do - it "should create an issue on Github Issues with err params, and set issue link for err" do - notice = Factory :notice - tracker = Factory :github_tracker, :app => notice.err.app - err = notice.err - - number = 5 - @issue_link = "https://github.com/#{tracker.project_id}/issues/#{number}" - body = < 201, :headers => {'Location' => @issue_link}, :body => body ) - - err.app.issue_tracker.create_issue(err) - err.reload - - requested = have_requested(:post, "https://#{tracker.username}%2Ftoken:#{tracker.api_token}@github.com/api/v2/json/issues/open/#{tracker.project_id}") - WebMock.should requested.with(:headers => {'Content-Type' => 'application/x-www-form-urlencoded'}) - WebMock.should requested.with(:body => /title=%5Bproduction%5D%5Bfoo%23bar%5D%20FooError%3A%20Too%20Much%20Bar/) - WebMock.should requested.with(:body => /See%20this%20exception%20on%20Errbit/) - - err.issue_link.should == @issue_link - end -end - -- libgit2 0.21.2