Cldc 1663 remove housing provider (#957)

* wip

* Rename managing_agents column

* add managing relationship

* f

* feat: add my features branched off managing agents branch

* feat: update nav behaviour

* feat: simplify housing_providers view

* feat: fix pluralise to default to plural rather than singular

* feat: remove managing agent related code so can be merged directly

* tests: update tests and add new ones for housing_providers

* refactor: rubocop conciliation

* tests: fix failing navigation tests

* tests: one more plural test

* refactor: erb linting

* refactor: erb linting

* feat: right-align "Remove" text

* feat: update nokogiri to pass bundler-audit

* feat: grey out search button

* feat: remove section-break

* feat: add housing provider page with details and button

* feat: tidy up routing

* feat: add wip housing provider behaviour without functioning search

* feat: wip add housing provider functionality hard coded to add FooBar LTD as provider to DLUHC

* feat: remove redundant code

* feat: add data passing behaviour without accessible autocomplete

* feat: use accessible autocomplete (not working)

* feat: use accessible autocomplete (now working)

* feat: wip commit error messages

* feat: add banner always

* feat: add conditional banner behaviour, back link and update logs titles

* feat: add search icon to accessible autocomplete, make hint text aligned correctly

* feat: use pluck not all

* tests: add initial test

* feat: refactor create logic

* feat: add sub org title

* feat: add correct no housing providers text

* feat: add correct no housing providers text

* tests: add tests for add housing provider page

* feat: remove unnecessary line from controller

* fet: simplify controller args

* fet: update schema

* feat: update schema

* feat: add core removal functionality

* fix: make secondary navbar display selected org not user org for support users

* refactor: tidy up code and add single quotes instead of apostrophes

* refactor: remove unnecessary lines

* test: add housing provider removal tests

* feat: use path helpers and beng methods

* refactor: linting

* refactor: erblinting

* refactor: renaming and tidying

* refactor: simplify format response and paths and remove redundant rendering

* feat: user flash notices instead of explicit notification banners

* test: update tests with new urls

* feat: add target org helper

* feat: add target org helper fix

* feat: add related org helper

* test: update paths in tests

Co-authored-by: Jack S <jacopo.scotti@softwire.com>

app/controllers/organisation_relationships_controller.rb

@@ -9,108 +9,106 @@ class OrganisationRelationshipsController < ApplicationController
     housing_providers = organisation.housing_providers
     unpaginated_filtered_housing_providers = filtered_collection(housing_providers, search_term)
     organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name)
-    respond_to do |format|
+    respond_to :html
-      format.html do
+    @pagy, @housing_providers = pagy(unpaginated_filtered_housing_providers)
-        @pagy, @housing_providers = pagy(unpaginated_filtered_housing_providers)
+    @organisations = organisations
-        @organisations = organisations
+    @searched = search_term.presence
-        @searched = search_term.presence
+    @total_count = housing_providers.size
-        @total_count = housing_providers.size
-        render "organisation_relationships/housing_providers", layout: "application"
-      end
-    end
   end
 
   def managing_agents
     managing_agents = organisation.managing_agents
     unpaginated_filtered_managing_agents = filtered_collection(managing_agents, search_term)
     organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name)
-    respond_to do |format|
+    respond_to :html
-      format.html do
+    @pagy, @managing_agents = pagy(unpaginated_filtered_managing_agents)
-        @pagy, @managing_agents = pagy(unpaginated_filtered_managing_agents)
+    @organisations = organisations
-        @organisations = organisations
+    @searched = search_term.presence
-        @searched = search_term.presence
+    @total_count = managing_agents.size
-        @total_count = managing_agents.size
-        render "organisation_relationships/managing_agents", layout: "application"
-      end
-    end
   end
 
   def add_housing_provider
     @organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name)
-    respond_to do |format|
+    respond_to :html
-      format.html do
-        render "organisation_relationships/add_housing_provider", layout: "application"
-      end
-    end
   end
 
   def add_managing_agent
     @organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name)
-    respond_to do |format|
+    respond_to :html
-      format.html do
-        render "organisation_relationships/add_managing_agent", layout: "application"
-      end
-    end
   end
 
   def create_housing_provider
