From e0cf595294c8f4369c0e788cd4f432b3e3fe1b9d Mon Sep 17 00:00:00 2001 From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com> Date: Fri, 9 Aug 2024 12:25:40 +0100 Subject: [PATCH] CLDC-2093 Merge organisations page - view all merge requests (#2561) --- app/controllers/merge_requests_controller.rb | 2 +- app/controllers/organisations_controller.rb | 3 + app/helpers/tag_helper.rb | 8 +++ app/models/merge_request.rb | 18 +++++- app/models/merge_request_organisation.rb | 6 +- .../_merge_request_list.html.erb | 58 +++++++++++++++++++ app/views/organisations/index.html.erb | 25 ++++---- ...134014_add_merge_date_to_merge_requests.rb | 5 ++ db/schema.rb | 3 +- spec/factories/merge_requests.rb | 7 +++ spec/features/schemes_spec.rb | 2 + spec/models/merge_request_spec.rb | 26 +++++++++ .../merge_requests_controller_spec.rb | 14 ++--- .../requests/organisations_controller_spec.rb | 13 +++++ 14 files changed, 166 insertions(+), 24 deletions(-) create mode 100644 app/views/organisations/_merge_request_list.html.erb create mode 100644 db/migrate/20240808134014_add_merge_date_to_merge_requests.rb create mode 100644 spec/factories/merge_requests.rb create mode 100644 spec/models/merge_request_spec.rb diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index d717f6ee7..5d1e2550e 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -25,7 +25,7 @@ class MergeRequestsController < ApplicationController def create ActiveRecord::Base.transaction do - @merge_request = MergeRequest.create!(merge_request_params.merge(status: :unsubmitted)) + @merge_request = MergeRequest.create!(merge_request_params.merge(status: :incomplete)) MergeRequestOrganisation.create!({ merge_request: @merge_request, merging_organisation: @merge_request.requesting_organisation }) end redirect_to organisations_merge_request_path(@merge_request) diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index dfe50fa22..5d2225a6e 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -16,6 +16,9 @@ class OrganisationsController < ApplicationController all_organisations = Organisation.order(:name) @pagy, @organisations = pagy(filtered_collection(all_organisations.visible, search_term)) + @merge_requests = MergeRequest.visible + .joins("LEFT JOIN organisations ON organisations.id = merge_requests.absorbing_organisation_id") + .order("organisations.name") @searched = search_term.presence @total_count = all_organisations.visible.size end diff --git a/app/helpers/tag_helper.rb b/app/helpers/tag_helper.rb index c389942dc..3c2e332f6 100644 --- a/app/helpers/tag_helper.rb +++ b/app/helpers/tag_helper.rb @@ -15,6 +15,10 @@ module TagHelper deleted: "Deleted", merged: "Merged", unconfirmed: "Unconfirmed", + merge_issues: "Merge issues", + request_merged: "Merged", + ready_to_merge: "Ready to merge", + processing: "Processing", }.freeze COLOUR = { @@ -31,6 +35,10 @@ module TagHelper deleted: "red", merged: "orange", unconfirmed: "blue", + merge_issues: "orange", + request_merged: "green", + ready_to_merge: "blue", + processing: "yellow", }.freeze def status_tag(status, classes = []) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e62b1d39d..42496a8fe 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -7,14 +7,28 @@ class MergeRequest < ApplicationRecord validates :new_telephone_number, presence: true, if: -> { telephone_number_correct == false } STATUS = { - "unsubmitted" => 0, - "submitted" => 1, + "merge_issues" => 0, + "incomplete" => 1, + "ready_to_merge" => 2, + "processing" => 3, + "request_merged" => 4, }.freeze enum status: STATUS + scope :not_merged, -> { where.not(status: "request_merged") } + scope :merged, -> { where(status: "request_merged") } + scope :visible, lambda { + open_collection_period_start_date = FormHandler.instance.start_date_of_earliest_open_collection_period + merged.where("merge_requests.merge_date >= ?", open_collection_period_start_date).or(not_merged) + } + def organisation_name_uniqueness if Organisation.where("lower(name) = ?", new_organisation_name&.downcase).exists? errors.add(:new_organisation_name, :invalid) end end + + def absorbing_organisation_name + absorbing_organisation&.name || "" + end end diff --git a/app/models/merge_request_organisation.rb b/app/models/merge_request_organisation.rb index 08c1d6d45..567b3f654 100644 --- a/app/models/merge_request_organisation.rb +++ b/app/models/merge_request_organisation.rb @@ -5,7 +5,7 @@ class MergeRequestOrganisation < ApplicationRecord validates :merging_organisation, presence: { message: I18n.t("validations.merge_request.merging_organisation_id.blank") } validate :validate_merging_organisations - scope :not_unsubmitted, -> { joins(:merge_request).where.not(merge_requests: { status: "unsubmitted" }) } + scope :merged, -> { joins(:merge_request).where(merge_requests: { status: "request_merged" }) } scope :with_merging_organisation, ->(merging_organisation) { where(merging_organisation:) } has_paper_trail @@ -17,12 +17,12 @@ private errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) end - if MergeRequestOrganisation.not_unsubmitted.with_merging_organisation(merging_organisation).count.positive? + if MergeRequestOrganisation.merged.with_merging_organisation(merging_organisation).count.positive? errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) end - if MergeRequest.not_unsubmitted.where.not(id: merge_request_id).where(requesting_organisation: merging_organisation).count.positive? + if MergeRequest.merged.where.not(id: merge_request_id).where(requesting_organisation: merging_organisation).count.positive? errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) end diff --git a/app/views/organisations/_merge_request_list.html.erb b/app/views/organisations/_merge_request_list.html.erb new file mode 100644 index 000000000..6e4bab9fa --- /dev/null +++ b/app/views/organisations/_merge_request_list.html.erb @@ -0,0 +1,58 @@ +
+ <% if @merge_requests.empty? %> +

No merge requests

+ <% else %> + <%= govuk_table do |table| %> + <%= table.with_caption(classes: %w[govuk-!-font-size-19 govuk-!-font-weight-regular]) do |caption| %> + <%= @merge_requests.where.not(status: 4).count %> unresolved merge requests + <% end %> + <%= table.with_head do |head| %> + <%= head.with_row do |row| %> + <% row.with_cell(header: true, text: "Absorbing organisation", html_attributes: { + scope: "col", + }) %> + <% row.with_cell(header: true, text: "Merge date", html_attributes: { + scope: "col", + }) %> + <% row.with_cell(header: true, text: "Status", html_attributes: { + scope: "col", + }) %> + <% row.with_cell(header: true, text: "", html_attributes: { + scope: "col", + }) %> + <% end %> + <% end %> + <% @merge_requests.each do |merge_request| %> + <%= table.with_body do |body| %> + <%= body.with_row do |row| %> + <% if merge_request.absorbing_organisation_name.blank? %> + <% row.with_cell(text: "You didn't answer this question", + html_attributes: { + scope: "row", + class: "app-!-colour-muted", + }) %> + <% else %> + <% row.with_cell(text: merge_request.absorbing_organisation_name) %> + <% end %> + <% merge_date = merge_request.merge_date %> + <% if merge_date.nil? %> + <% row.with_cell(text: "You didn't answer this question", + html_attributes: { + scope: "row", + class: "app-!-colour-muted", + }) %> + <% else %> + <% row.with_cell(text: merge_date.strftime("%d %B %Y")) %> + <% end %> + <% row.with_cell(text: status_tag(merge_request.status)) %> + <% row.with_cell(html_attributes: { + scope: "row", + }) do %> + <%= govuk_link_to("View details", "/merge-request/#{merge_request.id}/organisations") %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> +
diff --git a/app/views/organisations/index.html.erb b/app/views/organisations/index.html.erb index 72d1d2f43..4701e67fd 100644 --- a/app/views/organisations/index.html.erb +++ b/app/views/organisations/index.html.erb @@ -5,13 +5,18 @@ <%= render partial: "organisations/headings", locals: request.path == organisations_path ? { main: "Organisations", sub: nil } : { main: @organisation.name, sub: "Organisations" } %> -<% if current_user.support? %> - <%= govuk_button_link_to "Create a new organisation", new_organisation_path, html: { method: :get } %> -<% end %> - -<%= render SearchComponent.new(current_user:, search_label: "Search by organisation name", value: @searched) %> - -<%= govuk_section_break(visible: true, size: "m") %> - -<%= render partial: "organisation_list", locals: { organisations: @organisations, title: "Organisations", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> -<%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "organisations" } %> +
+ <%= govuk_tabs(title: "Collection resources", classes: %w[app-tab__large-headers]) do |c| %> + <% c.with_tab(label: "All organisations") do %> + <%= govuk_button_link_to "Create a new organisation", new_organisation_path, html: { method: :get } %> + <%= render SearchComponent.new(current_user:, search_label: "Search by organisation name", value: @searched) %> + <%= govuk_section_break(visible: true, size: "m") %> + <%= render partial: "organisation_list", locals: { organisations: @organisations, title: "Organisations", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> + <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "organisations" } %> + <% end %> + <% c.with_tab(label: "Merge requests") do %> + <%= govuk_button_link_to "Create new merge request", "/organisations/#{current_user.organisation_id}/merge-request", html: { method: :get } %> + <%= render partial: "merge_request_list", locals: { merge_requests: @merge_requests, title: "Merge requests", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> + <% end %> + <% end %> +
diff --git a/db/migrate/20240808134014_add_merge_date_to_merge_requests.rb b/db/migrate/20240808134014_add_merge_date_to_merge_requests.rb new file mode 100644 index 000000000..80a2c5650 --- /dev/null +++ b/db/migrate/20240808134014_add_merge_date_to_merge_requests.rb @@ -0,0 +1,5 @@ +class AddMergeDateToMergeRequests < ActiveRecord::Migration[7.0] + def change + add_column :merge_requests, :merge_date, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 50073d2d6..f9a17bd49 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_07_15_082338) do +ActiveRecord::Schema[7.0].define(version: 2024_08_08_134014) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -430,6 +430,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_07_15_082338) do t.string "new_organisation_address_line2" t.string "new_organisation_postcode" t.string "new_organisation_telephone_number" + t.datetime "merge_date" end create_table "notifications", force: :cascade do |t| diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb new file mode 100644 index 000000000..888639b82 --- /dev/null +++ b/spec/factories/merge_requests.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :merge_request do + status { "incomplete" } + merge_date { nil } + association :requesting_organisation, factory: :organisation + end +end diff --git a/spec/features/schemes_spec.rb b/spec/features/schemes_spec.rb index 2d43e9b02..0706a0d7e 100644 --- a/spec/features/schemes_spec.rb +++ b/spec/features/schemes_spec.rb @@ -766,8 +766,10 @@ RSpec.describe "Schemes scheme Features" do before do Timecop.freeze(Time.zone.local(2023, 10, 10)) + Singleton.__init__(FormHandler) FactoryBot.create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 4), location: deactivated_location) Timecop.unfreeze + Singleton.__init__(FormHandler) click_link(scheme.service_name) end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb new file mode 100644 index 000000000..e3bb24b46 --- /dev/null +++ b/spec/models/merge_request_spec.rb @@ -0,0 +1,26 @@ +require "rails_helper" + +RSpec.describe MergeRequest, type: :model do + describe ".visible" do + let(:open_collection_period_start_date) { 1.year.ago } + let!(:merged_recent) { create(:merge_request, status: "request_merged", merge_date: 3.months.ago) } + let!(:merged_old) { create(:merge_request, status: "request_merged", merge_date: 18.months.ago) } + let!(:not_merged) { create(:merge_request, status: "incomplete") } + + before do + allow(FormHandler.instance).to receive(:start_date_of_earliest_open_collection_period).and_return(open_collection_period_start_date) + end + + it "includes merged requests with merge dates after the open collection period start date" do + expect(described_class.visible).to include(merged_recent) + end + + it "excludes merged requests with merge dates before the open collection period start date" do + expect(described_class.visible).not_to include(merged_old) + end + + it "includes not_merged requests" do + expect(described_class.visible).to include(not_merged) + end + end +end diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 6f7b64f16..fe9f1b16e 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -14,7 +14,7 @@ RSpec.describe MergeRequestsController, type: :request do before { sign_in user } describe "#organisations" do - let(:params) { { merge_request: { requesting_organisation_id: "9", status: "unsubmitted" } } } + let(:params) { { merge_request: { requesting_organisation_id: "9", status: "incomplete" } } } context "when creating a new merge request" do before do @@ -29,7 +29,7 @@ RSpec.describe MergeRequestsController, type: :request do end context "when passing a different requesting organisation id" do - let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "unsubmitted" } } } + let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } } it "creates merge request with current user organisation" do follow_redirect! @@ -84,7 +84,7 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } before do - MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "submitted") + MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged") patch "/merge-request/#{merge_request.id}/organisations", headers:, params: end @@ -100,7 +100,7 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } before do - MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "unsubmitted") + MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "incomplete") patch "/merge-request/#{merge_request.id}/organisations", headers:, params: end @@ -116,7 +116,7 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do - existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "submitted") + existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged") MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id) patch "/merge-request/#{merge_request.id}/organisations", headers:, params: end @@ -134,7 +134,7 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do - existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "unsubmitted") + existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "incomplete") MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id) patch "/merge-request/#{merge_request.id}/organisations", headers:, params: end @@ -701,7 +701,7 @@ RSpec.describe MergeRequestsController, type: :request do end describe "#organisations" do - let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "unsubmitted" } } } + let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } } before do post "/merge-request", headers:, params: diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 13879a38c..6c4bc6202 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -1017,6 +1017,19 @@ RSpec.describe OrganisationsController, type: :request do expect(page).to have_field("search", type: "search") end + it "shows the merge request list" do + expect(page).to have_content("Merge requests") + end + + it "has a create new merge request button" do + expect(page).to have_link("Create new merge request") + end + + it "displays 'No merge requests' when @merge_requests is empty" do + allow(MergeRequest).to receive(:visible).and_return(nil) + expect(page).to have_content("No merge requests") + end + context "when viewing a specific organisation's lettings logs" do let(:parent_organisation) { create(:organisation) } let(:child_organisation) { create(:organisation) }