CLDC-1765 Remove managing organisation for sales log from ui and backend (#1169)

* feat: remove sales log managing_organisation_id

* feat: move shared managing org behaviour from general log to lettings log, update tests

* refator: linting

* revert change to create

* feat: tech review comments

* db:update

* tests: respond to comment

* feat: revert change to avoid nil issue

* test: update

* feat: add validation and update db

* feat: revert last commit

* db:update

* refactor: use safe navigation and lettings?

* refactor: move safe navigator and add shared org_params to log

app/components/log_summary_component.html.erb

@@ -25,7 +25,7 @@
         <% end %>
       </header>
 
-      <% if log.is_a?(LettingsLog) && (log.needstype? or log.startdate?) %>
+      <% if log.lettings? && (log.needstype? or log.startdate?) %>
         <p class="govuk-body govuk-!-margin-bottom-2">
           <% if log.needstype? %>
             <%= log.is_general_needs? ? "General needs" : "Supported housing" %><br>
@@ -37,18 +37,20 @@
       <% end %>
 
       <% if current_user.support? || current_user.organisation.has_managing_agents? %>
-        <% if log.owning_organisation or log.managing_organisation %>
         <dl class="app-metadata">
+          <% if log.owning_organisation %>
             <div class="app-metadata__item">
               <dt class="app-metadata__term">Owned by</dt>
               <dd class="app-metadata__definition"><%= log.owning_organisation&.name %></dd>
             </div>
+          <% end %>
+          <% if log.lettings? && log.managing_organisation %>
             <div class="app-metadata__item">
               <dt class="app-metadata__term">Managed by</dt>
               <dd class="app-metadata__definition"><%= log.managing_organisation&.name %></dd>
             </div>
-          </dl>
           <% end %>
+        </dl>
       <% end %>
     </div>
 

app/controllers/lettings_logs_controller.rb

@@ -101,6 +101,12 @@ class LettingsLogsController < LogsController
     end
   end
 
+  def org_params
+    super.merge(
+      { "managing_organisation_id" => current_user.organisation.id },
+    )
+  end
+
 private
 
   def permitted_log_params

app/controllers/logs_controller.rb

@@ -63,7 +63,6 @@ private
     owning_organisation_id = current_user.organisation.holds_own_stock? ? current_user.organisation.id : nil
     {
       "owning_organisation_id" => owning_organisation_id,
-      "managing_organisation_id" => current_user.organisation.id,
       "created_by_id" => current_user.id,
     }
   end

app/models/lettings_log.rb

@@ -35,6 +35,7 @@ class LettingsLog < Log
 
   belongs_to :scheme, optional: true
   belongs_to :location, optional: true
+  belongs_to :managing_organisation, class_name: "Organisation", optional: true
 
   scope :filter_by_year, ->(year) { where(startdate: Time.zone.local(year.to_i, 4, 1)...Time.zone.local(year.to_i + 1, 4, 1)) }
   scope :filter_by_tenant_code, ->(tenant_code) { where("tenancycode ILIKE ?", "%#{tenant_code}%") }
@@ -50,6 +51,7 @@ class LettingsLog < Log
                     }
   scope :filter_by_before_startdate, ->(date) { where("lettings_logs.startdate >= ?", date) }
   scope :unresolved, -> { where(unresolved: true) }
+  scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org).or(where(managing_organisation: org)) }
 
   AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze
   OPTIONAL_FIELDS = %w[first_time_property_let_as_social_housing tenancycode propcode].freeze
@@ -495,6 +497,18 @@ class LettingsLog < Log
     update(unresolved: false)
   end
 
+  def managing_organisation_provider_type
+    managing_organisation&.provider_type
+  end
+
+  def reset_created_by!
+    return unless updated_by&.support?
+    return if owning_organisation.blank? || managing_organisation.blank? || created_by.blank?
+    return if created_by&.organisation == managing_organisation || created_by&.organisation == owning_organisation
+
+    update!(created_by: nil)
+  end
+
 private
 
   def reset_derived_questions

app/models/log.rb

