From 8c477e5860108667bb2a89fbb4bc4e16e397f279 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 5 Mar 2018 07:40:01 +0100 Subject: [PATCH] Fix more project settings related specs --- app/assets/javascripts/members_form.js | 2 ++ app/controllers/groups_controller.rb | 11 ++++++- app/controllers/repositories_controller.rb | 12 +++---- .../users/memberships_controller.rb | 14 +++++--- app/controllers/watchers_controller.rb | 2 +- app/helpers/projects_helper.rb | 16 +++++----- .../members/edit_membership_service.rb | 3 +- app/views/members/_member_errors.html.erb | 30 ----------------- .../project_settings/_custom_fields.html.erb | 2 +- app/views/project_settings/_types.html.erb | 2 +- config/initializers/secure_headers.rb | 2 +- spec/controllers/messages_controller_spec.rb | 14 ++++++++ .../repositories_controller_spec.rb | 32 ++++--------------- .../users/memberships_controller_spec.rb | 5 ++- spec/features/users/user_memberships_spec.rb | 2 +- spec/models/users/allowed_to_spec.rb | 6 ++-- spec/routing/repositories_routing_spec.rb | 9 ------ spec/views/projects/settings.html.erb_spec.rb | 3 +- .../functional/messages_controller_spec.rb | 7 ---- .../functional/projects_controller_spec.rb | 7 ---- .../unit/lib/redmine/access_control_spec.rb | 2 +- 21 files changed, 71 insertions(+), 112 deletions(-) delete mode 100644 app/views/members/_member_errors.html.erb diff --git a/app/assets/javascripts/members_form.js b/app/assets/javascripts/members_form.js index 1dd54a4540d..0d8096002a8 100644 --- a/app/assets/javascripts/members_form.js +++ b/app/assets/javascripts/members_form.js @@ -77,6 +77,8 @@ jQuery(document).ready(function($) { // Toggle filter $('.toggle-member-filter-link').click(toggleMemberFilter); + + // Toggle editing row $('.toggle-membership-button').click(function() { var el = $(this); diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 423b9df0733..a4943118c2d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -122,6 +122,7 @@ class GroupsController < ApplicationController @users = User.includes(:memberships).where(id: params[:user_ids]) @group.users << @users + I18n.t :notice_successful_update redirect_to controller: '/groups', action: 'edit', id: @group, tab: 'users' end @@ -129,6 +130,7 @@ class GroupsController < ApplicationController @group = Group.includes(:users).find(params[:id]) @group.users.delete(User.includes(:memberships).find(params[:user_id])) + I18n.t :notice_successful_update redirect_to controller: '/groups', action: 'edit', id: @group, tab: 'users' end @@ -143,8 +145,13 @@ class GroupsController < ApplicationController @membership = membership_id.present? ? Member.find(membership_id) : Member.new(principal: @group) service = ::Members::EditMembershipService.new(@membership, save: true, current_user: current_user) - service.call(attributes: membership_params[:membership]) + result = service.call(attributes: membership_params[:membership]) + if result.success? + flash[:notice] = I18n.t :notice_successful_update + else + flash[:error] = reusult.errors.full_mesages.join("\n") + end redirect_to controller: '/groups', action: 'edit', id: @group, tab: 'memberships' end @@ -153,6 +160,8 @@ class GroupsController < ApplicationController def destroy_membership membership_params = permitted_params.group_membership Member.find(membership_params[:membership_id]).destroy + + flash[:notice] = I18n.t :notice_successful_delete redirect_to controller: '/groups', action: 'edit', id: @group, tab: 'memberships' end diff --git a/app/controllers/repositories_controller.rb b/app/controllers/repositories_controller.rb index 30381f061ac..4eb63a9ec12 100644 --- a/app/controllers/repositories_controller.rb +++ b/app/controllers/repositories_controller.rb @@ -56,7 +56,7 @@ class RepositoriesController < ApplicationController @repository = @project.repository update_repository(params.fetch(:repository, {})) - redirect_to settings_repository_tab_path + redirect_to helpers.settings_repository_tab_path end def create @@ -69,7 +69,7 @@ class RepositoriesController < ApplicationController flash[:error] = service.build_error end - redirect_to settings_repository_tab_path + redirect_to helpers.settings_repository_tab_path end def committers @@ -93,7 +93,7 @@ class RepositoriesController < ApplicationController def destroy_info @repository = @project.repository - @back_link = settings_repository_tab_path + @back_link = helpers.settings_repository_tab_path end def destroy @@ -103,7 +103,7 @@ class RepositoriesController < ApplicationController else flash[:error] = repository.errors.full_messages end - redirect_to settings_repository_tab_path + redirect_to helpers.settings_repository_tab_path end def show @@ -313,9 +313,9 @@ class RepositoriesController < ApplicationController @repository.attributes = @repository.class.permitted_params(repo_params) if @repository.save - flash.now[:notice] = l('repositories.update_settings_successful') + flash[:notice] = l('repositories.update_settings_successful') else - flash.now[:error] = @repository.errors.full_messages.join('\n') + flash[:error] = @repository.errors.full_messages.join('\n') end end diff --git a/app/controllers/users/memberships_controller.rb b/app/controllers/users/memberships_controller.rb index ca3512e58d8..3c6a926d552 100644 --- a/app/controllers/users/memberships_controller.rb +++ b/app/controllers/users/memberships_controller.rb @@ -35,11 +35,11 @@ class Users::MembershipsController < ApplicationController before_action :find_user def update - update_or_create(request.patch?) + update_or_create(request.patch?, :notice_successful_update) end def create - update_or_create(request.post?) + update_or_create(request.post?, :notice_successful_create) end def destroy @@ -47,6 +47,7 @@ class Users::MembershipsController < ApplicationController if @membership.deletable? && request.delete? @membership.destroy && @membership = nil + flash[:notice] = I18n.t(:notice_successful_delete) end redirect_to controller: '/users', action: 'edit', id: @user, tab: 'memberships' @@ -54,11 +55,16 @@ class Users::MembershipsController < ApplicationController private - def update_or_create(save_record) + def update_or_create(save_record, message) @membership = params[:id].present? ? Member.find(params[:id]) : Member.new(principal: @user) service = ::Members::EditMembershipService.new(@membership, save: save_record, current_user: current_user) - service.call(attributes: permitted_params.membership) + result = service.call(attributes: permitted_params.membership) + if result.success? + flash[:notice] = I18n.t(message) + else + flash[:error] = result.errors.full_messages.join("\n") + end redirect_to controller: '/users', action: 'edit', id: @user, tab: 'memberships' end diff --git a/app/controllers/watchers_controller.rb b/app/controllers/watchers_controller.rb index c811bf77cdd..45c5d6ff820 100644 --- a/app/controllers/watchers_controller.rb +++ b/app/controllers/watchers_controller.rb @@ -64,6 +64,6 @@ class WatchersController < ApplicationController def set_watcher(user, watching) @watched.set_watcher(user, watching) - redirect_back(home_url) + redirect_back(fallback_location: home_url) end end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 6c140c66ddc..5255af387e6 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -58,19 +58,19 @@ module ProjectsHelper { name: 'custom_fields', action: :edit_project, - partial: 'projects_settings/custom_fields', + partial: 'project_settings/custom_fields', label: :label_custom_field_plural }, { name: 'versions', action: :manage_versions, - partial: 'projects_settings/versions', + partial: 'project_settings/versions', label: :label_version_plural }, { name: 'categories', action: :manage_categories, - partial: 'projects_settings/categories', + partial: 'project_settings/categories', label: :label_work_package_category_plural }, { @@ -82,19 +82,19 @@ module ProjectsHelper { name: 'boards', action: :manage_boards, - partial: 'projects_settings/boards', + partial: 'project_settings/boards', label: :label_board_plural }, { name: 'activities', action: :manage_project_activities, - partial: 'projects_settings/activities', + partial: 'project_settings/activities', label: :enumeration_activities }, { name: 'types', action: :manage_types, - partial: 'projects_settings/types', + partial: 'project_settings/types', label: :label_work_package_types } ] @@ -169,9 +169,9 @@ module ProjectsHelper end def project_more_menu_settings_item(project) - if User.current.allowed_to?({ controller: 'projects', action: 'settings' }, project) + if User.current.allowed_to?({ controller: '/project_settings', action: 'show' }, project) [t(:label_project_settings), - { controller: 'projects', action: 'settings', id: project }, + { controller: '/project_settings', action: 'show', id: project }, class: 'icon-context icon-settings', title: t(:label_project_settings)] end diff --git a/app/services/members/edit_membership_service.rb b/app/services/members/edit_membership_service.rb index d275199b28c..6a3422aaa7f 100644 --- a/app/services/members/edit_membership_service.rb +++ b/app/services/members/edit_membership_service.rb @@ -78,7 +78,8 @@ module Members def validate_attributes!(attributes) # We need to check for empty roles here because that _implicitly_ # deletes the membership and causes failures - new_roles_are_empty = attributes[:role_ids].empty? + new_roles = attributes[:role_ids] + new_roles_are_empty = new_roles.nil? || new_roles.empty? if new_roles_are_empty && !has_inherited_roles? member.errors.add :roles, :role_blank diff --git a/app/views/members/_member_errors.html.erb b/app/views/members/_member_errors.html.erb deleted file mode 100644 index 8e2bb079635..00000000000 --- a/app/views/members/_member_errors.html.erb +++ /dev/null @@ -1,30 +0,0 @@ -<%#-- copyright -OpenProject is a project management system. -Copyright (C) 2012-2018 the OpenProject Foundation (OPF) - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License version 3. - -OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -Copyright (C) 2006-2017 Jean-Philippe Lang -Copyright (C) 2010-2013 the ChiliProject Team - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License -as published by the Free Software Foundation; either version 2 -of the License, or (at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU General Public License for more details. - -You should have received a copy of the GNU General Public License -along with this program; if not, write to the Free Software -Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - -See docs/COPYRIGHT.rdoc for more details. - -++#%> - -<%= error_messages_for :member, object: member %> diff --git a/app/views/project_settings/_custom_fields.html.erb b/app/views/project_settings/_custom_fields.html.erb index 27f74407b86..eefd5bda9e9 100644 --- a/app/views/project_settings/_custom_fields.html.erb +++ b/app/views/project_settings/_custom_fields.html.erb @@ -42,7 +42,7 @@ See docs/COPYRIGHT.rdoc for more details. <% if @issue_custom_fields.any? %> <%= labelled_tabular_form_for @project, - url: { action: 'custom_fields', id: @project }, + url: { controller: '/projects', action: 'custom_fields', id: @project }, method: :put, html: { id: 'modules-form' } do |form| %> diff --git a/app/views/project_settings/_types.html.erb b/app/views/project_settings/_types.html.erb index f007b8e40f5..143df558185 100644 --- a/app/views/project_settings/_types.html.erb +++ b/app/views/project_settings/_types.html.erb @@ -29,7 +29,7 @@ See docs/COPYRIGHT.rdoc for more details. <% if Type.all.any? %> <%= form_for @project, - url: { action: 'types', id: @project }, + url: { controller: '/projects', action: 'types', id: @project }, method: :patch, html: {id: 'types-form'} do |f| %> <%= render partial: 'projects/form/types', locals: { f: f, project: @project, withControlls: true } %> diff --git a/config/initializers/secure_headers.rb b/config/initializers/secure_headers.rb index 01a2685827f..4eb3e09da27 100644 --- a/config/initializers/secure_headers.rb +++ b/config/initializers/secure_headers.rb @@ -29,7 +29,7 @@ SecureHeaders::Configuration.default do |config| # Form targets can only be self form_action: %w('self'), # Allow iframe from vimeo (welcome video) - frame_src: %w(https://*.vimeo.com), + frame_src: %w(https://*.vimeo.com 'self'), frame_ancestors: %w('self'), # Allow images from anywhere img_src: %w(* data:), diff --git a/spec/controllers/messages_controller_spec.rb b/spec/controllers/messages_controller_spec.rb index ea50fbe77a6..e8b671d7a5e 100644 --- a/spec/controllers/messages_controller_spec.rb +++ b/spec/controllers/messages_controller_spec.rb @@ -212,4 +212,18 @@ describe MessagesController, type: :controller do let(:preview_params) { { board_id: board.id, id: message.id, message: {} } } end end + + describe 'quote' do + let(:message) { FactoryGirl.create :message, content: 'foo', subject: 'subject', board: board } + + context 'when allowed' do + let(:authorized) { true } + + it 'renders the content as json' do + get :quote, params: { board_id: board.id, id: message.id }, format: :json + expect(response).to be_success + expect(response.body).to eq '{"subject":"RE: subject","content":" wrote:\n\u003e foo\n\n"}' + end + end + end end diff --git a/spec/controllers/repositories_controller_spec.rb b/spec/controllers/repositories_controller_spec.rb index eea94b80595..841c28c641b 100644 --- a/spec/controllers/repositories_controller_spec.rb +++ b/spec/controllers/repositories_controller_spec.rb @@ -68,26 +68,6 @@ describe RepositoriesController, type: :controller do allow(controller).to receive(:authorize).and_return(true) end - shared_examples_for 'successful settings response' do - it 'is successful' do - expect(response).to be_success - end - - it 'renders the template' do - expect(response).to render_template 'repositories/settings/repository_form' - end - end - - context 'with #edit' do - before do - get :edit, - params: { project_id: project.id, scm_vendor: 'subversion' }, - xhr: true - end - - it_behaves_like 'successful settings response' - end - context 'with #destroy' do before do allow(repository).to receive(:destroy).and_return(true) @@ -104,7 +84,9 @@ describe RepositoriesController, type: :controller do put :update, params: { project_id: project.id }, xhr: true end - it_behaves_like 'successful settings response' + it 'redirects to settings' do + expect(response).to redirect_to(controller: '/project_settings', id: project.identifier, action: 'show', tab: 'repository') + end end context 'with #create' do @@ -115,13 +97,11 @@ describe RepositoriesController, type: :controller do scm_vendor: 'subversion', scm_type: 'local', url: 'file:///tmp/repo.svn/' - }, - xhr: true + } end - it 'renders a JS redirect' do - path = "\/projects\/#{project.identifier}/settings\/repository" - expect(response.body).to match(/window\.location = '#{path}'/) + it 'redirects to settings' do + expect(response).to redirect_to(controller: '/project_settings', id: project.identifier, action: 'show', tab: 'repository') end end end diff --git a/spec/controllers/users/memberships_controller_spec.rb b/spec/controllers/users/memberships_controller_spec.rb index 0eca11b2a13..369f87c8413 100644 --- a/spec/controllers/users/memberships_controller_spec.rb +++ b/spec/controllers/users/memberships_controller_spec.rb @@ -48,11 +48,10 @@ describe Users::MembershipsController, type: :controller do project_id: project.id, role_ids: [role.id] } - }, - format: 'js' + } end - expect(response.status).to eql(200) + expect(response).to redirect_to(controller: '/users', action: 'edit', id: user.id, tab: 'memberships') is_member = user.reload.memberships.any? { |m| m.project_id == project.id && m.role_ids.include?(role.id) diff --git a/spec/features/users/user_memberships_spec.rb b/spec/features/users/user_memberships_spec.rb index 2ffd03d4dea..53edf065e38 100644 --- a/spec/features/users/user_memberships_spec.rb +++ b/spec/features/users/user_memberships_spec.rb @@ -58,7 +58,7 @@ feature 'user memberships through user page', type: :feature, js: true do user_page.expect_project(project.name) user_page.edit_roles!(member, %w()) - expect(page).to have_selector('#errorExplanation', 'Please choose at least one role.') + expect(page).to have_selector('.flash.error', text: 'Please choose at least one role.') end context 'when user has an inherited role' do diff --git a/spec/models/users/allowed_to_spec.rb b/spec/models/users/allowed_to_spec.rb index 40e7baf0aef..ebf7e4d0393 100644 --- a/spec/models/users/allowed_to_spec.rb +++ b/spec/models/users/allowed_to_spec.rb @@ -302,7 +302,7 @@ describe User, 'allowed_to?' do w/ requesting a controller and action allowed by multiple permissions w/ the project being public w/ anonymous being allowed the action' do - let(:permission) { { controller: 'projects', action: 'settings' } } + let(:permission) { { controller: '/project_settings', action: 'show' } } before do project.is_public = true @@ -432,7 +432,7 @@ describe User, 'allowed_to?' do end it 'should be true' do - expect(user.allowed_to?({ controller: 'projects', action: 'settings' }, project)) + expect(user.allowed_to?({ controller: 'projects', action: 'show' }, project)) .to be_truthy end end @@ -560,7 +560,7 @@ describe User, 'allowed_to?' do end it 'should be true' do - expect(user.allowed_to?({ controller: 'projects', action: 'settings' }, nil, global: true)) + expect(user.allowed_to?({ controller: '/project_settings', action: 'show' }, nil, global: true)) .to be_truthy end end diff --git a/spec/routing/repositories_routing_spec.rb b/spec/routing/repositories_routing_spec.rb index 113bc9765f9..5616f8b1cdd 100644 --- a/spec/routing/repositories_routing_spec.rb +++ b/spec/routing/repositories_routing_spec.rb @@ -111,15 +111,6 @@ describe RepositoriesController, type: :routing do } end - describe 'edit' do - it { - expect(get('/projects/testproject/repository/edit')) - .to route_to(controller: 'repositories', - action: 'edit', - project_id: 'testproject') - } - end - describe 'create' do it { expect(post('/projects/testproject/repository/')) diff --git a/spec/views/projects/settings.html.erb_spec.rb b/spec/views/projects/settings.html.erb_spec.rb index e1b2cfaf3d3..c6349f04388 100644 --- a/spec/views/projects/settings.html.erb_spec.rb +++ b/spec/views/projects/settings.html.erb_spec.rb @@ -28,7 +28,7 @@ require 'spec_helper' -describe 'projects/settings', type: :view do +describe 'project_settings/show', type: :view do let(:project) { FactoryGirl.build_stubbed(:project) } describe 'project copy permission' do @@ -84,6 +84,7 @@ describe 'projects/settings', type: :view do assign(:project, project) allow(project).to receive(:copy_allowed?).and_return(true) allow(User).to receive(:current).and_return(non_admin) + allow(view).to receive(:render_tabs).and_return('') render end diff --git a/spec_legacy/functional/messages_controller_spec.rb b/spec_legacy/functional/messages_controller_spec.rb index b00809c0971..ce07b568972 100644 --- a/spec_legacy/functional/messages_controller_spec.rb +++ b/spec_legacy/functional/messages_controller_spec.rb @@ -160,11 +160,4 @@ describe MessagesController, type: :controller do assert_redirected_to project_board_path('ecookbook', 1) assert_nil Message.find_by(id: 1) end - - it 'should quote' do - session[:user_id] = 2 - get :quote, params: { board_id: 1, id: 3 }, xhr: true - assert_response :success - assert_template 'quote' - end end diff --git a/spec_legacy/functional/projects_controller_spec.rb b/spec_legacy/functional/projects_controller_spec.rb index 60b0d154c7c..0b4749baf6f 100644 --- a/spec_legacy/functional/projects_controller_spec.rb +++ b/spec_legacy/functional/projects_controller_spec.rb @@ -389,13 +389,6 @@ describe ProjectsController, type: :controller do assert_select 'a', content: /Private child/ end - it 'should settings' do - session[:user_id] = 2 # manager - get :settings, params: { id: 1 } - assert_response :success - assert_template 'settings' - end - it 'should update' do session[:user_id] = 2 # manager put :update, diff --git a/spec_legacy/unit/lib/redmine/access_control_spec.rb b/spec_legacy/unit/lib/redmine/access_control_spec.rb index 0cae16b1112..92586f20584 100644 --- a/spec_legacy/unit/lib/redmine/access_control_spec.rb +++ b/spec_legacy/unit/lib/redmine/access_control_spec.rb @@ -54,6 +54,6 @@ describe Redmine::AccessControl do assert_equal :edit_project, perm.name assert_nil perm.project_module assert perm.actions.is_a?(Array) - assert perm.actions.include?('projects/settings') + assert perm.actions.include?('project_settings/show') end end