From 07f345df71c02504f2a6ca9d58e414ae3cbb9e79 Mon Sep 17 00:00:00 2001
From: natdeanlewissoftwire
 <94526761+natdeanlewissoftwire@users.noreply.github.com>
Date: Wed, 14 Dec 2022 09:33:39 +0000
Subject: [PATCH] CLDC-1730-stock-owning-validation (#1073)

* feat: add error message if org doesn't own stock

* feat: move org relationship errors to model

* feat: use correct model for form

* tests: update params in tests

* feat: add specific fields for parent/child errors

* test: fix params in failing tests

* refactor: unrelated linting

* feat: simplify controller behaviour further

* test: address failing tests

* feat: add copy to en.yml

* feat: add updated validation message and rename related housing provider -> stock owner
---
 .../organisation_relationships_controller.rb  | 115 ++++++++----------
 app/helpers/tasklist_helper.rb                |   2 +-
 app/models/organisation_relationship.rb       |  13 ++
 app/views/devise/unlocks/new.html.erb         |   2 +-
 ...ated_organisation_select_question.html.erb |   2 +-
 .../add_housing_provider.html.erb             |   3 +-
 .../add_managing_agent.html.erb               |   3 +-
 .../managing_agents.html.erb                  |   2 +-
 config/locales/en.yml                         |   7 ++
 db/schema.rb                                  |   2 +-
 ...anisation_relationships_controller_spec.rb |  16 +--
 11 files changed, 88 insertions(+), 79 deletions(-)

diff --git a/app/controllers/organisation_relationships_controller.rb b/app/controllers/organisation_relationships_controller.rb
index e7a6a7856..9aa93c37c 100644
--- a/app/controllers/organisation_relationships_controller.rb
+++ b/app/controllers/organisation_relationships_controller.rb
@@ -5,13 +5,19 @@ class OrganisationRelationshipsController < ApplicationController
   before_action :authenticate_user!
   before_action :authenticate_scope!
 
+  before_action :organisations
+  before_action :target_organisation, only: %i[
+    remove_housing_provider
+    remove_managing_agent
+    delete_housing_provider
+    delete_managing_agent
+  ]
+
   def housing_providers
     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 :html
+
     @pagy, @housing_providers = pagy(unpaginated_filtered_housing_providers)
-    @organisations = organisations
     @searched = search_term.presence
     @total_count = housing_providers.size
   end
@@ -19,107 +25,84 @@ class OrganisationRelationshipsController < ApplicationController
   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 :html
+
     @pagy, @managing_agents = pagy(unpaginated_filtered_managing_agents)
-    @organisations = organisations
     @searched = search_term.presence
     @total_count = managing_agents.size
   end
 
   def add_housing_provider
-    @organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name)
-    respond_to :html
+    @organisation_relationship = organisation.parent_organisation_relationships.new
   end
 
   def add_managing_agent
-    @organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name)
-    respond_to :html
+    @organisation_relationship = organisation.child_organisation_relationships.new
   end
 
   def create_housing_provider
-    child_organisation = @organisation
-    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)
-      render "organisation_relationships/add_housing_provider"
-      return
+    @organisation_relationship = organisation.parent_organisation_relationships.new(organisation_relationship_params)
+    if @organisation_relationship.save(context: :housing_provider)
+      flash[:notice] = "#{@organisation_relationship.parent_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} housing providers"
+      redirect_to housing_providers_organisation_path
     else
-      parent_organisation = related_organisation
-      if OrganisationRelationship.exists?(child_organisation:, parent_organisation:)
-        @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
+      @organisations = Organisation.where.not(id: organisation.id).pluck(:id, :name)
+      render "organisation_relationships/add_housing_provider", status: :unprocessable_entity
     end