@@ -2,7 +2,6 @@ class Log < ApplicationRecord
   self.abstract_class = true
 
   belongs_to :owning_organisation, class_name: "Organisation", optional: true
-  belongs_to :managing_organisation, class_name: "Organisation", optional: true
   belongs_to :created_by, class_name: "User", optional: true
   belongs_to :updated_by, class_name: "User", optional: true
   belongs_to :bulk_upload, optional: true
@@ -12,7 +11,6 @@ class Log < ApplicationRecord
   STATUS = { "not_started" => 0, "in_progress" => 1, "completed" => 2 }.freeze
   enum status: STATUS
 
-  scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org).or(where(managing_organisation: org)) }
   scope :filter_by_status, ->(status, _user = nil) { where status: }
   scope :filter_by_years, lambda { |years, _user = nil|
     first_year = years.shift
@@ -44,10 +42,6 @@ class Log < ApplicationRecord
     ethnic_group == 17
   end
 
-  def managing_organisation_provider_type
-    managing_organisation&.provider_type
-  end
-
   def collection_period_open?
     form.end_date > Time.zone.today
   end
@@ -114,14 +108,6 @@ private
     reset_created_by!
   end
 
-  def reset_created_by!
-    return unless updated_by&.support?
-    return if owning_organisation.blank? || managing_organisation.blank? || created_by.blank?
-    return if created_by&.organisation == managing_organisation || created_by&.organisation == owning_organisation
-
-    update!(created_by: nil)
-  end
-
   PIO = PostcodeService.new
 
   def process_previous_postcode_changes!

app/models/organisation.rb

@@ -3,7 +3,6 @@ class Organisation < ApplicationRecord
   has_many :owned_lettings_logs, class_name: "LettingsLog", foreign_key: "owning_organisation_id", dependent: :delete_all
   has_many :managed_lettings_logs, class_name: "LettingsLog", foreign_key: "managing_organisation_id"
   has_many :owned_sales_logs, class_name: "SalesLog", foreign_key: "owning_organisation_id", dependent: :delete_all
-  has_many :managed_sales_logs, class_name: "SalesLog", foreign_key: "managing_organisation_id"
   has_many :data_protection_confirmations
   has_many :organisation_rent_periods
   has_many :owned_schemes, class_name: "Scheme", foreign_key: "owning_organisation_id", dependent: :delete_all

app/models/sales_log.rb

@@ -32,6 +32,7 @@ class SalesLog < Log
 
   scope :filter_by_year, ->(year) { where(saledate: Time.zone.local(year.to_i, 4, 1)...Time.zone.local(year.to_i + 1, 4, 1)) }
   scope :search_by, ->(param) { filter_by_id(param) }
+  scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org) }
 
   OPTIONAL_FIELDS = %w[purchid].freeze
   RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze
@@ -184,6 +185,14 @@ class SalesLog < Log
     process_postcode(postcode_full, "pcodenk", "is_la_inferred", "la")
   end
 
+  def reset_created_by!
+    return unless updated_by&.support?
+    return if owning_organisation.blank? || created_by.blank?
+    return if created_by&.organisation == owning_organisation
+
+    update!(created_by: nil)
+  end
+
   def retirement_age_for_person(person_num)
     gender = public_send("sex#{person_num}".to_sym)
     return unless gender

app/models/user.rb

@@ -10,7 +10,6 @@ class User < ApplicationRecord
   has_many :owned_lettings_logs, through: :organisation
   has_many :managed_lettings_logs, through: :organisation
   has_many :owned_sales_logs, through: :organisation
-  has_many :managed_sales_logs, through: :organisation
   has_many :legacy_users
   has_many :bulk_uploads
 

app/services/filter_service.rb

@@ -17,6 +17,14 @@ class FilterService
       logs = logs.public_send("filter_by_#{category}", values, user)
     end
     logs = logs.order(created_at: :desc)
-    user.support? ? logs.all.includes(:owning_organisation, :managing_organisation) : logs
+    if user.support?
+      if logs.first&.lettings?
+        logs.all.includes(:owning_organisation, :managing_organisation)
+      else
+        logs.all.includes(:owning_organisation)
+      end
+    else
+      logs
+    end
   end
 end