-    child_organisation_id = @organisation.id
+    child_organisation = @organisation
-    parent_organisation_id = related_organisation_id
     relationship_type = OrganisationRelationship::OWNING
-    if related_organisation_id.empty?
+    if params[:organisation][:related_organisation_id].empty?
       @organisation.errors.add :related_organisation_id, "You must choose a housing provider"
-      @organisations = Organisation.where.not(id: child_organisation_id).pluck(:id, :name)
+      @organisations = Organisation.where.not(id: child_organisation.id).pluck(:id, :name)
-      render "organisation_relationships/add_housing_provider"
-      return
-    elsif OrganisationRelationship.exists?(child_organisation_id:, parent_organisation_id:, relationship_type:)
-      @organisation.errors.add :related_organisation_id, "You have already added this housing provider"
-      @organisations = Organisation.where.not(id: child_organisation_id).pluck(:id, :name)
       render "organisation_relationships/add_housing_provider"
       return
+    else
+      parent_organisation = related_organisation
+      if OrganisationRelationship.exists?(child_organisation:, parent_organisation:, relationship_type:)
+        @organisation.errors.add :related_organisation_id, "You have already added this housing provider"
+        @organisations = Organisation.where.not(id: child_organisation.id).pluck(:id, :name)
+        render "organisation_relationships/add_housing_provider"
+        return
+      end
     end
-    create!(child_organisation_id:, parent_organisation_id:, relationship_type:)
+    create!(child_organisation:, parent_organisation:, relationship_type:)
-    redirect_to housing_providers_organisation_path(related_organisation_id:)
+    flash[:notice] = "#{related_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} housing providers"
+    redirect_to housing_providers_organisation_path
   end
 
   def create_managing_agent
-    parent_organisation_id = @organisation.id
+    parent_organisation = @organisation
-    child_organisation_id = related_organisation_id
     relationship_type = OrganisationRelationship::MANAGING
-
+    if params[:organisation][:related_organisation_id].empty?
-    if related_organisation_id.empty?
       @organisation.errors.add :related_organisation_id, "You must choose a managing agent"
-      @organisations = Organisation.where.not(id: parent_organisation_id).pluck(:id, :name)
+      @organisations = Organisation.where.not(id: parent_organisation.id).pluck(:id, :name)
-      render "organisation_relationships/add_managing_agent"
-      return
-    elsif OrganisationRelationship.exists?(child_organisation_id:, parent_organisation_id:, relationship_type:)
-      @organisation.errors.add :related_organisation_id, "You have already added this managing agent"
-      @organisations = Organisation.where.not(id: parent_organisation_id).pluck(:id, :name)
       render "organisation_relationships/add_managing_agent"
       return
+    else
+      child_organisation = related_organisation
+      if OrganisationRelationship.exists?(child_organisation:, parent_organisation:, relationship_type:)
+        @organisation.errors.add :related_organisation_id, "You have already added this managing agent"
+        @organisations = Organisation.where.not(id: parent_organisation.id).pluck(:id, :name)
+        render "organisation_relationships/add_managing_agent"
+        return
+      end
     end
+    create!(child_organisation:, parent_organisation:, relationship_type:)
+    flash[:notice] = "#{related_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} managing agents"
+    redirect_to managing_agents_organisation_path
+  end
+
+  def remove_housing_provider
+    @target_organisation_id = target_organisation.id
+  end
 
-    create!(child_organisation_id:, parent_organisation_id:, relationship_type:)
+  def delete_housing_provider
-    redirect_to managing_agents_organisation_path(related_organisation_id:)
+    organisation_relationship_to_remove = OrganisationRelationship.find_by!(child_organisation: @organisation, parent_organisation: target_organisation, relationship_type: OrganisationRelationship::OWNING)
+    organisation_relationship_to_remove.destroy!
+    flash[:notice] = "#{target_organisation.name} is no longer one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} housing providers"
+    redirect_to housing_providers_organisation_path
   end
 
 private
 
-  def create!(child_organisation_id:, parent_organisation_id:, relationship_type:)
+  def create!(child_organisation:, parent_organisation:, relationship_type:)
-    @resource = OrganisationRelationship.new(child_organisation_id:, parent_organisation_id:, relationship_type:)
+    @resource = OrganisationRelationship.new(child_organisation:, parent_organisation:, relationship_type:)
     @resource.save!
   end
 
