From 5d478cbbcbec6ffaeed80fc463b7294bb77195f6 Mon Sep 17 00:00:00 2001
From: Aaron Contreras <aaronlcaq@gmail.com>
Date: Tue, 1 Aug 2023 11:51:34 -0500
Subject: [PATCH 1/3] Fix 403 when no project selectded in global meeting form

Ensure `authorize_global` is the hook used for the create action
instead of `authorize`. By doing this, we ensure that based on
`@project` being set or not, we authorize appropriately and allow
the failure to save branch a chance of executing.
---
 .../app/controllers/meetings_controller.rb    |  4 +-
 .../controllers/meetings_controller_spec.rb   | 81 ++++++++++++++-----
 .../spec/features/meetings_new_spec.rb        | 35 ++++++++
 3 files changed, 96 insertions(+), 24 deletions(-)

diff --git a/modules/meeting/app/controllers/meetings_controller.rb b/modules/meeting/app/controllers/meetings_controller.rb
index 57c6481959c..754dc5ce55e 100644
--- a/modules/meeting/app/controllers/meetings_controller.rb
+++ b/modules/meeting/app/controllers/meetings_controller.rb
@@ -32,8 +32,8 @@ class MeetingsController < ApplicationController
   before_action :build_meeting, only: %i[new create]
   before_action :find_meeting, except: %i[index new create]
   before_action :convert_params, only: %i[create update]
-  before_action :authorize, except: %i[index new]
-  before_action :authorize_global, only: %i[index new]
+  before_action :authorize, except: %i[index new create]
+  before_action :authorize_global, only: %i[index new create]
 
   helper :watchers
   helper :meeting_contents
diff --git a/modules/meeting/spec/controllers/meetings_controller_spec.rb b/modules/meeting/spec/controllers/meetings_controller_spec.rb
index 7a0528ded4b..93f9c15e731 100644
--- a/modules/meeting/spec/controllers/meetings_controller_spec.rb
+++ b/modules/meeting/spec/controllers/meetings_controller_spec.rb
@@ -135,53 +135,90 @@ RSpec.describe MeetingsController do
         it { expect(assigns(:meeting)).to eql meeting }
       end
     end
+  end
 
+  describe 'POST' do
     describe 'create' do
       render_views
 
+      let(:base_params) do
+        {
+          project_id: project&.id,
+          meeting: meeting_params
+        }
+      end
+
+      let(:base_meeting_params) do
+        {
+          title: 'Foobar',
+          duration: '1.0',
+          start_date: '2015-06-01',
+          start_time_hour: '10:00'
+        }
+      end
+
+      let(:params) { base_params }
+      let(:meeting_params) { base_meeting_params }
+
       before do
         allow(Project).to receive(:find).and_return(project)
+
         post :create,
-             params: {
-               project_id: project.id,
-               meeting: {
-                 title: 'Foobar',
-                 duration: '1.0'
-               }.merge(params)
-             }
+             params:
       end
 
-      describe 'invalid start_date' do
-        let(:params) do
-          {
-            start_date: '-',
-            start_time_hour: '10:00'
-          }
+      context 'with a project_id' do
+        context 'and an invalid start_date with start_time_hour' do
+          let(:meeting_params) do
+            base_meeting_params.merge(start_date: '-')
+          end
+
+          it 'renders an error' do
+            expect(response).to have_http_status :ok
+            expect(response).to render_template :new
+            expect(response.body)
+              .to have_selector '#errorExplanation li',
+                                text: "Start date #{I18n.t('activerecord.errors.messages.not_an_iso_date')}"
+          end
         end
 
+        context 'and an invalid start_time_hour with start_date' do
+          let(:meeting_params) do
+            base_meeting_params.merge(start_time_hour: '-')
+          end
+
+          it 'renders an error' do
+            expect(response).to have_http_status :ok
+            expect(response).to render_template :new
+            expect(response.body)
+              .to have_selector '#errorExplanation li',
+                                text: "Starting time #{I18n.t('activerecord.errors.messages.invalid_time_format')}"
+          end
+        end
+      end
+
+      context 'with a nil project_id' do
+        let(:project) { nil }
+
         it 'renders an error' do
           expect(response).to have_http_status :ok
           expect(response).to render_template :new
           expect(response.body)
             .to have_selector '#errorExplanation li',