db/migrate/20230110094518_remove_managing_organisation_id_from_sales_logs.rb

@@ -0,0 +1,13 @@
+class RemoveManagingOrganisationIdFromSalesLogs < ActiveRecord::Migration[7.0]
+  def up
+    change_table :sales_logs, bulk: true do |t|
+      t.remove :managing_organisation_id
+    end
+  end
+
+  def down
+    change_table :sales_logs, bulk: true do |t|
+      t.column :managing_organisation_id, :bigint
+    end
+  end
+end

db/schema.rb

@@ -367,7 +367,6 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_18_170602) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.bigint "owning_organisation_id"
-    t.bigint "managing_organisation_id"
     t.bigint "created_by_id"
     t.string "purchid"
     t.integer "type"
@@ -490,23 +489,22 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_18_170602) do
     t.integer "mortgagelender"
     t.string "mortgagelenderother"
     t.integer "mortlen"
-    t.string "pcode1"
-    t.string "pcode2"
-    t.integer "pcodenk"
-    t.string "postcode_full"
-    t.boolean "is_la_inferred"
     t.integer "extrabor"
     t.integer "hhmemb"
     t.integer "totadult"
     t.integer "totchild"
     t.integer "hhtype"
-    t.integer "hodate_check"
+    t.string "pcode1"
+    t.string "pcode2"
+    t.integer "pcodenk"
+    t.string "postcode_full"
+    t.boolean "is_la_inferred"
     t.bigint "bulk_upload_id"
     t.integer "retirement_value_check"
+    t.integer "hodate_check"
     t.integer "extrabor_value_check"
     t.index ["bulk_upload_id"], name: "index_sales_logs_on_bulk_upload_id"
     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"
     t.index ["updated_by_id"], name: "index_sales_logs_on_updated_by_id"
   end

spec/components/log_summary_component_spec.rb

@@ -5,15 +5,16 @@ RSpec.describe LogSummaryComponent, type: :component do
   let(:coordinator_user) { FactoryBot.create(:user) }
   let(:propcode) { "P3647" }
   let(:tenancycode) { "T62863" }
-  let(:log) { FactoryBot.create(:lettings_log, needstype: 1, startdate: Time.utc(2022, 1, 1), tenancycode:, propcode:) }
+  let(:lettings_log) { FactoryBot.create(:lettings_log, needstype: 1, startdate: Time.utc(2022, 1, 1), tenancycode:, propcode:) }
+  let(:sales_log) { FactoryBot.create(:sales_log) }
 
-  context "when rendering log for a support user" do
+  context "when rendering lettings log for a support user" do
     it "show the log summary with organisational relationships" do
-      result = render_inline(described_class.new(current_user: support_user, log:))
+      result = render_inline(described_class.new(current_user: support_user, log: lettings_log))
 
-      expect(result).to have_link(log.id.to_s)
+      expect(result).to have_link(lettings_log.id.to_s)
-      expect(result).to have_text(log.tenancycode)
+      expect(result).to have_text(lettings_log.tenancycode)
-      expect(result).to have_text(log.propcode)
+      expect(result).to have_text(lettings_log.propcode)
       expect(result).to have_text("General needs")
       expect(result).to have_text("Tenancy starts 1 January 2022")
       expect(result).to have_text("Created 8 February 2022")
@@ -23,12 +24,30 @@ RSpec.describe LogSummaryComponent, type: :component do
     end
   end
 
-  context "when rendering log for a data coordinator user" do
+  context "when rendering lettings log for a data coordinator user" do
     it "show the log summary" do
-      result = render_inline(described_class.new(current_user: coordinator_user, log:))
+      result = render_inline(described_class.new(current_user: coordinator_user, log: lettings_log))
 