-  def create(child_organisation_id, parent_organisation_id, relationship_type)
+  def organisation
-    @resource = OrganisationRelationship.new(child_organisation_id:, parent_organisation_id:, relationship_type:)
+    @organisation ||= Organisation.find(params[:id])
-    @resource.save!
   end
 
-  def related_organisation_id
+  def related_organisation
-    params["organisation"]["related_organisation_id"]
+    @related_organisation ||= Organisation.find(params[:organisation][:related_organisation_id])
   end
 
-  def organisation
+  def target_organisation
-    @organisation ||= Organisation.find(params[:id])
+    @target_organisation ||= Organisation.find(params[:target_organisation_id])
   end
 
   def search_term

app/helpers/navigation_items_helper.rb

@@ -40,8 +40,8 @@ module NavigationItemsHelper
         NavigationItem.new("Schemes", "/organisations/#{current_organisation_id}/schemes", subnav_supported_housing_schemes_path?(path)),
         NavigationItem.new("Users", "/organisations/#{current_organisation_id}/users", subnav_users_path?(path)),
         NavigationItem.new("About this organisation", "/organisations/#{current_organisation_id}", subnav_details_path?(path)),
-        (NavigationItem.new("Housing providers", housing_providers_organisation_path(current_user.organisation), housing_providers_path?(path)) if FeatureToggle.managing_owning_enabled?),
+        (NavigationItem.new("Housing providers", housing_providers_organisation_path(current_organisation_id), housing_providers_path?(path)) if FeatureToggle.managing_owning_enabled?),
-        (NavigationItem.new("Managing agents", managing_agents_organisation_path(current_user.organisation), managing_agents_path?(path)) if FeatureToggle.managing_owning_enabled?),
+        (NavigationItem.new("Managing agents", managing_agents_organisation_path(current_organisation_id), managing_agents_path?(path)) if FeatureToggle.managing_owning_enabled?),
       ].compact
     else
       [
@@ -49,8 +49,8 @@ module NavigationItemsHelper
         FeatureToggle.sales_log_enabled? ? NavigationItem.new("Sales logs", "/organisations/#{current_organisation_id}/sales-logs", sales_logs_current?(path)) : nil,
         NavigationItem.new("Users", "/organisations/#{current_organisation_id}/users", subnav_users_path?(path)),
         NavigationItem.new("About this organisation", "/organisations/#{current_organisation_id}", subnav_details_path?(path)),
-        (NavigationItem.new("Housing providers", housing_providers_organisation_path(current_user.organisation), housing_providers_path?(path)) if FeatureToggle.managing_owning_enabled?),
+        (NavigationItem.new("Housing providers", housing_providers_organisation_path(current_organisation_id), housing_providers_path?(path)) if FeatureToggle.managing_owning_enabled?),
-        (NavigationItem.new("Managing agents", managing_agents_organisation_path(current_user.organisation), managing_agents_path?(path)) if FeatureToggle.managing_owning_enabled?),
+        (NavigationItem.new("Managing agents", managing_agents_organisation_path(current_organisation_id), managing_agents_path?(path)) if FeatureToggle.managing_owning_enabled?),
       ].compact
     end
   end

app/views/organisation_relationships/_housing_provider_list.erb

@@ -12,7 +12,7 @@
               scope: "row",
               class: "govuk-!-text-align-right",
             }) do %>
-              <%= govuk_link_to("Remove", "housing-providers/#{housing_provider.id}") %>
+              <%= govuk_link_to("Remove", housing_providers_remove_organisation_path(target_organisation_id: housing_provider.id)) %>
             <% end %>
           <% end %>
         <% end %>

app/views/organisation_relationships/housing_providers.html.erb

@@ -1,29 +1,8 @@
 <% item_label = format_label(@pagy.count, "housing providers") %>
-
-<% if current_user.data_coordinator? %>
-  <% if params["related_organisation_id"] %>
-    <%= govuk_notification_banner(
-          title_text: "Success",
-          success: true, title_heading_level: 3,
-          title_id: "swanky-notifications"
-        ) do |notification_banner|
-          notification_banner.heading(text: "#{Organisation.find(params['related_organisation_id']).name} is now one of your housing providers")
-        end %>
-  <% end %>
-<% end %>
 <% if current_user.support? %>
   <%= render partial: "organisations/headings", locals: { main: @organisation.name, sub: nil } %>
   <%= render SubNavigationComponent.new(items: secondary_items(request.path, @organisation.id)) %>
   <h2 class="govuk-visually-hidden">Housing Providers</h2>