-                              text: "Start date #{I18n.t('activerecord.errors.messages.not_an_iso_date')}"
+                              text: "Project #{I18n.t('activerecord.errors.messages.blank')}"
         end
       end
 
-      describe 'invalid start_time_hour' do
-        let(:params) do
-          {
-            start_date: '2015-06-01',
-            start_time_hour: '-'
-          }
-        end
+      context 'without a project_id' do
+        let(:params) { base_params.except(:project_id) }
+        let(:project) { nil }
 
         it 'renders an error' do
           expect(response).to have_http_status :ok
           expect(response).to render_template :new
           expect(response.body)
             .to have_selector '#errorExplanation li',
-                              text: "Starting time #{I18n.t('activerecord.errors.messages.invalid_time_format')}"
+                              text: "Project #{I18n.t('activerecord.errors.messages.blank')}"
         end
       end
     end
diff --git a/modules/meeting/spec/features/meetings_new_spec.rb b/modules/meeting/spec/features/meetings_new_spec.rb
index 44ef3008e99..c6a4d2226ee 100644
--- a/modules/meeting/spec/features/meetings_new_spec.rb
+++ b/modules/meeting/spec/features/meetings_new_spec.rb
@@ -64,6 +64,7 @@ RSpec.describe 'Meetings new', :js do
     end
 
     let(:index_page) { Pages::Meetings::Index.new(project: nil) }
+    let(:new_page) { Pages::Meetings::New.new(nil) }
 
     context 'with permission to create meetings' do
       it 'does not render menus' do
@@ -95,6 +96,40 @@ RSpec.describe 'Meetings new', :js do
           show_page.expect_date_time "03/28/2013 01:30 PM - 03:00 PM"
         end
       end
+
+      context 'without a title set', :with_cuprite do
+        before do
+          new_page.visit!
+
+          # Wait for project dropdown to be initialized
+          expect_angular_frontend_initialized
+
+          new_page.set_project project
+        end
+
+        it 'renders a validation error' do
+          expect do
+            new_page.click_create
+          end.not_to change(Query, :count)
+
+          # HTML required attribute validation error
+          expect(page).to have_current_path(new_page.path)
+        end
+      end
+
+      context 'without a project set', :with_cuprite do
+        before do
+          new_page.visit!
+          new_page.set_title 'Some title'
+        end
+
+        it 'renders a validation error' do
+          new_page.click_create
+
+          new_page.expect_toast(message: "#{Project.model_name.human} #{I18n.t('activerecord.errors.messages.blank')}",
+                                type: :error)
+        end
+      end
     end
 
     context 'without permission to create meetings' do

From c401daa022b5b95ac57e8bfd2f4c7f87187be8bf Mon Sep 17 00:00:00 2001
From: Aaron Contreras <aaronlcaq@gmail.com>
Date: Tue, 1 Aug 2023 12:30:36 -0500
Subject: [PATCH 2/3] Optimize `meetings_new_spec` by visiting `new` page
 directly

We don't really need to go via the index page every time. As long
as we can ensure that clicking on the "New meeting" button from it
routes properly in both the global and project-specific context, we
can just visit the new page directly for every other spec since
that's the real feature under test.
---
 .../spec/features/meetings_new_spec.rb        | 215 +++++++++++++-----
 .../spec/support/pages/meetings/new.rb        |   6 +-
 2 files changed, 157 insertions(+), 64 deletions(-)

diff --git a/modules/meeting/spec/features/meetings_new_spec.rb b/modules/meeting/spec/features/meetings_new_spec.rb
index c6a4d2226ee..fcf0b82f1eb 100644
--- a/modules/meeting/spec/features/meetings_new_spec.rb
+++ b/modules/meeting/spec/features/meetings_new_spec.rb
@@ -30,7 +30,7 @@ require 'spec_helper'
 
 require_relative '../support/pages/meetings/index'
 
-RSpec.describe 'Meetings new', :js do
+RSpec.describe 'Meetings new', :js, with_cuprite: false do
   shared_let(:project) { create(:project, enabled_module_names: %w[meetings]) }
   shared_let(:admin) { create(:admin) }
   let(:time_zone) { 'utc' }
@@ -67,17 +67,26 @@ RSpec.describe 'Meetings new', :js do
     let(:new_page) { Pages::Meetings::New.new(nil) }
 
     context 'with permission to create meetings' do
