diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 6b9cffc92..bb4a0ee90 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -40,6 +40,7 @@ Metrics/AbcSize: # Configuration parameters: CountComments, Max, CountAsOne. Metrics/ClassLength: Exclude: + - 'app/controllers/admin/chapters_controller.rb' - 'app/controllers/admin/workshops_controller.rb' - 'app/controllers/application_controller.rb' - 'app/controllers/events_controller.rb' diff --git a/app/controllers/admin/chapters_controller.rb b/app/controllers/admin/chapters_controller.rb index 06d0f8127..4c472ae29 100644 --- a/app/controllers/admin/chapters_controller.rb +++ b/app/controllers/admin/chapters_controller.rb @@ -9,9 +9,12 @@ def new def create @chapter = Chapter.new(chapter_params) - authorize(@chapter) + authorize @chapter + + result = ChapterCreationService.call(chapter_params) + @chapter = result.chapter - if @chapter.save + if result.success flash[:notice] = "Chapter #{@chapter.name} has been successfully created" redirect_to [:admin, @chapter] else diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 3abf00058..cf9820418 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -1,24 +1,6 @@ class Admin::GroupsController < Admin::ApplicationController after_action :verify_authorized - def new - @group = Group.new - authorize @group - end - - def create - @group = Group.new(group_params) - authorize @group - - if @group.save - flash[:notice] = "Group #{@group.name} for chapter #{@group.chapter.name} has been successfully created" - redirect_to [:admin, @group] - else - flash[:notice] = @group.errors.full_messages - render 'new' - end - end - def show @group = Group.find(params[:id]) authorize @group @@ -27,10 +9,4 @@ def show @total_count = @group.members.count @pagy, @members = pagy(Group.members_by_recent_rsvp(@group), items: 20) end - - private - - def group_params - params.expect(group: %i[name description chapter_id]) - end end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 5e3d40e70..984476220 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -1,8 +1,4 @@ class GroupPolicy < ApplicationPolicy - def create? - user.is_admin? - end - def show? admin_or_chapter_organiser? end diff --git a/app/services/chapter_creation_service.rb b/app/services/chapter_creation_service.rb new file mode 100644 index 000000000..e4bafdde5 --- /dev/null +++ b/app/services/chapter_creation_service.rb @@ -0,0 +1,17 @@ +class ChapterCreationService + Result = Data.define(:chapter, :success, :errors) + + def self.call(params) + chapter = Chapter.new(params) + + ActiveRecord::Base.transaction do + chapter.save! + chapter.groups.create!(name: 'Students') + chapter.groups.create!(name: 'Coaches') + end + + Result.new(chapter: chapter, success: true, errors: nil) + rescue ActiveRecord::RecordInvalid => e + Result.new(chapter: chapter, success: false, errors: e.message) + end +end diff --git a/app/views/admin/groups/new.html.haml b/app/views/admin/groups/new.html.haml deleted file mode 100644 index 1866a536d..000000000 --- a/app/views/admin/groups/new.html.haml +++ /dev/null @@ -1,13 +0,0 @@ -.container.py-4.py-lg-5 - .row.mb-4 - .col - %h1 New Group - - .row - .col.col-lg-8 - = simple_form_for [:admin, @group] do |f| - = f.input :name, label: 'Name', collection: Group::NAMES - = f.input :description, input_html: { rows: 3 } - = f.association :chapter, required: true - .text-right - = f.button :button, 'Create group', class: 'btn btn-primary' diff --git a/app/views/layouts/_admin_menu.html.haml b/app/views/layouts/_admin_menu.html.haml index d457a637b..67e469646 100644 --- a/app/views/layouts/_admin_menu.html.haml +++ b/app/views/layouts/_admin_menu.html.haml @@ -24,9 +24,6 @@ %li = link_to new_admin_chapter_path, class: 'dropdown-item' do New chapter -%li - = link_to new_admin_group_path, class: 'dropdown-item' do - New group %li = link_to admin_testimonials_path, class: 'dropdown-item' do Testimonials diff --git a/config/routes.rb b/config/routes.rb index ebcd1a730..41f128025 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -133,7 +133,7 @@ end resources :meeting_invitations, only: %i[create update] - resources :groups, only: %i[index new create show] + resources :groups, only: %i[show] resources :sponsors, except: [:destroy] resources :feedback, only: [:index] resources :contacts diff --git a/spec/controllers/admin/chapters_controller_spec.rb b/spec/controllers/admin/chapters_controller_spec.rb index 01bdea3a3..ea641430f 100644 --- a/spec/controllers/admin/chapters_controller_spec.rb +++ b/spec/controllers/admin/chapters_controller_spec.rb @@ -5,6 +5,29 @@ login_as_admin(admin) end + describe '#create' do + context 'when a chapter-scoped organiser creates a chapter' do + let(:organiser) { Fabricate(:chapter_organiser) } + + before do + login(organiser) + end + + it 'is denied before anything is persisted' do + expect do + post :create, params: { chapter: { + name: 'codebar Brighton', + email: 'brighton@codebar.io', + city: 'Brighton', + time_zone: 'London' + } } + end.not_to(change { [Chapter.count, Group.count] }) + + expect(response).to redirect_to(root_path) + end + end + end + describe '#status' do it 'renders successfully with default 6 months' do get :status diff --git a/spec/features/admin/chapters_spec.rb b/spec/features/admin/chapters_spec.rb index 4f6dac954..0335024c5 100644 --- a/spec/features/admin/chapters_spec.rb +++ b/spec/features/admin/chapters_spec.rb @@ -27,6 +27,23 @@ click_on 'Create chapter' expect(page).to have_text('Chapter codebar Brighton has been successfully created') + + chapter = Chapter.find_by(name: 'codebar Brighton') + expect(chapter.groups.pluck(:name)).to match_array(%w[Students Coaches]) + end + + scenario 'an admin submitting an invalid form sees validation errors and no chapter is created' do + visit new_admin_chapter_path + + fill_in 'Name', with: '' + fill_in 'Email', with: '' + fill_in 'City', with: 'Brighton' + + click_on 'Create chapter' + + expect(page).to have_text("Name can't be blank") + expect(page).to have_text("Email can't be blank") + expect(Chapter.count).to eq(0) end end diff --git a/spec/features/admin/groups_spec.rb b/spec/features/admin/groups_spec.rb index 8785ff2a4..769d6c5ef 100644 --- a/spec/features/admin/groups_spec.rb +++ b/spec/features/admin/groups_spec.rb @@ -1,23 +1,4 @@ RSpec.feature 'admin groups', type: :feature do - describe '#creating a new group' do - let(:member) { Fabricate(:member) } - - before do - Fabricate(:chapter, name: 'Brighton') - login_as_admin(member) - end - - scenario 'an admin can create a new chapter' do - visit new_admin_group_path - - select 'Students', from: 'group[name]' - select 'Brighton', from: 'group[chapter_id]' - click_on 'Create group' - - expect(page).to have_text('Group Students for chapter Brighton has been successfully created') - end - end - describe '#show page' do let(:member) { Fabricate(:member) } let(:chapter) { Fabricate(:chapter, name: 'Brighton') } diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 77b54223e..658a5280e 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -5,24 +5,6 @@ let(:admin) { Fabricate(:member).tap { |m| m.add_role(:admin) } } let(:regular_member) { Fabricate(:member) } - describe '#create?' do - context 'when user is admin' do - let(:user) { admin } - - it 'permits access' do - expect(policy.create?).to be true - end - end - - context 'when user is regular member' do - let(:user) { regular_member } - - it 'denies access' do - expect(policy.create?).to be false - end - end - end - describe '#show?' do context 'when user is admin' do let(:user) { admin } diff --git a/spec/services/chapter_creation_service_spec.rb b/spec/services/chapter_creation_service_spec.rb new file mode 100644 index 000000000..c9a56ab31 --- /dev/null +++ b/spec/services/chapter_creation_service_spec.rb @@ -0,0 +1,43 @@ +RSpec.describe ChapterCreationService do + let(:valid_params) do + { + name: 'codebar Brighton', + email: 'brighton@codebar.io', + city: 'Brighton', + time_zone: 'London' + } + end + + describe '.call' do + it 'creates chapter with Students and Coaches groups' do + result = described_class.call(valid_params) + + expect(result.success).to be true + expect(result.chapter.persisted?).to be true + expect(result.chapter.groups.pluck(:name)).to match_array(%w[Students Coaches]) + end + + it 'fails when chapter params are invalid' do + invalid_params = valid_params.merge(name: '') + result = described_class.call(invalid_params) + + expect(result.success).to be false + expect(result.errors).to be_present + expect(Chapter.where(name: '')).not_to exist + end + + it 'rolls back chapter if groups fail' do + # 'bogus' is not in Group::NAMES, so the group fails its real + # inclusion validation and save! raises RecordInvalid inside the + # service's transaction. + invalid_group = Group.new(name: 'bogus') + allow(Group).to receive(:new).and_return(invalid_group) + + result = described_class.call(valid_params) + + expect(result.success).to be false + expect(result.errors).to be_present + expect(Chapter.where(name: valid_params[:name])).not_to exist + end + end +end