-      expect(result).not_to have_content("Owned by\n              DLUHC")
+      expect(result).not_to have_content("Owned by")
-      expect(result).not_to have_content("Managed by\n              DLUHC")
+      expect(result).not_to have_content("Managed by")
+    end
+  end
+
+  context "when rendering sales log for a support user" do
+    it "show the log summary with organisational relationships" do
+      result = render_inline(described_class.new(current_user: support_user, log: sales_log))
+
+      expect(result).to have_content("Owned by\n              DLUHC")
+      expect(result).not_to have_content("Managed by")
+    end
+  end
+
+  context "when rendering sales log for a data coordinator user" do
+    it "show the log summary" do
+      result = render_inline(described_class.new(current_user: coordinator_user, log: sales_log))
+
+      expect(result).not_to have_content("Owned by")
+      expect(result).not_to have_content("Managed by")
     end
   end
 end

spec/factories/sales_log.rb

@@ -2,7 +2,6 @@ FactoryBot.define do
   factory :sales_log do
     created_by { FactoryBot.create(:user) }
     owning_organisation { created_by.organisation }
-    managing_organisation { created_by.organisation }
     created_at { Time.utc(2022, 2, 8, 16, 52, 15) }
     updated_at { Time.utc(2022, 2, 8, 16, 52, 15) }
     trait :in_progress do

spec/features/form/check_answers_page_lettings_logs_spec.rb

@@ -143,7 +143,7 @@ RSpec.describe "Lettings Log Check Answers Page" do
     end
 
     context "when the user is checking their answers for the household characteristics subsection" do
-      it "they see a seperate summary card for each member of the household" do
+      it "they see a separate summary card for each member of the household" do
         visit("/lettings-logs/#{completed_lettings_log.id}/#{subsection}/check-answers")
         assert_selector ".x-govuk-summary-card__title", text: "Lead tenant", count: 1
         assert_selector ".x-govuk-summary-card__title", text: "Person 2", count: 1

spec/features/form/check_answers_page_sales_logs_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe "Sales Log Check Answers Page" do
 
     context "when the user is checking their answers for the household characteristics subsection" do
       context "and the log is for a joint purchase" do
-        it "they see a seperate summary card for each member of the household" do
+        it "they see a separate summary card for each member of the household" do
           visit("/sales-logs/#{completed_sales_log_joint_purchase.id}/#{subsection}/check-answers")
           assert_selector ".x-govuk-summary-card__title", text: "Buyer 1", count: 1
           assert_selector ".x-govuk-summary-card__title", text: "Buyer 2", count: 1
@@ -49,7 +49,7 @@ RSpec.describe "Sales Log Check Answers Page" do
       end
 
       context "and the log is for a non-joint purchase" do
-        it "they see a seperate summary card for each member of the household" do
+        it "they see a separate summary card for each member of the household" do
           visit("/sales-logs/#{completed_sales_log_non_joint_purchase.id}/#{subsection}/check-answers")
           assert_selector ".x-govuk-summary-card__title", text: "Buyer 1", count: 1
           assert_selector ".x-govuk-summary-card__title", text: "Buyer 2", count: 0

spec/features/organisation_spec.rb

@@ -215,7 +215,7 @@ RSpec.describe "User Features" do
       let(:number_of_sales_logs) { SalesLog.count }
 
       before do
-        FactoryBot.create_list(:sales_log, 4, owning_organisation_id: organisation.id, managing_organisation_id: organisation.id)
+        FactoryBot.create_list(:sales_log, 4, owning_organisation_id: organisation.id)
         visit("/organisations/#{org_id}/sales-logs")
       end
 

spec/models/lettings_log_spec.rb

@@ -2392,12 +2392,6 @@ RSpec.describe LettingsLog do
         FactoryBot.create(:lettings_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_2, created_by: nil)
       end
 
-      it "filters by given organisation id" do
-        expect(described_class.filter_by_organisation([organisation_1.id]).count).to eq(3)
-        expect(described_class.filter_by_organisation([organisation_1.id, organisation_2.id]).count).to eq(4)
-        expect(described_class.filter_by_organisation([organisation_3.id]).count).to eq(0)
-      end
-
       it "filters by given organisation" do
         expect(described_class.filter_by_organisation([organisation_1]).count).to eq(3)
         expect(described_class.filter_by_organisation([organisation_1, organisation_2]).count).to eq(4)