-      it 'does not render menus' do
-        index_page.expect_no_main_menu
+      it 'does not render menus', :with_cuprite do
+        new_page.visit!
+        new_page.expect_no_main_menu
+      end
+
+      describe 'clicking on the create new meeting button', :with_cuprite do
+        it 'navigates to the global create form' do
+          index_page.visit!
+          index_page.click_create_new
+          expect(page).to have_current_path(new_page.path)
+        end
       end
 
       ['CET', 'UTC', '', 'Pacific Time (US & Canada)'].each do |zone|
         let(:time_zone) { zone }
 
-        it "allows creating a project and handles errors in time zone #{zone}", with_cuprite: false do
-          index_page.visit!
+        it "allows creating a project and handles errors in time zone #{zone}" do
+          new_page.visit!
 
-          new_page = index_page.click_create_new
+          expect_angular_frontend_initialized # Wait for project dropdown to be ready
 
           new_page.set_title 'Some title'
           new_page.set_project project
@@ -97,7 +106,94 @@ RSpec.describe 'Meetings new', :js do
         end
       end
 
-      context 'without a title set', :with_cuprite do
+      context 'without a title set' do
+        before do
+          new_page.visit!
+
+          # Wait for project dropdown to be initialized
+          expect_angular_frontend_initialized
+
+          new_page.set_project project
+
+          new_page.set_start_date '2013-03-28'
+          new_page.set_start_time '13:30'
+          new_page.set_duration '1.5'
+          new_page.invite(other_user)
+        end
+
+        it 'renders a validation error' do
+          expect do
+            new_page.click_create
+          end.not_to change(Query, :count)
+
+          # HTML required attribute validation error
+          expect(page).to have_current_path(new_page.path)
+        end
+      end
+
+      context 'without a project set' do
+        before do
+          new_page.visit!
+          new_page.set_title 'Some title'
+          new_page.set_start_date '2013-03-28'
+          new_page.set_start_time '13:30'
+          new_page.set_duration '1.5'
+        end
+
+        it 'renders a validation error' do
+          new_page.click_create
+
+          new_page.expect_toast(message: "#{Project.model_name.human} #{I18n.t('activerecord.errors.messages.blank')}",
+                                type: :error)
+        end
+      end
+    end
+
+    context 'without permission to create meetings', :with_cuprite do
+      let(:permissions) { %i[view_meetings] }
+
+      it 'shows no edit link' do
+        index_page.visit!
+
+        index_page.expect_no_create_new_button
+      end
+    end
+
+    context 'as an admin', :with_cuprite do
+      let(:current_user) { admin }
+
+      it 'allows creating meeting in a project without members' do
+        new_page.visit!
+
+        expect_angular_frontend_initialized # Wait for project dropdown to be ready
+
+        new_page.set_title 'Some title'
+
+        new_page.set_project project
+
+        show_page = new_page.click_create
+
+        show_page.expect_toast(message: 'Successful creation')
+
+        # Not sure if that is then intended behaviour but that is what is currently programmed
+        show_page.expect_invited(admin)
+      end
+
+      context 'without a project set' do
+        before do
+          new_page.visit!
+          new_page.set_title 'Some title'
+        end
+
+        it 'renders a validation error' do
+          new_page.click_create
+
+          new_page.expect_toast(message: "#{Project.model_name.human} #{I18n.t('activerecord.errors.messages.blank')}",
+                                type: :error)
+        end
+      end
+
+      context 'without a title set' do
         before do
           new_page.visit!
 
@@ -116,68 +212,31 @@ RSpec.describe 'Meetings new', :js do
           expect(page).to have_current_path(new_page.path)
         end
       end
-
-      context 'without a project set', :with_cuprite do
-        before do
-          new_page.visit!
-          new_page.set_title 'Some title'
-        end
-
-        it 'renders a validation error' do
-          new_page.click_create
-
-          new_page.expect_toast(message: "#{Project.model_name.human} #{I18n.t('activerecord.errors.messages.blank')}",
-                                type: :error)
-        end
-      end
-    end
-
-    context 'without permission to create meetings' do
-      let(:permissions) { %i[view_meetings] }
-
-      it 'shows no edit link' do
-        index_page.visit!
-
-        index_page.expect_no_create_new_button
-      end
-    end
-
-    context 'as an admin' do
-      let(:current_user) { admin }
-
-      it 'allows creating meeting in a project without members', with_cuprite: false do
-        index_page.visit!
-
-        new_page = index_page.click_create_new
-        new_page.set_title 'Some title'
-
-        new_page.set_project project
-
-        show_page = new_page.click_create
-
-        show_page.expect_toast(message: 'Successful creation')
-
-        # Not sure if that is then intended behaviour but that is what is currently programmed
-        show_page.expect_invited(admin)
-      end
     end
   end
 
   context 'when creating a meeting from the project-specific page' do
     let(:index_page) { Pages::Meetings::Index.new(project:) }