-  <% if params["related_organisation_id"] %>
-    <%= govuk_notification_banner(
-          title_text: "Success",
-          success: true, title_heading_level: 3,
-          title_id: "swanky-notifications"
-        ) do |notification_banner|
-          notification_banner.heading(text: "#{Organisation.find(params['related_organisation_id']).name} is now one of this organisation's housing providers")
-        end %>
-  <% end %>
   <p class="govuk-body">This organisation can submit logs for its housing providers.</p>
   <% if @total_count == 0 %>
     <p class="govuk-body">This organisation does not currently have any housing providers.</p>
@@ -35,10 +14,7 @@
     <p class="govuk-body">You do not currently have any housing providers.</p>
   <% end %>
 <% end %>
-
+<% if current_user.support? || current_user.data_coordinator? %>
-<% if current_user.support? %>
-  <%= govuk_button_link_to "Add a housing provider", housing_providers_add_organisation_path(organisation_id: @organisation.id), html: { method: :get } %>
-  <% elsif current_user.data_coordinator? %>
   <%= govuk_button_link_to "Add a housing provider", housing_providers_add_organisation_path, html: { method: :get } %>
 <% end %>
 <% if @total_count != 0 %>

app/views/organisation_relationships/managing_agents.html.erb

@@ -38,9 +38,7 @@
     <p class="govuk-body">This organisation does not currently have any managing agents.</p>
   <% end %>
 <% end %>
-<% if current_user.support? %>
+<% if current_user.support? || current_user.data_coordinator? %>
-  <%= govuk_button_link_to "Add a managing agent", managing_agents_add_organisation_path(organisation_id: @organisation.id), html: { method: :get } %>
-<% elsif current_user.data_coordinator? %>
   <%= govuk_button_link_to "Add a managing agent", managing_agents_add_organisation_path, html: { method: :get } %>
 <% end %>
 <% if @total_count != 0 %>

app/views/organisation_relationships/remove_housing_provider.html.erb

@@ -0,0 +1,21 @@
+<%= form_with url: housing_providers_organisation_path(target_organisation_id: @target_organisation.id), method: "delete", local: true do |f| %>
+  <% if current_user.support? %>
+    <%= render partial: "organisations/headings", locals: { main: @organisation.name, sub: nil } %>
+    <%= render SubNavigationComponent.new(items: secondary_items(request.path, @organisation.id)) %>
+    <h2 class="govuk-visually-hidden">Remove Housing Provider</h2>
+  <% end %>
+  <% if current_user.support? %>
+    <%= govuk_back_link(href: :back) %>
+    <%= render partial: "organisations/headings", locals: { main: "You are removing ‘#{@target_organisation.name}’ from this organisation's housing providers", sub: nil } %>
+  <% else %>
+    <% content_for :before_content do %>
+      <%= govuk_back_link(href: :back) %>
+    <% end %>
+    <%= render partial: "organisations/headings", locals: { main: "You are removing ‘#{@target_organisation.name}’ from your organisation's housing providers", sub: nil } %>
+  <% end %>
+  <%= govuk_warning_text text: "You will no longer be able to submit logs for #{@target_organisation.name}" %>
+  <div class="govuk-button-group">
+      <%= f.govuk_submit "Confirm" %>
+      <%= govuk_button_link_to "Cancel", housing_providers_organisation_path(current_user.organisation), html: { method: :get }, secondary: true %>
+  </div>
+<% end %>

config/routes.rb

@@ -81,7 +81,9 @@ Rails.application.routes.draw do
       get "schemes", to: "organisations#schemes"
       get "housing-providers", to: "organisation_relationships#housing_providers"
       get "housing-providers/add", to: "organisation_relationships#add_housing_provider"
+      get "housing-providers/remove", to: "organisation_relationships#remove_housing_provider"
       post "housing-providers", to: "organisation_relationships#create_housing_provider"