spec/models/organisation_spec.rb

@@ -196,7 +196,7 @@ RSpec.describe Organisation, type: :model do
       let!(:user) { FactoryBot.create(:user, :support, last_sign_in_at: Time.zone.now, organisation:) }
       let!(:scheme_to_delete) { FactoryBot.create(:scheme, owning_organisation: user.organisation) }
       let!(:log_to_delete) { FactoryBot.create(:lettings_log, owning_organisation: user.organisation) }
-      let!(:sales_log_to_delete) { FactoryBot.create(:sales_log, owning_organisation: user.organisation, managing_organisation: user.organisation) }
+      let!(:sales_log_to_delete) { FactoryBot.create(:sales_log, owning_organisation: user.organisation) }
 
       context "when organisation is deleted" do
         it "child relationships ie logs, schemes and users are deleted too - application" do

spec/models/sales_log_spec.rb

@@ -93,21 +93,14 @@ RSpec.describe SalesLog, type: :model do
     let(:organisation_3) { create(:organisation) }
 
     before do
-      create(:sales_log, :in_progress, owning_organisation: organisation_1, managing_organisation: organisation_1)
+      create(:sales_log, :in_progress, owning_organisation: organisation_1)
-      create(:sales_log, :completed, owning_organisation: organisation_1, managing_organisation: organisation_2)
+      create(:sales_log, :completed, owning_organisation: organisation_1)
-      create(:sales_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_1)
+      create(:sales_log, :completed, owning_organisation: organisation_2)
-      create(:sales_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_2)
-    end
-
-    it "filters by given organisation id" do
-      expect(described_class.filter_by_organisation([organisation_1.id]).count).to eq(3)
-      expect(described_class.filter_by_organisation([organisation_1.id, organisation_2.id]).count).to eq(4)
-      expect(described_class.filter_by_organisation([organisation_3.id]).count).to eq(0)
     end
 
     it "filters by given organisation" do
-      expect(described_class.filter_by_organisation([organisation_1]).count).to eq(3)
+      expect(described_class.filter_by_organisation([organisation_1]).count).to eq(2)
-      expect(described_class.filter_by_organisation([organisation_1, organisation_2]).count).to eq(4)
+      expect(described_class.filter_by_organisation([organisation_1, organisation_2]).count).to eq(3)
       expect(described_class.filter_by_organisation([organisation_3]).count).to eq(0)
     end
   end