-    create!(child_organisation:, parent_organisation:)
-    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 = @organisation
-    if params[:organisation][: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)
-      render "organisation_relationships/add_managing_agent"
-      return
+    @organisation_relationship = organisation.child_organisation_relationships.new(organisation_relationship_params)
+    if @organisation_relationship.save
+      flash[:notice] = "#{@organisation_relationship.child_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} managing agents"
+      redirect_to managing_agents_organisation_path
     else
-      child_organisation = related_organisation
-      if OrganisationRelationship.exists?(child_organisation:, parent_organisation:)
-        @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
+      @organisations = Organisation.where.not(id: organisation.id).pluck(:id, :name)
+      render "organisation_relationships/add_managing_agent", status: :unprocessable_entity
     end
-    create!(child_organisation:, parent_organisation:)
-    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
+  def remove_housing_provider; end
 
   def delete_housing_provider
-    relationship = OrganisationRelationship.find_by!(
-      child_organisation: @organisation,
+    OrganisationRelationship.find_by!(
+      child_organisation: organisation,
       parent_organisation: target_organisation,
-    )
-    relationship.destroy!
+    ).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
 
-  def remove_managing_agent
-    @target_organisation_id = target_organisation.id
-  end
+  def remove_managing_agent; end
 
   def delete_managing_agent
-    relationship = OrganisationRelationship.find_by!(
-      parent_organisation: @organisation,
+    OrganisationRelationship.find_by!(
+      parent_organisation: organisation,
       child_organisation: target_organisation,
-    )
-    relationship.destroy!
+    ).destroy!
     flash[:notice] = "#{target_organisation.name} is no longer one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} managing agents"
     redirect_to managing_agents_organisation_path
   end
 
 private
 
-  def create!(child_organisation:, parent_organisation:)
-    @resource = OrganisationRelationship.new(child_organisation:, parent_organisation:)
-    @resource.save!
+  def organisation
+    @organisation ||= if current_user.support?
+                        Organisation.find(params[:id])
+                      else
+                        current_user.organisation
+                      end
   end
 
-  def organisation
-    @organisation ||= Organisation.find(params[:id])
+  def organisations
+    @organisations ||= Organisation.where.not(id: organisation.id).pluck(:id, :name)
+  end
+
+  def parent_organisation
+    @parent_organisation ||= Organisation.find(params[:organisation_relationship][:parent_organisation_id])
   end
 
-  def related_organisation
-    @related_organisation ||= Organisation.find(params[:organisation][:related_organisation_id])
+  def child_organisation
+    @child_organisation ||= Organisation.find(params[:organisation_relationship][:child_organisation_id])
   end
 
   def target_organisation
@@ -130,8 +113,12 @@ private
     params["search"]
   end
 
+  def organisation_relationship_params
+    params.require(:organisation_relationship).permit(:parent_organisation_id, :child_organisation_id)
+  end
+
   def authenticate_scope!
-    if current_user.organisation != organisation && !current_user.support?
+    if current_user.organisation != Organisation.find(params[:id]) && !current_user.support?
       render_not_found
     end
   end
diff --git a/app/helpers/tasklist_helper.rb b/app/helpers/tasklist_helper.rb
index 05c84d1b7..f297de2ff 100644
--- a/app/helpers/tasklist_helper.rb
+++ b/app/helpers/tasklist_helper.rb
@@ -39,7 +39,7 @@ module TasklistHelper
 
   def review_log_text(log)
     if log.collection_period_open?
-      "You can #{govuk_link_to 'review and make changes to this log', review_lettings_log_path(log)} until #{(log.form.end_date).to_formatted_s(:govuk_date)}.".html_safe
+      "You can #{govuk_link_to 'review and make changes to this log', review_lettings_log_path(log)} until #{log.form.end_date.to_formatted_s(:govuk_date)}.".html_safe
     else
       "This log is from the #{log.form.start_date.year}/#{log.form.start_date.year + 1} collection window, which is now closed."
     end