+    let(:new_page) { Pages::Meetings::New.new(project) }
 
     context 'with permission to create meetings' do
       before do
         other_user
       end
 
+      describe 'clicking on the create new meeting button', :with_cuprite do
+        it 'navigates to the project-specific create form' do
+          index_page.visit!
+          index_page.click_create_new
+          expect(page).to have_current_path(new_page.path)
+        end
+      end
+
       ['CET', 'UTC', '', 'Pacific Time (US & Canada)'].each do |zone|
         let(:time_zone) { zone }
 
-        it "allows creating a project and handles errors in time zone #{zone}", with_cuprite: false do
-          index_page.visit!
-
-          new_page = index_page.click_create_new
+        it "allows creating a project and handles errors in time zone #{zone}" do
+          new_page.visit!
 
           new_page.set_title 'Some title'
           new_page.set_start_date '2013-03-28'
@@ -194,9 +253,28 @@ RSpec.describe 'Meetings new', :js do
           show_page.expect_date_time "03/28/2013 01:30 PM - 03:00 PM"
         end
       end
+
+      context 'without a title set' do
+        before do
+          new_page.visit!
+          new_page.set_start_date '2013-03-28'
+          new_page.set_start_time '13:30'
+          new_page.set_duration '1.5'
+          new_page.invite(other_user)
+        end
+
+        it 'renders a validation error' do
+          expect do
+            new_page.click_create
+          end.not_to change(Query, :count)
+
+          # HTML required attribute validation error
+          expect(page).to have_current_path(new_page.path)
+        end
+      end
     end
 
-    context 'without permission to create meetings' do
+    context 'without permission to create meetings', :with_cuprite do
       let(:permissions) { %i[view_meetings] }
 
       it 'shows no edit link' do
@@ -206,7 +284,7 @@ RSpec.describe 'Meetings new', :js do
       end
     end
 