@@ -165,7 +158,6 @@ RSpec.describe SalesLog, type: :model do
       FactoryBot.create(
         :sales_log,
         :completed,
-        managing_organisation: owning_organisation,
         owning_organisation:,
         created_by: created_by_user,
         pcodenk: 0,
@@ -297,7 +289,6 @@ RSpec.describe SalesLog, type: :model do
 
     let!(:address_sales_log) do
       described_class.create({
-        managing_organisation: owning_organisation,
         owning_organisation:,
         created_by: created_by_user,
         ppcodenk: 1,

spec/requests/organisations_controller_spec.rb

@@ -734,8 +734,8 @@ RSpec.describe OrganisationsController, type: :request do
           let(:number_of_org2_sales_logs) { 4 }
 
           before do
-            FactoryBot.create_list(:sales_log, number_of_org1_sales_logs, owning_organisation_id: organisation.id, managing_organisation_id: organisation.id)
+            FactoryBot.create_list(:sales_log, number_of_org1_sales_logs, owning_organisation_id: organisation.id)
-            FactoryBot.create_list(:sales_log, number_of_org2_sales_logs, owning_organisation_id: unauthorised_organisation.id, managing_organisation_id: unauthorised_organisation.id)
+            FactoryBot.create_list(:sales_log, number_of_org2_sales_logs, owning_organisation_id: unauthorised_organisation.id)
 
             get "/organisations/#{organisation.id}/sales-logs", headers:, params: {}
           end

spec/requests/sales_logs_controller_spec.rb

@@ -3,7 +3,6 @@ require "rails_helper"
 RSpec.describe SalesLogsController, type: :request do
   let(:user) { FactoryBot.create(:user) }
   let(:owning_organisation) { user.organisation }
-  let(:managing_organisation) { owning_organisation }
   let(:api_username) { "test_user" }
   let(:api_password) { "test_password" }
   let(:basic_credentials) do
@@ -14,7 +13,6 @@ RSpec.describe SalesLogsController, type: :request do
   let(:params) do
     {
       "owning_organisation_id": owning_organisation.id,
-      "managing_organisation_id": managing_organisation.id,
       "created_by_id": user.id,
     }
   end
@@ -51,7 +49,6 @@ RSpec.describe SalesLogsController, type: :request do
       it "creates a sales log with the values passed" do
         json_response = JSON.parse(response.body)
         expect(json_response["owning_organisation_id"]).to eq(owning_organisation.id)
-        expect(json_response["managing_organisation_id"]).to eq(managing_organisation.id)
         expect(json_response["created_by_id"]).to eq(user.id)
       end
 
@@ -94,14 +91,12 @@ RSpec.describe SalesLogsController, type: :request do
       FactoryBot.create(
         :sales_log,
         owning_organisation: organisation,
-        managing_organisation: organisation,
       )
     end
     let!(:unauthorized_sales_log) do
       FactoryBot.create(
         :sales_log,
         owning_organisation: other_organisation,
-        managing_organisation: other_organisation,
       )
     end
 
@@ -119,7 +114,7 @@ RSpec.describe SalesLogsController, type: :request do
         it "does have organisation values" do
           get "/sales-logs", headers: headers, params: {}
           expect(page).to have_content("Owned by")
-          expect(page).to have_content("Managed by")
+          expect(page).not_to have_content("Managed by")
         end
 
         it "shows sales logs for all organisations" do
@@ -146,13 +141,11 @@ RSpec.describe SalesLogsController, type: :request do
             let!(:not_started_sales_log) do
               FactoryBot.create(:sales_log,
                                 owning_organisation: organisation,
-                                managing_organisation: organisation,
                                 created_by: user)
             end
             let!(:completed_sales_log) do
               FactoryBot.create(:sales_log, :completed,
                                 owning_organisation: organisation_2,
-                                managing_organisation: organisation,
                                 created_by: user_2)
             end
 
@@ -189,14 +182,12 @@ RSpec.describe SalesLogsController, type: :request do
             let!(:sales_log_2021) do
               FactoryBot.create(:sales_log, :in_progress,
                                 owning_organisation: organisation,
-                                saledate: Time.zone.local(2022, 3, 1),
+                                saledate: Time.zone.local(2022, 3, 1))
-                                managing_organisation: organisation)
             end
             let!(:sales_log_2022) do
               sales_log = FactoryBot.build(:sales_log, :completed,
                                            owning_organisation: organisation,
-                                           saledate: Time.zone.local(2022, 12, 1),
+                                           saledate: Time.zone.local(2022, 12, 1))
-                                           managing_organisation: organisation)
               sales_log.save!(validate: false)
               sales_log
             end
@@ -227,14 +218,12 @@ RSpec.describe SalesLogsController, type: :request do
               FactoryBot.create(:sales_log, :completed,
                                 owning_organisation: organisation,
                                 saledate: Time.zone.local(2022, 3, 1),
-                                managing_organisation: organisation,
                                 created_by: user)
             end
             let!(:sales_log_2022) do
               FactoryBot.create(:sales_log,
                                 owning_organisation: organisation,
                                 saledate: Time.zone.local(2022, 12, 1),
-                                managing_organisation: organisation,
                                 created_by: user)
             end
 
@@ -362,7 +351,7 @@ RSpec.describe SalesLogsController, type: :request do
 
         context "when there are more than 20 logs" do
           before do
-            FactoryBot.create_list(:sales_log, 25, owning_organisation: organisation, managing_organisation: organisation)
+            FactoryBot.create_list(:sales_log, 25, owning_organisation: organisation)
           end
 
           context "when on the first page" do