diff --git a/app/models/organisation_relationship.rb b/app/models/organisation_relationship.rb
index 034fc5d0e..7f3a85cae 100644
--- a/app/models/organisation_relationship.rb
+++ b/app/models/organisation_relationship.rb
@@ -1,4 +1,17 @@
 class OrganisationRelationship < ApplicationRecord
   belongs_to :child_organisation, class_name: "Organisation"
   belongs_to :parent_organisation, class_name: "Organisation"
+  validates :parent_organisation_id, presence: { message: I18n.t("validations.organisation.housing_provider.blank") }
+  validates :child_organisation_id, presence: { message: I18n.t("validations.organisation.managing_agent.blank") }
+  validates :parent_organisation_id, uniqueness: { scope: :child_organisation_id, message: I18n.t("validations.organisation.housing_provider.already_added") }
+  validates :child_organisation_id, uniqueness: { scope: :parent_organisation_id, message: I18n.t("validations.organisation.managing_agent.already_added") }
+  validate :validate_housing_provider_owns_stock, on: :housing_provider
+
+private
+
+  def validate_housing_provider_owns_stock
+    if parent_organisation_id.present? && !parent_organisation.holds_own_stock
+      errors.add :parent_organisation_id, I18n.t("validations.organisation.housing_provider.does_not_own_stock")
+    end
+  end
 end
diff --git a/app/views/devise/unlocks/new.html.erb b/app/views/devise/unlocks/new.html.erb
index 4f0de4c0b..e5ad2b7b0 100644
--- a/app/views/devise/unlocks/new.html.erb
+++ b/app/views/devise/unlocks/new.html.erb
@@ -1,7 +1,7 @@
 <h2>Resend unlock instructions</h2>
 
 <%= form_for(resource, as: resource_name, url: unlock_path(resource_name), html: { method: :post }) do |f| %>
-  <%= render "devise/shared/error_messages", resource: resource %>
+  <%= render "devise/shared/error_messages", resource: %>
 
   <%= f.govuk_email_field :email,
     label: { text: "Email address" },
diff --git a/app/views/organisation_relationships/_related_organisation_select_question.html.erb b/app/views/organisation_relationships/_related_organisation_select_question.html.erb
index cc27c2b8b..551bce5ed 100644
--- a/app/views/organisation_relationships/_related_organisation_select_question.html.erb
+++ b/app/views/organisation_relationships/_related_organisation_select_question.html.erb
@@ -1,3 +1,3 @@
 <% answers = question.answer_options.map { |key, value| OpenStruct.new(id: key, name: value) } %>
-<%= f.govuk_collection_select :related_organisation_id, answers, :id, :name, label: { hidden: true }, "data-controller": "accessible-autocomplete" do %>
+<%= f.govuk_collection_select field, answers, :id, :name, label: { hidden: true }, "data-controller": "accessible-autocomplete" do %>
 <% end %>
diff --git a/app/views/organisation_relationships/add_housing_provider.html.erb b/app/views/organisation_relationships/add_housing_provider.html.erb
index 3cce90db1..cb95fd6ac 100644
--- a/app/views/organisation_relationships/add_housing_provider.html.erb
+++ b/app/views/organisation_relationships/add_housing_provider.html.erb
@@ -1,4 +1,4 @@
-<%= form_with model: @organisation, url: housing_providers_organisation_path, method: "post", local: true do |f| %>
+<%= form_with model: @organisation_relationship, url: housing_providers_organisation_path, method: "post", 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)) %>
@@ -18,6 +18,7 @@
     <% answer_options[organisation[0]] = organisation[1] %>
   <% end %>
   <%= render partial: "organisation_relationships/related_organisation_select_question", locals: {
+    field: :parent_organisation_id,
     question: Form::Question.new("", { "answer_options" => answer_options }, nil),
     f:,
   } %>