-    context 'as an admin' do
+    context 'as an admin', :with_cuprite do
       let(:current_user) { admin }
       let(:field) do
         TextEditorField.new(page,
@@ -215,9 +293,7 @@ RSpec.describe 'Meetings new', :js do
       end
 
       it 'allows creating meeting in a project without members' do
-        index_page.visit!
-
-        new_page = index_page.click_create_new
+        new_page.visit!
 
         new_page.set_title 'Some title'
 
@@ -229,10 +305,23 @@ RSpec.describe 'Meetings new', :js do
         show_page.expect_invited(admin)
       end
 
-      it 'can save the meeting agenda via cmd+Enter' do
-        index_page.visit!
+      context 'without a title set' do
+        before do
+          new_page.visit!
+        end
 
-        new_page = index_page.click_create_new
+        it 'renders a validation error' do
+          expect do
+            new_page.click_create
+          end.not_to change(Query, :count)
+
+          # HTML required attribute validation error
+          expect(page).to have_current_path(new_page.path)
+        end
+      end
+
+      it 'can save the meeting agenda via cmd+Enter' do
+        new_page.visit!
 
         new_page.set_title 'Some title'
 
diff --git a/modules/meeting/spec/support/pages/meetings/new.rb b/modules/meeting/spec/support/pages/meetings/new.rb
index 042828e2989..49978f73513 100644
--- a/modules/meeting/spec/support/pages/meetings/new.rb
+++ b/modules/meeting/spec/support/pages/meetings/new.rb
@@ -33,6 +33,10 @@ module Pages::Meetings
   class New < Base
     include Components::Autocompleter::NgSelectAutocompleteHelpers
 
+    def expect_no_main_menu
+      expect(page).not_to have_selector '#main-menu'
+    end
+
     def click_create
       click_button 'Create'
 
@@ -50,7 +54,7 @@ module Pages::Meetings
     end
 
     def set_project(project)
-      select_autocomplete find("[data-qa-selector='project_id'"),
+      select_autocomplete find("[data-qa-selector='project_id']"),
                           query: project.name,
                           results_selector: 'body'
     end

From d66ed3d3ac788188e225fcda0aab1bfb5a23bf25 Mon Sep 17 00:00:00 2001
From: Aaron Contreras <aaronlcaq@gmail.com>
Date: Tue, 1 Aug 2023 12:51:29 -0500
Subject: [PATCH 3/3] Ensure project dropdown remains present after validation
 errors

When re-rendering the `new` action, the path was no longer
`new_meeting_path`. Extending the `global_create_context?` method
to account for the path no longer being only `new_meeting_path` but
also the create action's path with no `@project` set since this
is still part of the global create context.
---
 modules/meeting/app/helpers/meetings_helper.rb     | 10 +++++++++-
 modules/meeting/app/views/meetings/_form.html.erb  |  2 +-
 modules/meeting/spec/features/meetings_new_spec.rb |  3 +++
 modules/meeting/spec/support/pages/meetings/new.rb |  4 ++++
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/modules/meeting/app/helpers/meetings_helper.rb b/modules/meeting/app/helpers/meetings_helper.rb
index acd2a3b5e1a..b1e123d093a 100644
--- a/modules/meeting/app/helpers/meetings_helper.rb
+++ b/modules/meeting/app/helpers/meetings_helper.rb
@@ -171,7 +171,15 @@ module MeetingsHelper
     content_tag('div', "#{header}#{details}".html_safe, id: "change-#{journal.id}", class: 'journal')
   end
 
-  def global_create_context?
+  def global_meeting_create_context?
+    global_new_meeting_action? || global_create_meeting_action?
+  end
+
+  def global_new_meeting_action?
     request.path == new_meeting_path
   end
+
+  def global_create_meeting_action?
+    request.path == meetings_path && @project.nil?
+  end
 end
diff --git a/modules/meeting/app/views/meetings/_form.html.erb b/modules/meeting/app/views/meetings/_form.html.erb
index 11738e292f3..14902e91bca 100644
--- a/modules/meeting/app/views/meetings/_form.html.erb
+++ b/modules/meeting/app/views/meetings/_form.html.erb
@@ -35,7 +35,7 @@ See COPYRIGHT and LICENSE files for more details.
     <%= f.text_field :title, :required => true, :size => 60, container_class: '-wide' %>
   </div>
 
-  <% if global_create_context? %>
+  <% if global_meeting_create_context? %>
     <div class="form--field -required">
       <label class="form--label" for="project_id"><%= Meeting.human_attribute_name(:project) %>:</label>
       <div class="form--field-container">
diff --git a/modules/meeting/spec/features/meetings_new_spec.rb b/modules/meeting/spec/features/meetings_new_spec.rb
index fcf0b82f1eb..0bcae2a8b9d 100644
--- a/modules/meeting/spec/features/meetings_new_spec.rb
+++ b/modules/meeting/spec/features/meetings_new_spec.rb
@@ -145,6 +145,8 @@ RSpec.describe 'Meetings new', :js, with_cuprite: false do
 
           new_page.expect_toast(message: "#{Project.model_name.human} #{I18n.t('activerecord.errors.messages.blank')}",
                                 type: :error)
+
+          new_page.expect_project_dropdown
         end
       end
     end
@@ -190,6 +192,7 @@ RSpec.describe 'Meetings new', :js, with_cuprite: false do
 
           new_page.expect_toast(message: "#{Project.model_name.human} #{I18n.t('activerecord.errors.messages.blank')}",
                                 type: :error)
+          new_page.expect_project_dropdown
         end
       end
 
diff --git a/modules/meeting/spec/support/pages/meetings/new.rb b/modules/meeting/spec/support/pages/meetings/new.rb
index 49978f73513..528552fedc2 100644
--- a/modules/meeting/spec/support/pages/meetings/new.rb
+++ b/modules/meeting/spec/support/pages/meetings/new.rb
@@ -53,6 +53,10 @@ module Pages::Meetings
       fill_in 'Title', with: text
     end
 
+    def expect_project_dropdown
+      find "[data-qa-selector='project_id']"
+    end
+
     def set_project(project)
       select_autocomplete find("[data-qa-selector='project_id']"),
                           query: project.name,