+      delete "housing-providers", to: "organisation_relationships#delete_housing_provider"
       get "managing-agents", to: "organisation_relationships#managing_agents"
       get "managing-agents/add", to: "organisation_relationships#add_managing_agent"
       post "managing-agents", to: "organisation_relationships#create_managing_agent"

db/schema.rb

@@ -359,8 +359,6 @@ ActiveRecord::Schema[7.0].define(version: 2022_10_19_082625) do
     t.integer "hholdcount"
     t.integer "age3"
     t.integer "age3_known"
-    t.integer "income1"
-    t.integer "income1nk"
     t.integer "age4"
     t.integer "age4_known"
     t.integer "age5"
@@ -369,6 +367,8 @@ ActiveRecord::Schema[7.0].define(version: 2022_10_19_082625) do
     t.integer "age6_known"
     t.string "la"
     t.integer "la_known"
+    t.integer "income1"
+    t.integer "income1nk"
     t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id"
     t.index ["managing_organisation_id"], name: "index_sales_logs_on_managing_organisation_id"
     t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id"

spec/requests/organisation_relationships_controller_spec.rb

@@ -158,7 +158,7 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         it "redirects to the organisation list" do
           request
-          expect(response).to redirect_to("/organisations/#{organisation.id}/housing-providers?related_organisation_id=#{housing_provider.id}")
+          expect(response).to redirect_to("/organisations/#{organisation.id}/housing-providers")
         end
       end
 
@@ -186,7 +186,30 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         it "redirects to the organisation list" do
           request
-          expect(response).to redirect_to("/organisations/#{organisation.id}/managing-agents?related_organisation_id=#{managing_agent.id}")
+          expect(response).to redirect_to("/organisations/#{organisation.id}/managing-agents")
+        end
+      end
+
+      describe "organisation_relationships#delete_housing_provider" do
+        let!(:housing_provider) { FactoryBot.create(:organisation) }
+        let(:params) do
+          {
+            "target_organisation_id": housing_provider.id,
+          }
+        end
+        let(:request) { delete "/organisations/#{organisation.id}/housing-providers", headers:, params: }
+
+        before do
+          FactoryBot.create(:organisation_relationship, :owning, child_organisation: organisation, parent_organisation: housing_provider)
+        end
+
+        it "deletes the new organisation relationship" do
+          expect { request }.to change(OrganisationRelationship, :count).by(-1)
+        end
+
+        it "redirects to the organisation list" do
+          request
+          expect(response).to redirect_to("/organisations/#{organisation.id}/housing-providers")
         end
       end
     end
@@ -337,7 +360,7 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         it "redirects to the organisation list" do
           request
-          expect(response).to redirect_to("/organisations/#{organisation.id}/housing-providers?related_organisation_id=#{housing_provider.id}")
+          expect(response).to redirect_to("/organisations/#{organisation.id}/housing-providers")
         end
       end
 
@@ -365,7 +388,30 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         it "redirects to the organisation list" do
           request
-          expect(response).to redirect_to("/organisations/#{organisation.id}/managing-agents?related_organisation_id=#{managing_agent.id}")
+          expect(response).to redirect_to("/organisations/#{organisation.id}/managing-agents")
+        end
+      end
+
+      describe "organisation_relationships#delete_housing_provider" do
+        let!(:housing_provider) { FactoryBot.create(:organisation) }
+        let(:params) do
+          {
+            "target_organisation_id": housing_provider.id,
+          }
+        end
+        let(:request) { delete "/organisations/#{organisation.id}/housing-providers", headers:, params: }
+
+        before do
+          FactoryBot.create(:organisation_relationship, :owning, child_organisation: organisation, parent_organisation: housing_provider)
+        end
+
+        it "deletes the new organisation relationship" do
+          expect { request }.to change(OrganisationRelationship, :count).by(-1)
+        end
+
+        it "redirects to the organisation list" do
+          request
+          expect(response).to redirect_to("/organisations/#{organisation.id}/housing-providers")
         end
       end
 
@@ -400,6 +446,10 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
           expect(page).not_to have_content(other_org_housing_provider.name)
         end
 
+        it "shows remove link(s)" do
+          expect(response.body).to include("Remove")
+        end
+
         it "shows the pagination count" do
           expect(page).to have_content("1 total housing providers")
         end