diff --git a/app/views/organisation_relationships/add_managing_agent.html.erb b/app/views/organisation_relationships/add_managing_agent.html.erb
index bf5c380d5..173e845ad 100644
--- a/app/views/organisation_relationships/add_managing_agent.html.erb
+++ b/app/views/organisation_relationships/add_managing_agent.html.erb
@@ -1,4 +1,4 @@
-<%= form_with model: @organisation, url: managing_agents_organisation_path, method: "post", local: true do |f| %>
+<%= form_with model: @organisation_relationship, url: managing_agents_organisation_path, method: "post", 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)) %>
@@ -18,6 +18,7 @@
     <% answer_options[organisation[0]] = organisation[1] %>
   <% end %>
   <%= render partial: "organisation_relationships/related_organisation_select_question", locals: {
+    field: :child_organisation_id,
     question: Form::Question.new("", { "answer_options" => answer_options }, nil),
     f:,
   } %>
diff --git a/app/views/organisation_relationships/managing_agents.html.erb b/app/views/organisation_relationships/managing_agents.html.erb
index 2f17f0a7e..ae84d8eac 100644
--- a/app/views/organisation_relationships/managing_agents.html.erb
+++ b/app/views/organisation_relationships/managing_agents.html.erb
@@ -11,7 +11,7 @@
     <p class="govuk-body">This organisation does not currently have any managing agents.</p>
   <% end %>
 <% else %>
-  <%= render partial: "organisations/headings", locals: { main: "This organisation managing agents", sub: current_user.organisation.name } %>
+  <%= render partial: "organisations/headings", locals: { main: "Your managing agents", sub: current_user.organisation.name } %>
   <p class="govuk-body">A managing agent can submit logs for this organisation.</p>
   <% if @total_count == 0 %>
     <p class="govuk-body">This organisation does not currently have any managing agents.</p>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 05a828dea..fae17aa0f 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -116,6 +116,13 @@ en:
     organisation:
       name_missing: "Enter the name of the organisation"
       provider_type_missing: "Select the organisation type"
+      housing_provider:
+        blank: "You must choose a stock owner"
+        already_added: "You have already added this stock owner"
+        does_not_own_stock: "You can only add stock owners who own stock, which this organisation does not."
+      managing_agent:
+        blank: "You must choose a managing agent"
+        already_added: "You have already added this managing agent"
     not_answered: "You must answer %{question}"
 
     other_field_missing: "If %{main_field_label} is other then %{other_field_label} must be provided"
diff --git a/db/schema.rb b/db/schema.rb
index f16ab6d9d..9beffb241 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -399,9 +399,9 @@ ActiveRecord::Schema[7.0].define(version: 2022_12_12_161657) do
     t.integer "inc1mort"
     t.integer "income2"
     t.integer "income2nk"
-    t.integer "prevown"
     t.integer "savingsnk"
     t.integer "savings"
+    t.integer "prevown"
     t.string "sex3"
     t.integer "details_known_1"
     t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id"
diff --git a/spec/requests/organisation_relationships_controller_spec.rb b/spec/requests/organisation_relationships_controller_spec.rb
index 99c57a01a..47593d1c0 100644
--- a/spec/requests/organisation_relationships_controller_spec.rb
+++ b/spec/requests/organisation_relationships_controller_spec.rb
@@ -139,8 +139,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         let(:params) do
           {
-            "organisation": {
-              "related_organisation_id": housing_provider.id,
+            "organisation_relationship": {
+              "parent_organisation_id": housing_provider.id,
             },
           }
         end
@@ -167,8 +167,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         let(:params) do
           {
-            "organisation": {
-              "related_organisation_id": managing_agent.id,
+            "organisation_relationship": {
+              "child_organisation_id": managing_agent.id,
             },
           }
         end
@@ -368,8 +368,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         let(:params) do
           {
-            "organisation": {
-              "related_organisation_id": housing_provider.id,
+            "organisation_relationship": {
+              "parent_organisation_id": housing_provider.id,
             },
           }
         end
@@ -396,8 +396,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
 
         let(:params) do
           {
-            "organisation": {
-              "related_organisation_id": managing_agent.id,
+            "organisation_relationship": {
+              "child_organisation_id": managing_agent.id,
             },
           }
         end