From a3dbf209ad7779d5b1d121b48a392a9cc8c82aaa Mon Sep 17 00:00:00 2001 From: Cyril Mougel Date: Fri, 25 Oct 2013 18:25:16 +0200 Subject: [PATCH] Simplify AppsController --- Gemfile | 7 ++++--- Gemfile.lock | 8 ++++++++ app/controllers/apps_controller.rb | 11 ----------- app/decorators/issue_tracker_decorator.rb | 4 ++-- app/models/issue_tracker.rb | 4 +--- app/models/issue_trackers/none.rb | 5 +++++ app/views/apps/edit.html.haml | 8 ++++---- app/views/apps/new.html.haml | 6 +++--- config/locales/en.yml | 10 ++++++++++ spec/acceptance/acceptance_helper.rb | 3 +++ spec/acceptance/app_regenerate_api_key_spec.rb | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- spec/controllers/apps_controller_spec.rb | 4 ++-- spec/spec_helper.rb | 2 +- 13 files changed, 110 insertions(+), 31 deletions(-) create mode 100644 app/models/issue_trackers/none.rb diff --git a/Gemfile b/Gemfile index e080b11..1dcda45 100644 --- a/Gemfile +++ b/Gemfile @@ -103,9 +103,10 @@ group :development do end group :test do - gem 'capybara' - gem 'launchy' - gem 'database_cleaner' + gem 'capybara', :require => false + gem 'poltergeist', :require => false + gem 'launchy', :require => false + gem 'database_cleaner', :require => false gem 'email_spec' gem 'timecop' gem 'coveralls', :require => false diff --git a/Gemfile.lock b/Gemfile.lock index 8d6bcf3..ac35328 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -66,6 +66,7 @@ GEM rack (>= 1.0.0) rack-test (>= 0.5.4) xpath (~> 2.0) + cliver (0.2.2) coderay (1.0.9) columnize (0.3.6) coveralls (0.7.0) @@ -239,6 +240,11 @@ GEM rest-client (~> 1.6.0) pjax_rails (0.3.4) jquery-rails + poltergeist (1.4.1) + capybara (~> 2.1.0) + cliver (~> 0.2.1) + multi_json (~> 1.0) + websocket-driver (>= 0.2.0) polyglot (0.3.3) premailer (1.7.3) css_parser (>= 1.1.9) @@ -363,6 +369,7 @@ GEM webmock (1.15.0) addressable (>= 2.2.7) crack (>= 0.3.2) + websocket-driver (0.3.0) xmpp4r (0.5.5) xpath (2.0.0) nokogiri (~> 1.3) @@ -414,6 +421,7 @@ DEPENDENCIES oruen_redmine_client pivotal-tracker pjax_rails + poltergeist pry-rails puma quiet_assets diff --git a/app/controllers/apps_controller.rb b/app/controllers/apps_controller.rb index ec3e2c2..7cebb18 100644 --- a/app/controllers/apps_controller.rb +++ b/app/controllers/apps_controller.rb @@ -49,7 +49,6 @@ class AppsController < ApplicationController end def create - initialize_subclassed_issue_tracker initialize_subclassed_notification_service if app.save redirect_to app_url(app), :flash => { :success => I18n.t('controllers.apps.flash.create.success') } @@ -60,7 +59,6 @@ class AppsController < ApplicationController end def update - initialize_subclassed_issue_tracker initialize_subclassed_notification_service if app.save redirect_to app_url(app), :flash => { :success => I18n.t('controllers.apps.flash.update.success') } @@ -90,15 +88,6 @@ class AppsController < ApplicationController protected - def initialize_subclassed_issue_tracker - # set the app's issue tracker - if params[:app][:issue_tracker_attributes] && tracker_type = params[:app][:issue_tracker_attributes][:type] - if IssueTracker.subclasses.map(&:name).concat(["IssueTracker"]).include?(tracker_type) - app.issue_tracker = tracker_type.constantize.new(params[:app][:issue_tracker_attributes]) - end - end - end - def initialize_subclassed_notification_service # set the app's notification service if params[:app][:notification_service_attributes] && notification_type = params[:app][:notification_service_attributes][:type] diff --git a/app/decorators/issue_tracker_decorator.rb b/app/decorators/issue_tracker_decorator.rb index 86dd985..4abea5c 100644 --- a/app/decorators/issue_tracker_decorator.rb +++ b/app/decorators/issue_tracker_decorator.rb @@ -4,8 +4,8 @@ class IssueTrackerDecorator < Draper::Decorator def issue_trackers @issue_trackers ||= [ - IssueTracker, - IssueTracker.subclasses + IssueTracker::None, + IssueTracker.subclasses.select{|klass| klass != IssueTracker::None } ].flatten @issue_trackers.each do |it| yield IssueTrackerDecorator.new(it) diff --git a/app/models/issue_tracker.rb b/app/models/issue_tracker.rb index 78179d3..5cf412a 100644 --- a/app/models/issue_tracker.rb +++ b/app/models/issue_tracker.rb @@ -4,9 +4,7 @@ class IssueTracker include HashHelper include Rails.application.routes.url_helpers - Fields = [] - Note = 'When no issue tracker has been configured, you will be able to leave comments on errors.' - Label = 'none' + Note = '' default_url_options[:host] = ActionMailer::Base.default_url_options[:host] diff --git a/app/models/issue_trackers/none.rb b/app/models/issue_trackers/none.rb new file mode 100644 index 0000000..43e09d9 --- /dev/null +++ b/app/models/issue_trackers/none.rb @@ -0,0 +1,5 @@ +class IssueTrackers::None < IssueTracker + Fields = [] + Note = 'When no issue tracker has been configured, you will be able to leave comments on errors.' + Label = 'none' +end diff --git a/app/views/apps/edit.html.haml b/app/views/apps/edit.html.haml index d0d5729..6979c25 100644 --- a/app/views/apps/edit.html.haml +++ b/app/views/apps/edit.html.haml @@ -1,13 +1,13 @@ -- content_for :title, 'Edit App' +- content_for :title, t('.title') - content_for :action_bar do = link_to_copy_attributes_from_other_app - = link_to 'delete application', app_path(app), :method => :delete, + = link_to t('.destroy'), app_path(app), :method => :delete, :data => { :confirm => t('apps.confirm_delete') }, :class => 'button' - = link_to('cancel', app_path(app), :class => 'button') + = link_to(t('.cancel'), app_path(app), :class => 'button') = form_for app_decorate do |f| = render 'fields', :f => f - %div.buttons= f.submit 'Update App' + %div.buttons= f.submit t('.update') diff --git a/app/views/apps/new.html.haml b/app/views/apps/new.html.haml index 36e90fd..a825f5b 100644 --- a/app/views/apps/new.html.haml +++ b/app/views/apps/new.html.haml @@ -1,11 +1,11 @@ -- content_for :title, 'Add App' +- content_for :title, t('.title') - content_for :action_bar do = link_to_copy_attributes_from_other_app - = link_to('cancel', apps_path, :class => 'button') + = link_to(t('.cancel'), apps_path, :class => 'button') = form_for app_decorate do |f| = render 'fields', :f => f - %div.buttons= f.submit 'Add App' + %div.buttons= f.submit t('.add_app') diff --git a/config/locales/en.yml b/config/locales/en.yml index 2ccec9d..8bf6d6a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -127,4 +127,14 @@ en: who: Who issue_tracker_fields: legend: Issue Tracker + new: + add_app: "Add App" + cancel: "cancel" + title: "Add App" + edit: + title: 'Edit App' + destroy: 'destroy application' + cancel: 'cancel' + seriously: 'Seriously?' + update: 'Update App' diff --git a/spec/acceptance/acceptance_helper.rb b/spec/acceptance/acceptance_helper.rb index 2255870..1d8c1c5 100644 --- a/spec/acceptance/acceptance_helper.rb +++ b/spec/acceptance/acceptance_helper.rb @@ -1,5 +1,8 @@ require 'spec_helper' require 'capybara/rspec' +require 'capybara/poltergeist' + +Capybara.javascript_driver = :poltergeist OmniAuth.config.test_mode = true diff --git a/spec/acceptance/app_regenerate_api_key_spec.rb b/spec/acceptance/app_regenerate_api_key_spec.rb index a0d20de..f82bcb1 100644 --- a/spec/acceptance/app_regenerate_api_key_spec.rb +++ b/spec/acceptance/app_regenerate_api_key_spec.rb @@ -1,12 +1,16 @@ require 'acceptance/acceptance_helper' feature "Regeneration api_Key" do - let!(:app) { Fabricate(:app) } - let!(:admin) { Fabricate(:admin) } + let(:app) { Fabricate(:app) } + let(:admin) { Fabricate(:admin) } let(:user) { Fabricate(:user_watcher, :app => app).user } + before do + app && admin + end + scenario "an admin change api_key" do visit '/' log_in admin @@ -29,3 +33,64 @@ feature "Regeneration api_Key" do expect(page).to_not have_button I18n.t('apps.show.edit') end end + +feature "Create an application" do + + let(:admin) { Fabricate(:admin) } + let(:user) { + Fabricate(:user_watcher, :app => app).user + } + + before do + admin + end + + scenario "create an apps without issue tracker and edit it" do + visit '/' + log_in admin + click_on I18n.t('apps.index.new_app') + fill_in 'app_name', :with => 'My new app' + click_on I18n.t('apps.new.add_app') + page.has_content?(I18n.t('controllers.apps.flash.create.success')) + expect(App.where(:name => 'My new app').count).to eql 1 + expect(App.where(:name => 'My new app 2').count).to eql 0 + + + click_on I18n.t('shared.navigation.apps') + click_on 'My new app' + click_link I18n.t('apps.show.edit') + fill_in 'app_name', :with => 'My new app 2' + click_on I18n.t('apps.edit.update') + page.has_content?(I18n.t('controllers.apps.flash.update.success')) + expect(App.where(:name => 'My new app').count).to eql 0 + expect(App.where(:name => 'My new app 2').count).to eql 1 + + end + + scenario "create an apps with issue tracker and edit it", :js => true do + visit '/' + log_in admin + click_on I18n.t('apps.index.new_app') + fill_in 'app_name', :with => 'My new app' + find('.label_radio.bitbucket').click + within ".bitbucket.tracker_params" do + fill_in 'app_issue_tracker_attributes_api_token', :with => 'token' + fill_in 'app_issue_tracker_attributes_project_id', :with => 'pass' + end + click_on I18n.t('apps.new.add_app') + expect(page.has_content?(I18n.t('controllers.apps.flash.create.success'))).to eql true + app = App.where(:name => 'My new app').first + expect(app.issue_tracker).to be_a IssueTracker::BitbucketIssuesTracker + expect(app.issue_tracker.api_token).to eql 'token' + expect(app.issue_tracker.project_id).to eql 'pass' + + click_on I18n.t('shared.navigation.apps') + click_on 'My new app' + click_link I18n.t('apps.show.edit') + find('.issue_tracker .label_radio.none').click + click_on I18n.t('apps.edit.update') + expect(page.has_content?(I18n.t('controllers.apps.flash.update.success'))).to eql true + app = App.where(:name => 'My new app').first + expect(app.issue_tracker).to be_a IssueTracker::None + end +end diff --git a/spec/controllers/apps_controller_spec.rb b/spec/controllers/apps_controller_spec.rb index e572217..b001a29 100644 --- a/spec/controllers/apps_controller_spec.rb +++ b/spec/controllers/apps_controller_spec.rb @@ -323,8 +323,8 @@ describe AppsController do it "should show validation notice when sufficient params are not present" do # Leave out one required param - params = tracker_klass::Fields[1..-1].inject({}){|hash,f| hash[f[0]] = "test_value"; hash } - params[:type] = tracker_klass.to_s + # TODO. previous test was not relevant because one params can be enough. So put noone + params = {:type => tracker_klass.to_s } put :update, :id => @app.id, :app => {:issue_tracker_attributes => params} @app.reload diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5ab660e..47080d8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -55,7 +55,7 @@ RSpec.configure do |config| end config.after(:all) do - WebMock.disable_net_connect! :allow => /coveralls\.io/ + WebMock.disable_net_connect! :allow => /coveralls\.io|127\.0\.0\.1/ end end -- libgit2 0.21.2