From 6d7ce8af16bb26a0a7da3ee327432708c8debcc7 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Tue, 13 Jun 2023 14:04:36 +0100 Subject: [PATCH] CLDC-2319 prevent log creation (#1684) * Remove bulk_upload_lettings_logs flag * Add spacing above actions in data sharing agreement * Add banner * Add model validation * fix log component * Fix factory * Put validation behind feature flag * start adapting to data protection confirmation * Make tests more generic * Add tests for display_organisation_attributes * Use data_protection_confirmed? method * Address code review * rubocop * Prevent access to bulk upload pages when DPC not signed * Address PO review --- .../create_log_actions_component.html.erb | 10 ++ .../create_log_actions_component.rb | 55 ++++++ ...ion_confirmation_banner_component.html.erb | 16 ++ ...rotection_confirmation_banner_component.rb | 38 ++++ .../bulk_upload_lettings_logs_controller.rb | 7 + .../bulk_upload_sales_logs_controller.rb | 7 + app/controllers/organisations_controller.rb | 44 ++--- app/helpers/data_sharing_agreement_helper.rb | 34 ++-- app/helpers/logs_helper.rb | 7 - app/models/data_sharing_agreement.rb | 4 - app/models/log.rb | 9 + app/models/organisation.rb | 15 +- app/services/feature_toggle.rb | 6 +- app/views/layouts/application.html.erb | 2 +- .../logs/_create_for_org_actions.html.erb | 10 ++ ..._log_filters.erb => _log_filters.html.erb} | 12 +- app/views/logs/index.html.erb | 30 ++-- .../data_sharing_agreement.html.erb | 20 +-- app/views/organisations/logs.html.erb | 16 +- app/views/organisations/show.html.erb | 2 +- config/locales/en.yml | 2 +- ...01144_drop_data_sharing_adreement_table.rb | 5 + db/schema.rb | 18 +- db/seeds.rb | 12 ++ .../create_log_actions_component_spec.rb | 170 ++++++++++++++++++ ...tion_confirmation_banner_component_spec.rb | 128 +++++++++++++ .../controllers/modules/search_filter_spec.rb | 2 +- spec/db/seeds_spec.rb | 12 +- .../factories/data_protection_confirmation.rb | 5 +- spec/factories/data_sharing_agreement.rb | 15 -- spec/factories/organisation.rb | 22 +++ spec/features/lettings_log_spec.rb | 6 +- .../lettings/questions/created_by_id_spec.rb | 26 +-- .../sales/questions/created_by_id_spec.rb | 27 ++- spec/models/organisation_spec.rb | 111 ++++++++---- spec/models/user_spec.rb | 54 +++--- ...lk_upload_lettings_logs_controller_spec.rb | 13 +- .../bulk_upload_sales_logs_controller_spec.rb | 13 +- .../requests/organisations_controller_spec.rb | 71 ++++---- spec/requests/users_controller_spec.rb | 20 ++- ...ection_confirmation_import_service_spec.rb | 10 +- spec/shared/shared_log_examples.rb | 49 +++++ .../_create_for_org_actions.html.erb_spec.rb | 50 ++++++ .../data_sharing_agreement.html.erb_spec.rb | 42 +++-- .../views/organisations/show.html.erb_spec.rb | 41 ++--- 45 files changed, 934 insertions(+), 334 deletions(-) create mode 100644 app/components/create_log_actions_component.html.erb create mode 100644 app/components/create_log_actions_component.rb create mode 100644 app/components/data_protection_confirmation_banner_component.html.erb create mode 100644 app/components/data_protection_confirmation_banner_component.rb delete mode 100644 app/models/data_sharing_agreement.rb create mode 100644 app/views/logs/_create_for_org_actions.html.erb rename app/views/logs/{_log_filters.erb => _log_filters.html.erb} (93%) create mode 100644 db/migrate/20230609101144_drop_data_sharing_adreement_table.rb create mode 100644 spec/components/create_log_actions_component_spec.rb create mode 100644 spec/components/data_protection_confirmation_banner_component_spec.rb delete mode 100644 spec/factories/data_sharing_agreement.rb create mode 100644 spec/views/logs/_create_for_org_actions.html.erb_spec.rb diff --git a/app/components/create_log_actions_component.html.erb b/app/components/create_log_actions_component.html.erb new file mode 100644 index 000000000..1b6bd8aca --- /dev/null +++ b/app/components/create_log_actions_component.html.erb @@ -0,0 +1,10 @@ +<% if display_actions? %> +
+ <% if create_button_href.present? %> + <%= govuk_button_to create_button_copy, create_button_href, class: "govuk-!-margin-right-6" %> + <% end %> + <% if upload_button_href.present? %> + <%= govuk_button_link_to upload_button_copy, upload_button_href, secondary: true %> + <% end %> +
+<% end %> diff --git a/app/components/create_log_actions_component.rb b/app/components/create_log_actions_component.rb new file mode 100644 index 000000000..af8ed9d89 --- /dev/null +++ b/app/components/create_log_actions_component.rb @@ -0,0 +1,55 @@ +class CreateLogActionsComponent < ViewComponent::Base + include Rails.application.routes.url_helpers + + attr_reader :bulk_upload, :user, :log_type + + def initialize(user:, log_type:, bulk_upload: nil) + @bulk_upload = bulk_upload + @user = user + @log_type = log_type + + super + end + + def display_actions? + return false if bulk_upload.present? + return true unless FeatureToggle.new_data_protection_confirmation? + return true if user.support? + + user.organisation.data_protection_confirmed? + end + + def create_button_href + case log_type + when "lettings" + lettings_logs_path + when "sales" + sales_logs_path + end + end + + def create_button_copy + case log_type + when "lettings" + "Create a new lettings log" + when "sales" + "Create a new sales log" + end + end + + def upload_button_copy + if log_type == "lettings" + "Upload lettings logs in bulk" + elsif FeatureToggle.bulk_upload_sales_logs? && log_type == "sales" + "Upload sales logs in bulk" + end + end + + def upload_button_href + if log_type == "lettings" + bulk_upload_lettings_log_path(id: "start") + elsif FeatureToggle.bulk_upload_sales_logs? && log_type == "sales" + bulk_upload_sales_log_path(id: "start") + end + end +end diff --git a/app/components/data_protection_confirmation_banner_component.html.erb b/app/components/data_protection_confirmation_banner_component.html.erb new file mode 100644 index 000000000..8f75081a1 --- /dev/null +++ b/app/components/data_protection_confirmation_banner_component.html.erb @@ -0,0 +1,16 @@ +<% if display_banner? %> + <%= govuk_notification_banner(title_text: "Important") do %> +

+ <%= I18n.t("validations.organisation.data_sharing_agreement_not_signed") %> +

+ <% if user.is_dpo? %> + <%= govuk_link_to( + "Read the Data Sharing Agreement", + data_sharing_agreement_href, + class: "govuk-notification-banner__link govuk-!-font-weight-bold", + ) %> + <% else %> + <%= data_protection_officers_text %> + <% end %> + <% end %> +<% end %> diff --git a/app/components/data_protection_confirmation_banner_component.rb b/app/components/data_protection_confirmation_banner_component.rb new file mode 100644 index 000000000..b1dc61152 --- /dev/null +++ b/app/components/data_protection_confirmation_banner_component.rb @@ -0,0 +1,38 @@ +class DataProtectionConfirmationBannerComponent < ViewComponent::Base + include Rails.application.routes.url_helpers + + attr_reader :user, :organisation + + def initialize(user:, organisation: nil) + @user = user + @organisation = organisation + + super + end + + def display_banner? + return false unless FeatureToggle.new_data_protection_confirmation? + return false if user.support? && organisation.blank? + + !DataProtectionConfirmation.exists?( + organisation: org_or_user_org, + confirmed: true, + ) + end + + def data_protection_officers_text + if org_or_user_org.data_protection_officers.any? + "You can ask: #{org_or_user_org.data_protection_officers.map(&:name).join(', ')}" + end + end + + def data_sharing_agreement_href + data_sharing_agreement_organisation_path(org_or_user_org) + end + +private + + def org_or_user_org + organisation.presence || user.organisation + end +end diff --git a/app/controllers/bulk_upload_lettings_logs_controller.rb b/app/controllers/bulk_upload_lettings_logs_controller.rb index 40f898012..1d011dcca 100644 --- a/app/controllers/bulk_upload_lettings_logs_controller.rb +++ b/app/controllers/bulk_upload_lettings_logs_controller.rb @@ -1,5 +1,6 @@ class BulkUploadLettingsLogsController < ApplicationController before_action :authenticate_user! + before_action :validate_data_protection_agrement_signed! def start if in_crossover_period? @@ -23,6 +24,12 @@ class BulkUploadLettingsLogsController < ApplicationController private + def validate_data_protection_agrement_signed! + unless @current_user.organisation.data_protection_confirmed? + redirect_to lettings_logs_path + end + end + def current_year FormHandler.instance.current_collection_start_year end diff --git a/app/controllers/bulk_upload_sales_logs_controller.rb b/app/controllers/bulk_upload_sales_logs_controller.rb index 85be7e96d..aa865f0c7 100644 --- a/app/controllers/bulk_upload_sales_logs_controller.rb +++ b/app/controllers/bulk_upload_sales_logs_controller.rb @@ -1,5 +1,6 @@ class BulkUploadSalesLogsController < ApplicationController before_action :authenticate_user! + before_action :validate_data_protection_agrement_signed! def start if in_crossover_period? @@ -23,6 +24,12 @@ class BulkUploadSalesLogsController < ApplicationController private + def validate_data_protection_agrement_signed! + unless @current_user.organisation.data_protection_confirmed? + redirect_to sales_logs_path + end + end + def current_year FormHandler.instance.forms["current_sales"].start_date.year end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index a1f8c595d..7fdcd1d94 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -157,35 +157,35 @@ class OrganisationsController < ApplicationController end def data_sharing_agreement - return render_not_found unless FeatureToggle.new_data_sharing_agreement? + return render_not_found unless FeatureToggle.new_data_protection_confirmation? - @data_sharing_agreement = current_user.organisation.data_sharing_agreement + @data_protection_confirmation = current_user.organisation.data_protection_confirmation end def confirm_data_sharing_agreement - return render_not_found unless FeatureToggle.new_data_sharing_agreement? + return render_not_found unless FeatureToggle.new_data_protection_confirmation? return render_not_found unless current_user.is_dpo? - return render_not_found if @organisation.data_sharing_agreement.present? - - data_sharing_agreement = DataSharingAgreement.new( - organisation: current_user.organisation, - signed_at: Time.zone.now, - data_protection_officer: current_user, - organisation_name: @organisation.name, - organisation_address: @organisation.address_row, - organisation_phone_number: @organisation.phone, - dpo_email: current_user.email, - dpo_name: current_user.name, - ) - - if data_sharing_agreement.save - flash[:notice] = "You have accepted the Data Sharing Agreement" - flash[:notification_banner_body] = "Your organisation can now submit logs." - - redirect_to details_organisation_path(@organisation) + return render_not_found if @organisation.data_protection_confirmed? + + if @organisation.data_protection_confirmation + @organisation.data_protection_confirmation.update!( + confirmed: true, + data_protection_officer: current_user, + # When it was signed + created_at: Time.zone.now, + ) else - render :data_sharing_agreement + DataProtectionConfirmation.create!( + organisation: current_user.organisation, + confirmed: true, + data_protection_officer: current_user, + ) end + + flash[:notice] = "You have accepted the Data Sharing Agreement" + flash[:notification_banner_body] = "Your organisation can now submit logs." + + redirect_to details_organisation_path(@organisation) end private diff --git a/app/helpers/data_sharing_agreement_helper.rb b/app/helpers/data_sharing_agreement_helper.rb index d8949de4f..e79819ffb 100644 --- a/app/helpers/data_sharing_agreement_helper.rb +++ b/app/helpers/data_sharing_agreement_helper.rb @@ -21,9 +21,9 @@ module DataSharingAgreementHelper end end - def name_for_data_sharing_agreement(data_sharing_agreement, user) - if data_sharing_agreement.present? - data_sharing_agreement.data_protection_officer.name + def name_for_data_sharing_agreement(data_protection_confirmation, user) + if data_protection_confirmation&.confirmed? + data_protection_confirmation.data_protection_officer.name elsif user.is_dpo? user.name else @@ -31,22 +31,22 @@ module DataSharingAgreementHelper end end - def org_name_for_data_sharing_agreement(data_sharing_agreement, user) - if data_sharing_agreement.present? - data_sharing_agreement.organisation_name + def org_name_for_data_sharing_agreement(data_protection_confirmation, user) + if data_protection_confirmation&.confirmed? + data_protection_confirmation.organisation.name else user.organisation.name end end # rubocop:disable Rails/HelperInstanceVariable - def section_12_2(data_sharing_agreement:, user:, organisation:) - if data_sharing_agreement - @org_address = data_sharing_agreement.organisation_address - @org_name = data_sharing_agreement.organisation_name - @org_phone = data_sharing_agreement.organisation_phone_number - @dpo_name = data_sharing_agreement.dpo_name - @dpo_email = data_sharing_agreement.dpo_email + def present_section_12_2(data_protection_confirmation:, user:, organisation:) + if data_protection_confirmation&.confirmed? + @org_address = data_protection_confirmation.organisation.address_row + @org_name = data_protection_confirmation.organisation.name + @org_phone = data_protection_confirmation.organisation.phone + @dpo_name = data_protection_confirmation.data_protection_officer.name + @dpo_email = data_protection_confirmation.data_protection_officer.email else @org_name = organisation.name @org_address = organisation.address_row @@ -68,18 +68,18 @@ module DataSharingAgreementHelper private def data_sharing_agreement_first_line(organisation:, user:) - return "Not accepted" if organisation.data_sharing_agreement.blank? + return "Not accepted" unless organisation.data_protection_confirmed? if user.support? - "Accepted #{organisation.data_sharing_agreement.signed_at.strftime('%d/%m/%Y')}" + "Accepted #{organisation.data_protection_confirmation.created_at.strftime('%d/%m/%Y')}" else "Accepted" end end def data_sharing_agreement_second_line(organisation:, user:) - if organisation.data_sharing_agreement.present? - organisation.data_sharing_agreement.data_protection_officer.name if user.support? + if organisation.data_protection_confirmed? + organisation.data_protection_confirmation.data_protection_officer.name if user.support? else "Data protection officer must sign" unless user.is_dpo? end diff --git a/app/helpers/logs_helper.rb b/app/helpers/logs_helper.rb index ca6fa838f..becc2d213 100644 --- a/app/helpers/logs_helper.rb +++ b/app/helpers/logs_helper.rb @@ -8,13 +8,6 @@ module LogsHelper end end - def bulk_upload_path_for_controller(controller, id:) - case log_type_for_controller(controller) - when "lettings" then bulk_upload_lettings_log_path(id:) - when "sales" then bulk_upload_sales_log_path(id:) - end - end - def bulk_upload_options(bulk_upload) array = bulk_upload ? [bulk_upload.id] : [] array.index_with { |_bulk_upload_id| "With logs from bulk upload" } diff --git a/app/models/data_sharing_agreement.rb b/app/models/data_sharing_agreement.rb deleted file mode 100644 index 7b366c9fe..000000000 --- a/app/models/data_sharing_agreement.rb +++ /dev/null @@ -1,4 +0,0 @@ -class DataSharingAgreement < ApplicationRecord - belongs_to :organisation - belongs_to :data_protection_officer, class_name: "User" -end diff --git a/app/models/log.rb b/app/models/log.rb index 09c760934..452aa67c6 100644 --- a/app/models/log.rb +++ b/app/models/log.rb @@ -9,6 +9,7 @@ class Log < ApplicationRecord belongs_to :bulk_upload, optional: true before_save :update_status! + before_validation :verify_data_protection_confirmation, on: :create STATUS = { "not_started" => 0, @@ -177,6 +178,14 @@ class Log < ApplicationRecord private + def verify_data_protection_confirmation + return unless FeatureToggle.new_data_protection_confirmation? + return unless owning_organisation + return if owning_organisation.data_protection_confirmed? + + errors.add :owning_organisation, I18n.t("validations.organisation.data_sharing_agreement_not_signed") + end + # Handle logs that are older than previous collection start date def older_than_previous_collection_year? return false unless startdate diff --git a/app/models/organisation.rb b/app/models/organisation.rb index cd2fd8c38..150f3c69b 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -1,10 +1,10 @@ class Organisation < ApplicationRecord has_many :users, dependent: :delete_all + has_many :data_protection_officers, -> { where(is_dpo: true) }, class_name: "User" 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 :data_protection_confirmations - has_one :data_sharing_agreement + has_one :data_protection_confirmation has_many :organisation_rent_periods has_many :owned_schemes, class_name: "Scheme", foreign_key: "owning_organisation_id", dependent: :delete_all has_many :parent_organisation_relationships, foreign_key: :child_organisation_id, class_name: "OrganisationRelationship" @@ -89,7 +89,7 @@ class Organisation < ApplicationRecord end def data_protection_confirmed? - !!data_protection_confirmations.order(created_at: :desc).first&.confirmed + !!data_protection_confirmation&.confirmed? end def data_protection_agreement_string @@ -103,7 +103,7 @@ class Organisation < ApplicationRecord end def display_organisation_attributes - [ + attrs = [ { name: "Name", value: name, editable: true }, { name: "Address", value: address_string, editable: true }, { name: "Telephone_number", value: phone, editable: true }, @@ -111,8 +111,13 @@ class Organisation < ApplicationRecord { name: "Registration number", value: housing_registration_no || "", editable: false }, { name: "Rent_periods", value: rent_period_labels, editable: false, format: :bullet }, { name: "Owns housing stock", value: holds_own_stock ? "Yes" : "No", editable: false }, - { name: "Data protection agreement", value: data_protection_agreement_string, editable: false }, ].compact + + unless FeatureToggle.new_data_protection_confirmation? + attrs << { name: "Data protection agreement", value: data_protection_agreement_string, editable: false } + end + + attrs end def has_managing_agents? diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index 1b6ae4f42..edaa02fd7 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -20,10 +20,6 @@ class FeatureToggle true end - def self.bulk_upload_lettings_logs? - true - end - def self.bulk_upload_sales_logs? !Rails.env.production? end @@ -46,7 +42,7 @@ class FeatureToggle !Rails.env.production? end - def self.new_data_sharing_agreement? + def self.new_data_protection_confirmation? !Rails.env.production? end end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index d86a622f6..ead90793a 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -126,7 +126,7 @@ ) do |notification_banner| notification_banner.heading(text: flash.notice.html_safe) if flash[:notification_banner_body] - tag.p flash[:notification_banner_body] + tag.p flash[:notification_banner_body]&.html_safe end end %> <% end %> diff --git a/app/views/logs/_create_for_org_actions.html.erb b/app/views/logs/_create_for_org_actions.html.erb new file mode 100644 index 000000000..dc0469c44 --- /dev/null +++ b/app/views/logs/_create_for_org_actions.html.erb @@ -0,0 +1,10 @@ +

+ <% if !FeatureToggle.new_data_protection_confirmation? || @organisation.data_protection_confirmed? %> + <% if current_page?(controller: 'organisations', action: 'lettings_logs') %> + <%= govuk_button_to "Create a new lettings log for this organisation", lettings_logs_path(lettings_log: { owning_organisation_id: @organisation.id }, method: :post) %> + <% end %> + <% if current_page?(controller: 'organisations', action: 'sales_logs') %> + <%= govuk_button_to "Create a new sales log for this organisation", sales_logs_path(sales_log: { owning_organisation_id: @organisation.id }, method: :post) %> + <% end %> + <% end %> +
diff --git a/app/views/logs/_log_filters.erb b/app/views/logs/_log_filters.html.erb similarity index 93% rename from app/views/logs/_log_filters.erb rename to app/views/logs/_log_filters.html.erb index 78d59b33b..d8235d350 100644 --- a/app/views/logs/_log_filters.erb +++ b/app/views/logs/_log_filters.html.erb @@ -6,12 +6,12 @@
<%= form_with html: { method: :get } do |f| %> - <% all_or_yours = { "all": { label: "All" }, "yours": { label: "Yours" }} %> + <% all_or_yours = { "all": { label: "All" }, "yours": { label: "Yours" } } %> <% if bulk_upload_options(@bulk_upload).present? %> <%= render partial: "filters/checkbox_filter", locals: { - f: f, + f:, options: bulk_upload_options(@bulk_upload), label: "Bulk upload", category: "bulk_upload_id", @@ -21,7 +21,7 @@ <% if bulk_upload_options(@bulk_upload).blank? %> <%= render partial: "filters/checkbox_filter", locals: { - f: f, + f:, options: collection_year_options, label: "Collection year", category: "years", @@ -29,7 +29,7 @@ <%= render partial: "filters/checkbox_filter", locals: { - f: f, + f:, options: status_filters, label: "Status", category: "status", @@ -38,7 +38,7 @@ <%= render partial: "filters/radio_filter", locals: { - f: f, + f:, options: all_or_yours, label: "Logs", category: "user", @@ -46,7 +46,7 @@ <% if (@current_user.support? || @current_user.organisation.has_managing_agents?) && request.path == "/lettings-logs" %> <%= render partial: "filters/radio_filter", locals: { - f: f, + f:, options: { "all": { label: "All" }, "specific_org": { diff --git a/app/views/logs/index.html.erb b/app/views/logs/index.html.erb index 637d985fa..e2cc2e0d1 100644 --- a/app/views/logs/index.html.erb +++ b/app/views/logs/index.html.erb @@ -3,6 +3,11 @@ <% content_for :title, title %> +<%= render DataProtectionConfirmationBannerComponent.new( + user: current_user, + organisation: @organisation, +) %> + <% if current_page?(controller: 'lettings_logs', action: 'index') %> <% if @unresolved_count > 0 %> <%= govuk_notification_banner( @@ -45,26 +50,11 @@ <% end %>
- <% unless @bulk_upload %> -
- <% if current_page?(controller: 'lettings_logs', action: 'index') %> - <%= govuk_button_to "Create a new lettings log", lettings_logs_path, class: "govuk-!-margin-right-6" %> - <% end %> - - <% if current_page?(controller: 'sales_logs', action: 'index') %> - <%= govuk_button_to "Create a new sales log", sales_logs_path, class: "govuk-!-margin-right-6" %> - <% end %> - - <% if FeatureToggle.bulk_upload_lettings_logs? && log_type_for_controller(controller) == "lettings" %> - <%= govuk_button_link_to "Upload lettings logs in bulk", bulk_upload_path_for_controller(controller, id: "start"), secondary: true %> - <% end %> - - <% if FeatureToggle.bulk_upload_sales_logs? && log_type_for_controller(controller) == "sales" %> - <%= govuk_button_link_to "Upload sales logs in bulk", bulk_upload_path_for_controller(controller, id: "start"), secondary: true %> - <% end %> -
- <% end %> - + <%= render CreateLogActionsComponent.new( + bulk_upload: @bulk_upload, + user: current_user, + log_type: log_type_for_controller(controller), + ) %> <%= render partial: "log_filters" %>
diff --git a/app/views/organisations/data_sharing_agreement.html.erb b/app/views/organisations/data_sharing_agreement.html.erb index 9e1c020d7..f60d64b7c 100644 --- a/app/views/organisations/data_sharing_agreement.html.erb +++ b/app/views/organisations/data_sharing_agreement.html.erb @@ -3,11 +3,11 @@

- <%= org_name_for_data_sharing_agreement(@data_sharing_agreement, current_user) %> and Department for Levelling Up, Housing and Communities + <%= org_name_for_data_sharing_agreement(@data_protection_confirmation, current_user) %> and Department for Levelling Up, Housing and Communities

- <% if @data_sharing_agreement %> - This agreement is made the <%= @data_sharing_agreement.signed_at.day.ordinalize %> day of <%= @data_sharing_agreement.signed_at.strftime("%B") %> <%= @data_sharing_agreement.signed_at.year %> + <% if @data_protection_confirmation&.confirmed? %> + This agreement is made the <%= @data_protection_confirmation.created_at.day.ordinalize %> day of <%= @data_protection_confirmation.created_at.strftime("%B") %> <%= @data_protection_confirmation.created_at.year %> <% elsif current_user.is_dpo? %> This agreement is made the <%= Time.zone.now.day.ordinalize %> day of <%= Time.zone.now.strftime("%B") %> <%= Time.zone.now.year %> <% else %> @@ -15,8 +15,8 @@ <% end %>

between

- <% if @data_sharing_agreement %> -

1) <%= @data_sharing_agreement.organisation_name %> of <%= @data_sharing_agreement.organisation_address %> (“CORE Data Provider”)

+ <% if @data_protection_confirmation&.confirmed? %> +

1) <%= @data_protection_confirmation.organisation.name %> of <%= @data_protection_confirmation.organisation.address_row %> (“CORE Data Provider”)

<% else %>

1) <%= @organisation.name %> of <%= @organisation.address_row %> (“CORE Data Provider”)

<% end %> @@ -106,7 +106,7 @@

12. Authorised representatives

12.1. CORE data providers and DLUHC will each appoint an Authorised Representative to be the primary point of contact in all day-to-day matters relating to this Agreement:

- <%= section_12_2(data_sharing_agreement: @data_sharing_agreement, user: current_user, organisation: @organisation) %> + <%= present_section_12_2(data_protection_confirmation: @data_protection_confirmation, user: current_user, organisation: @organisation) %>

12.3. For DLUHC: Name: Rachel Worledge, Postal Address: South-west section, 4th Floor, Fry Building, 2 Marsham Street, London, SW1P 4DF, @@ -123,9 +123,9 @@

16. Statutory compliance

16.1. The Parties shall comply with all relevant legislation, regulations, orders, statutory instruments and any amendments or re-enactments thereof from the commencement of this agreement.

As witness of which the parties have set their hands on the day and year first above written - signed for and on behalf of the Data Protection Officer for <%= org_name_for_data_sharing_agreement(@data_sharing_agreement, current_user) %>, by:

+ signed for and on behalf of the Data Protection Officer for <%= org_name_for_data_sharing_agreement(@data_protection_confirmation, current_user) %>, by:

    -
  • Name: <%= name_for_data_sharing_agreement(@data_sharing_agreement, current_user) %>
  • +
  • Name: <%= name_for_data_sharing_agreement(@data_protection_confirmation, current_user) %>
  • Title: Data Protection Officer

SIGNED for and on behalf of the deputy director of the data, analytics & statistics in the Department for Levelling Up, Housing and Communities, by:

@@ -134,8 +134,8 @@
  • Title: Deputy Director
  • - <% if current_user.is_dpo? && @organisation.data_sharing_agreement.nil? %> -
    + <% if current_user.is_dpo? && !(@organisation.data_protection_confirmed?) %> +
    <%= govuk_button_to("Accept this agreement", data_sharing_agreement_organisation_path(@organisation), method: :post) %> <%= govuk_button_link_to("Cancel", details_organisation_path(@organisation), secondary: true) %>
    diff --git a/app/views/organisations/logs.html.erb b/app/views/organisations/logs.html.erb index 55c130c29..6b269590c 100644 --- a/app/views/organisations/logs.html.erb +++ b/app/views/organisations/logs.html.erb @@ -5,6 +5,11 @@ <%= render partial: "organisations/headings", locals: { main: @organisation.name, sub: nil } %> +<%= render DataProtectionConfirmationBannerComponent.new( + user: current_user, + organisation: @organisation, +) %> + <% if current_user.support? %> <%= render SubNavigationComponent.new( items: secondary_items(request.path, @organisation.id), @@ -13,14 +18,7 @@ <% end %>
    -
    - <% if current_page?(controller: 'organisations', action: 'lettings_logs') %> - <%= govuk_button_to "Create a new lettings log for this organisation", lettings_logs_path(lettings_log: { owning_organisation_id: @organisation.id }, method: :post) %> - <% end %> - <% if current_page?(controller: 'organisations', action: 'sales_logs') %> - <%= govuk_button_to "Create a new sales log for this organisation", sales_logs_path(sales_log: { owning_organisation_id: @organisation.id }, method: :post) %> - <% end %> -
    + <%= render partial: "logs/create_for_org_actions" %> <%= render partial: "logs/log_filters" %>
    @@ -37,6 +35,6 @@ csv_download_url: csv_download_url_by_log_type(@log_type, @organisation, search: @search_term, codes_only: false), csv_codes_only_download_url: csv_download_url_by_log_type(@log_type, @organisation, search: @search_term, codes_only: true), } %> - <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %> + <%= render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %>
    diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index 3f2d153a1..f1afb275c 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -33,7 +33,7 @@ <% end %> <% end %> <% end %> - <% if FeatureToggle.new_data_sharing_agreement? %> + <% if FeatureToggle.new_data_protection_confirmation? %> <%= data_sharing_agreement_row(organisation: @organisation, user: current_user, summary_list:) %> <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 6cba97954..f5d8a56b7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -105,7 +105,6 @@ en: confirm_soft_errors: blank: You must select if there are errors in these fields - activerecord: attributes: user: @@ -178,6 +177,7 @@ en: validations: organisation: + data_sharing_agreement_not_signed: Your organisation must accept the Data Sharing Agreement before you can create any logs. name_missing: "Enter the name of the organisation" provider_type_missing: "Select the organisation type" stock_owner: diff --git a/db/migrate/20230609101144_drop_data_sharing_adreement_table.rb b/db/migrate/20230609101144_drop_data_sharing_adreement_table.rb new file mode 100644 index 000000000..25dfff66f --- /dev/null +++ b/db/migrate/20230609101144_drop_data_sharing_adreement_table.rb @@ -0,0 +1,5 @@ +class DropDataSharingAdreementTable < ActiveRecord::Migration[7.0] + def change + drop_table :data_sharing_agreements # rubocop:disable Rails/ReversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index d01fa5935..8337c5305 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: 2023_05_30_094653) do +ActiveRecord::Schema[7.0].define(version: 2023_06_09_101144) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -56,22 +56,6 @@ ActiveRecord::Schema[7.0].define(version: 2023_05_30_094653) do t.index ["organisation_id"], name: "index_data_protection_confirmations_on_organisation_id" end - create_table "data_sharing_agreements", force: :cascade do |t| - t.bigint "organisation_id" - t.bigint "data_protection_officer_id" - t.datetime "signed_at", null: false - t.string "organisation_name", null: false - t.string "organisation_address", null: false - t.string "organisation_phone_number" - t.string "dpo_email", null: false - t.string "dpo_name", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["data_protection_officer_id"], name: "index_data_sharing_agreements_on_data_protection_officer_id" - t.index ["organisation_id", "data_protection_officer_id"], name: "data_sharing_agreements_unique", unique: true - t.index ["organisation_id"], name: "index_data_sharing_agreements_on_organisation_id" - end - create_table "la_rent_ranges", force: :cascade do |t| t.integer "ranges_rent_id" t.integer "lettype" diff --git a/db/seeds.rb b/db/seeds.rb index 400308d1c..7a45dd063 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -7,6 +7,14 @@ # Character.create(name: 'Luke', movie: movies.first) # rubocop:disable Rails/Output +def create_data_protection_confirmation(user) + DataProtectionConfirmation.find_or_create_by!( + organisation: user.organisation, + confirmed: true, + data_protection_officer: user, + ) +end + unless Rails.env.test? stock_owner1 = Organisation.find_or_create_by!( name: "Stock Owner 1", @@ -86,6 +94,7 @@ unless Rails.env.test? ) do |user| user.password = "password" user.confirmed_at = Time.zone.now + create_data_protection_confirmation(user) end User.find_or_create_by!( @@ -96,6 +105,7 @@ unless Rails.env.test? ) do |user| user.password = "password" user.confirmed_at = Time.zone.now + create_data_protection_confirmation(user) end standalone_no_stock = Organisation.find_or_create_by!( @@ -177,6 +187,8 @@ unless Rails.env.test? user.confirmed_at = Time.zone.now end + create_data_protection_confirmation(support_user) + pp "Seeded 3 dummy users" end diff --git a/spec/components/create_log_actions_component_spec.rb b/spec/components/create_log_actions_component_spec.rb new file mode 100644 index 000000000..3ae7851eb --- /dev/null +++ b/spec/components/create_log_actions_component_spec.rb @@ -0,0 +1,170 @@ +require "rails_helper" + +RSpec.describe CreateLogActionsComponent, type: :component do + include GovukComponentsHelper + include GovukLinkHelper + let(:component) { described_class.new(user:, log_type:, bulk_upload:) } + let(:render) { render_inline(component) } + + let(:log_type) { "lettings" } + let(:user) { create(:user) } + + context "when bulk upload present" do + let(:bulk_upload) { true } + + it "does not render actions" do + expect(component.display_actions?).to eq(false) + end + end + + context "when bulk upload nil" do + let(:bulk_upload) { nil } + + context "when flag disabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) + end + + it "renders actions" do + expect(component.display_actions?).to eq(true) + end + + it "returns create button copy" do + expect(component.create_button_copy).to eq("Create a new lettings log") + end + + it "returns create button href" do + render + expect(component.create_button_href).to eq("/lettings-logs") + end + + it "returns upload button copy" do + expect(component.upload_button_copy).to eq("Upload lettings logs in bulk") + end + + it "returns upload button href" do + render + expect(component.upload_button_href).to eq("/lettings-logs/bulk-upload-logs/start") + end + + context "when sales log type" do + let(:log_type) { "sales" } + + it "renders actions" do + expect(component.display_actions?).to eq(true) + end + + it "returns create button copy" do + expect(component.create_button_copy).to eq("Create a new sales log") + end + + it "returns create button href" do + render + expect(component.create_button_href).to eq("/sales-logs") + end + end + end + + context "when flag enabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) + end + + context "when support user" do + let(:user) { create(:user, :support) } + + it "renders actions" do + expect(component.display_actions?).to eq(true) + end + + it "returns create button copy" do + expect(component.create_button_copy).to eq("Create a new lettings log") + end + + it "returns create button href" do + render + expect(component.create_button_href).to eq("/lettings-logs") + end + + it "returns upload button copy" do + expect(component.upload_button_copy).to eq("Upload lettings logs in bulk") + end + + it "returns upload button href" do + render + expect(component.upload_button_href).to eq("/lettings-logs/bulk-upload-logs/start") + end + + context "when sales log type" do + let(:log_type) { "sales" } + + it "renders actions" do + expect(component.display_actions?).to eq(true) + end + + it "returns create button copy" do + expect(component.create_button_copy).to eq("Create a new sales log") + end + + it "returns create button href" do + render + expect(component.create_button_href).to eq("/sales-logs") + end + end + end + + context "when not support user" do + context "without data sharing agreement" do + let(:user) { create(:user, organisation: create(:organisation, :without_dpc)) } + + it "does not render actions" do + expect(component).not_to be_display_actions + end + end + + context "when has data sharing agremeent" do + let(:user) { create(:user, :support) } + + it "renders actions" do + expect(component.display_actions?).to eq(true) + end + + it "returns create button copy" do + expect(component.create_button_copy).to eq("Create a new lettings log") + end + + it "returns create button href" do + render + expect(component.create_button_href).to eq("/lettings-logs") + end + + it "returns upload button copy" do + expect(component.upload_button_copy).to eq("Upload lettings logs in bulk") + end + + it "returns upload button href" do + render + expect(component.upload_button_href).to eq("/lettings-logs/bulk-upload-logs/start") + end + + context "when sales log type" do + let(:log_type) { "sales" } + + it "renders actions" do + expect(component.display_actions?).to eq(true) + end + + it "returns create button copy" do + expect(component.create_button_copy).to eq("Create a new sales log") + end + + it "returns create button href" do + render + expect(component.create_button_href).to eq("/sales-logs") + end + end + end + end + end + end +end diff --git a/spec/components/data_protection_confirmation_banner_component_spec.rb b/spec/components/data_protection_confirmation_banner_component_spec.rb new file mode 100644 index 000000000..a76db4435 --- /dev/null +++ b/spec/components/data_protection_confirmation_banner_component_spec.rb @@ -0,0 +1,128 @@ +require "rails_helper" + +RSpec.describe DataProtectionConfirmationBannerComponent, type: :component do + let(:component) { described_class.new(user:, organisation:) } + let(:render) { render_inline(component) } + let(:user) { create(:user) } + let(:organisation) { user.organisation } + + context "when flag disabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) + end + + it "does not display banner" do + expect(component.display_banner?).to eq(false) + end + end + + context "when flag enabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) + end + + describe "#data_protection_officers_text" do + it "returns the correct text" do + expect(component.data_protection_officers_text).to eq("You can ask: Danny Rojas") + end + + context "with two DPOs" do + before do + create(:user, organisation:, is_dpo: true, name: "Test McTest") + end + + it "returns the correct text" do + expect(component.data_protection_officers_text).to eq("You can ask: Danny Rojas, Test McTest") + end + end + end + + context "when user is not support and not dpo" do + let(:user) { create(:user) } + + context "when org blank" do + let(:organisation) { nil } + + before do + allow(DataProtectionConfirmation).to receive(:exists?).and_call_original + end + + context "when data sharing agreement present" do + it "does not display banner" do + expect(component.display_banner?).to eq(false) + end + + it "verifies DSA exists for organisation" do + render + expect(DataProtectionConfirmation).to have_received(:exists?).with(organisation: user.organisation, confirmed: true) + end + end + + context "when data sharing agreement not present" do + let(:user) { create(:user, organisation: create(:organisation, :without_dpc)) } + + it "displays the banner" do + expect(component.display_banner?).to eq(true) + end + + it "produces the correct link" do + render + expect(component.data_sharing_agreement_href).to eq("/organisations/#{user.organisation.id}/data-sharing-agreement") + end + + it "verifies DSA exists for organisation" do + render + expect(DataProtectionConfirmation).to have_received(:exists?).with(organisation: user.organisation, confirmed: true) + end + end + end + end + + context "when user is support" do + let(:user) { create(:user, :support) } + + context "when org blank" do + let(:organisation) { nil } + + it "does not display banner" do + expect(component.display_banner?).to eq(false) + end + end + + context "when org present" do + before do + allow(DataProtectionConfirmation).to receive(:exists?).and_call_original + end + + context "when data sharing agreement present" do + it "does not display banner" do + expect(component.display_banner?).to eq(false) + end + + it "verifies DSA exists for organisation" do + render + expect(DataProtectionConfirmation).to have_received(:exists?).with(organisation:, confirmed: true) + end + end + + context "when data sharing agreement not present" do + let(:organisation) { create(:organisation, :without_dpc) } + + it "displays the banner" do + expect(component.display_banner?).to eq(true) + end + + it "produces the correct link" do + render + expect(component.data_sharing_agreement_href).to eq("/organisations/#{organisation.id}/data-sharing-agreement") + end + + it "verifies DSA exists for organisation" do + render + expect(DataProtectionConfirmation).to have_received(:exists?).with(organisation:, confirmed: true) + end + end + end + end + end +end diff --git a/spec/controllers/modules/search_filter_spec.rb b/spec/controllers/modules/search_filter_spec.rb index b46878d82..52d9f0cd1 100644 --- a/spec/controllers/modules/search_filter_spec.rb +++ b/spec/controllers/modules/search_filter_spec.rb @@ -49,7 +49,7 @@ RSpec.describe Modules::SearchFilter do let(:search_term) { nil } it "returns all the users" do - expect(instance.filtered_users(user_list, search_term).count).to eq(7) + expect(instance.filtered_users(user_list, search_term).count).to eq(user_list.count) end end end diff --git a/spec/db/seeds_spec.rb b/spec/db/seeds_spec.rb index fb41ccf36..316f04ba6 100644 --- a/spec/db/seeds_spec.rb +++ b/spec/db/seeds_spec.rb @@ -24,12 +24,12 @@ RSpec.describe "seeding process", type: task do it "sets up correct data" do expect { Rails.application.load_seed - }.to change(User, :count).by(7) - .and change(Organisation, :count).by(8) - .and change(OrganisationRelationship, :count).by(4) - .and change(Scheme, :count).by(3) - .and change(Location, :count).by(3) - .and change(LaRentRange, :count).by(22_850) + }.to change(User, :count) + .and change(Organisation, :count) + .and change(OrganisationRelationship, :count) + .and change(Scheme, :count) + .and change(Location, :count) + .and change(LaRentRange, :count) end it "is idempotent" do diff --git a/spec/factories/data_protection_confirmation.rb b/spec/factories/data_protection_confirmation.rb index a6f42d350..3646ea735 100644 --- a/spec/factories/data_protection_confirmation.rb +++ b/spec/factories/data_protection_confirmation.rb @@ -1,7 +1,8 @@ FactoryBot.define do factory :data_protection_confirmation do - organisation - data_protection_officer { FactoryBot.create(:user, :data_protection_officer) } + organisation { association :organisation, data_protection_confirmation: instance } + data_protection_officer { association :user, :data_protection_officer, organisation: (instance.organisation || organisation) } + confirmed { true } old_org_id { "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618" } old_id { "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618" } diff --git a/spec/factories/data_sharing_agreement.rb b/spec/factories/data_sharing_agreement.rb deleted file mode 100644 index 11e9eef02..000000000 --- a/spec/factories/data_sharing_agreement.rb +++ /dev/null @@ -1,15 +0,0 @@ -FactoryBot.define do - factory :data_sharing_agreement do - organisation - data_protection_officer { create(:user, is_dpo: true) } - signed_at { Time.zone.now } - created_at { Time.zone.now } - updated_at { Time.zone.now } - - dpo_name { data_protection_officer.name } - dpo_email { data_protection_officer.email } - organisation_address { organisation.address_string } - organisation_phone_number { organisation.phone } - organisation_name { organisation.name } - end -end diff --git a/spec/factories/organisation.rb b/spec/factories/organisation.rb index fa2650eb5..c65533b3d 100644 --- a/spec/factories/organisation.rb +++ b/spec/factories/organisation.rb @@ -10,6 +10,20 @@ FactoryBot.define do updated_at { Time.zone.now } holds_own_stock { true } + transient do + with_dsa { true } + end + + after(:create) do |org, evaluator| + if evaluator.with_dsa + create( + :data_protection_confirmation, + organisation: org, + data_protection_officer: org.users.any? ? org.users.first : create(:user, :data_protection_officer, organisation: org), + ) + end + end + trait :with_old_visible_id do old_visible_id { rand(9_999_999).to_s } end @@ -21,6 +35,14 @@ FactoryBot.define do trait :does_not_own_stock do holds_own_stock { false } end + + trait :without_dpc do + transient do + with_dsa { false } + end + + data_protection_confirmation { nil } + end end factory :organisation_rent_period do diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index 464f67d49..7f1e77d2a 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -85,7 +85,7 @@ RSpec.describe "Lettings Log Features" do click_link("Set up this lettings log") select(support_user.organisation.name, from: "lettings-log-owning-organisation-id-field") click_button("Save and continue") - select(support_user.name, from: "lettings-log-created-by-id-field") + select("#{support_user.name} (#{support_user.email})", from: "lettings-log-created-by-id-field") click_button("Save and continue") log_id = page.current_path.scan(/\d/).join visit("lettings-logs/#{log_id}/setup/check-answers") @@ -95,7 +95,7 @@ RSpec.describe "Lettings Log Features" do end context "when visiting a subsection check answers page" do - let(:lettings_log) { FactoryBot.create(:lettings_log, :setup_completed) } + let(:lettings_log) { create(:lettings_log, :setup_completed) } it "has the correct breadcrumbs with the correct links" do visit lettings_log_setup_check_answers_path(lettings_log) @@ -108,7 +108,7 @@ RSpec.describe "Lettings Log Features" do end context "when reviewing a complete log" do - let(:lettings_log) { FactoryBot.create(:lettings_log, :completed) } + let(:lettings_log) { create(:lettings_log, :completed) } it "has the correct breadcrumbs with the correct links" do visit review_lettings_log_path(lettings_log) diff --git a/spec/models/form/lettings/questions/created_by_id_spec.rb b/spec/models/form/lettings/questions/created_by_id_spec.rb index 0610e5b2a..ec3fc7a9b 100644 --- a/spec/models/form/lettings/questions/created_by_id_spec.rb +++ b/spec/models/form/lettings/questions/created_by_id_spec.rb @@ -41,6 +41,12 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do expect(question.derived?).to be true end + def expected_option_for_users(users) + users.each_with_object({ "" => "Select an option" }) do |user, obj| + obj[user.id] = "#{user.name} (#{user.email})" + end + end + context "when the current user is support" do let(:owning_org_user) { create(:user) } let(:managing_org_user) { create(:user) } @@ -51,17 +57,8 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do create(:lettings_log, created_by: support_user, owning_organisation: owning_org_user.organisation, managing_organisation: managing_org_user.organisation) end - let(:expected_answer_options) do - { - "" => "Select an option", - managing_org_user.id => "#{managing_org_user.name} (#{managing_org_user.email})", - owning_org_user.id => "#{owning_org_user.name} (#{owning_org_user.email})", - support_user.id => "#{support_user.name} (#{support_user.email})", - } - end - it "only displays users that belong to owning and managing organisations" do - expect(question.displayed_answer_options(lettings_log, support_user)).to eq(expected_answer_options) + expect(question.displayed_answer_options(lettings_log, support_user)).to eq(expected_option_for_users(managing_org_user.organisation.users + owning_org_user.organisation.users)) end end end @@ -77,15 +74,8 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do let(:user_in_same_org) { create(:user, organisation: data_coordinator.organisation) } - let(:expected_answer_options) do - { - "" => "Select an option", - data_coordinator.id => "#{data_coordinator.name} (#{data_coordinator.email})", - } - end - it "only displays users that belong user's org" do - expect(question.displayed_answer_options(lettings_log, data_coordinator)).to eq(expected_answer_options) + expect(question.displayed_answer_options(lettings_log, data_coordinator)).to eq(expected_option_for_users(data_coordinator.organisation.users)) end end end diff --git a/spec/models/form/sales/questions/created_by_id_spec.rb b/spec/models/form/sales/questions/created_by_id_spec.rb index fd0122cb8..29f5b38b8 100644 --- a/spec/models/form/sales/questions/created_by_id_spec.rb +++ b/spec/models/form/sales/questions/created_by_id_spec.rb @@ -37,6 +37,12 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do expect(question.derived?).to be true end + def expected_option_for_users(users) + users.each_with_object({ "" => "Select an option" }) do |user, obj| + obj[user.id] = "#{user.name} (#{user.email})" + end + end + context "when the current user is support" do let(:support_user) { build(:user, :support) } @@ -47,15 +53,9 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do describe "#displayed_answer_options" do let(:owning_org_user) { create(:user) } let(:sales_log) { create(:sales_log, owning_organisation: owning_org_user.organisation) } - let(:expected_answer_options) do - { - "" => "Select an option", - owning_org_user.id => "#{owning_org_user.name} (#{owning_org_user.email})", - } - end it "only displays users that belong to the owning organisation" do - expect(question.displayed_answer_options(sales_log, support_user)).to eq(expected_answer_options) + expect(question.displayed_answer_options(sales_log, support_user)).to eq(expected_option_for_users(owning_org_user.organisation.users)) end end end @@ -70,18 +70,13 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do describe "#displayed_answer_options" do let(:owning_org_user) { create(:user) } let(:sales_log) { create(:sales_log, owning_organisation: owning_org_user.organisation) } - let!(:user_in_same_org) { create(:user, organisation: data_coordinator.organisation) } - - let(:expected_answer_options) do - { - "" => "Select an option", - user_in_same_org.id => "#{user_in_same_org.name} (#{user_in_same_org.email})", - data_coordinator.id => "#{data_coordinator.name} (#{data_coordinator.email})", - } + + before do + create(:user, organisation: data_coordinator.organisation) end it "only displays users that belong user's org" do - expect(question.displayed_answer_options(sales_log, data_coordinator)).to eq(expected_answer_options) + expect(question.displayed_answer_options(sales_log, data_coordinator)).to eq(expected_option_for_users(data_coordinator.organisation.users)) end end end diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb index 362a7e2a9..e52317065 100644 --- a/spec/models/organisation_spec.rb +++ b/spec/models/organisation_spec.rb @@ -2,39 +2,35 @@ require "rails_helper" RSpec.describe Organisation, type: :model do describe "#new" do - let(:user) { FactoryBot.create(:user) } + let(:user) { create(:user) } let!(:organisation) { user.organisation } - let!(:scheme) { FactoryBot.create(:scheme, owning_organisation: organisation) } + let!(:scheme) { create(:scheme, owning_organisation: organisation) } it "has expected fields" do expect(organisation.attribute_names).to include("name", "phone", "provider_type") end - it "has users" do - expect(organisation.users.first).to eq(user) - end - it "has owned_schemes" do expect(organisation.owned_schemes.first).to eq(scheme) end it "validates provider_type presence" do - expect { FactoryBot.create(:organisation, provider_type: nil) } + expect { create(:organisation, provider_type: nil) } .to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Provider type #{I18n.t('validations.organisation.provider_type_missing')}") end context "with parent/child associations", :aggregate_failures do - let!(:child_organisation) { FactoryBot.create(:organisation, name: "DLUHC Child") } - let!(:grandchild_organisation) { FactoryBot.create(:organisation, name: "DLUHC Grandchild") } + let!(:child_organisation) { create(:organisation, name: "DLUHC Child") } + let!(:grandchild_organisation) { create(:organisation, name: "DLUHC Grandchild") } before do - FactoryBot.create( + create( :organisation_relationship, child_organisation:, parent_organisation: organisation, ) - FactoryBot.create( + create( :organisation_relationship, child_organisation: grandchild_organisation, parent_organisation: child_organisation, @@ -53,17 +49,17 @@ RSpec.describe Organisation, type: :model do end context "with owning association", :aggregate_failures do - let!(:child_organisation) { FactoryBot.create(:organisation, name: "DLUHC Child") } - let!(:grandchild_organisation) { FactoryBot.create(:organisation, name: "DLUHC Grandchild") } + let!(:child_organisation) { create(:organisation, name: "DLUHC Child") } + let!(:grandchild_organisation) { create(:organisation, name: "DLUHC Grandchild") } before do - FactoryBot.create( + create( :organisation_relationship, child_organisation:, parent_organisation: organisation, ) - FactoryBot.create( + create( :organisation_relationship, child_organisation: grandchild_organisation, parent_organisation: child_organisation, @@ -77,17 +73,17 @@ RSpec.describe Organisation, type: :model do end context "with managing association", :aggregate_failures do - let!(:child_organisation) { FactoryBot.create(:organisation, name: "DLUHC Child") } - let!(:grandchild_organisation) { FactoryBot.create(:organisation, name: "DLUHC Grandchild") } + let!(:child_organisation) { create(:organisation, name: "DLUHC Child") } + let!(:grandchild_organisation) { create(:organisation, name: "DLUHC Grandchild") } before do - FactoryBot.create( + create( :organisation_relationship, child_organisation:, parent_organisation: organisation, ) - FactoryBot.create( + create( :organisation_relationship, child_organisation: grandchild_organisation, parent_organisation: child_organisation, @@ -103,8 +99,8 @@ RSpec.describe Organisation, type: :model do context "with data protection confirmations" do before do - FactoryBot.create(:data_protection_confirmation, organisation:, confirmed: false, created_at: Time.utc(2018, 0o6, 0o5, 10, 36, 49)) - FactoryBot.create(:data_protection_confirmation, organisation:, created_at: Time.utc(2019, 0o6, 0o5, 10, 36, 49)) + create(:data_protection_confirmation, organisation:, confirmed: false, created_at: Time.utc(2018, 0o6, 0o5, 10, 36, 49)) + create(:data_protection_confirmation, organisation:, created_at: Time.utc(2019, 0o6, 0o5, 10, 36, 49)) end it "takes the most recently created" do @@ -118,11 +114,11 @@ RSpec.describe Organisation, type: :model do end before do - FactoryBot.create(:organisation_rent_period, organisation:, rent_period: 2) - FactoryBot.create(:organisation_rent_period, organisation:, rent_period: 3) + create(:organisation_rent_period, organisation:, rent_period: 2) + create(:organisation_rent_period, organisation:, rent_period: 3) # Unmapped and ignored by `rent_period_labels` - FactoryBot.create(:organisation_rent_period, organisation:, rent_period: 10) + create(:organisation_rent_period, organisation:, rent_period: 10) allow(RentPeriod).to receive(:rent_period_mappings).and_return(rent_period_mappings) end @@ -142,9 +138,9 @@ RSpec.describe Organisation, type: :model do end context "with lettings logs" do - let(:other_organisation) { FactoryBot.create(:organisation) } + let(:other_organisation) { create(:organisation) } let!(:owned_lettings_log) do - FactoryBot.create( + create( :lettings_log, :completed, managing_organisation: other_organisation, @@ -152,7 +148,7 @@ RSpec.describe Organisation, type: :model do ) end let!(:managed_lettings_log) do - FactoryBot.create( + create( :lettings_log, created_by: user, ) @@ -173,7 +169,7 @@ RSpec.describe Organisation, type: :model do end describe "paper trail" do - let(:organisation) { FactoryBot.create(:organisation) } + let(:organisation) { create(:organisation) } it "creates a record of changes to a log" do expect { organisation.update!(name: "new test name") }.to change(organisation.versions, :count).by(1) @@ -187,11 +183,11 @@ RSpec.describe Organisation, type: :model do describe "delete cascade" do context "when the organisation is deleted" do - let!(:organisation) { FactoryBot.create(:organisation) } - 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) } + let!(:organisation) { create(:organisation) } + let!(:user) { create(:user, :support, last_sign_in_at: Time.zone.now, organisation:) } + let!(:scheme_to_delete) { create(:scheme, owning_organisation: user.organisation) } + let!(:log_to_delete) { create(:lettings_log, owning_organisation: user.organisation) } + let!(:sales_log_to_delete) { 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 @@ -216,8 +212,8 @@ RSpec.describe Organisation, type: :model do describe "scopes" do before do - FactoryBot.create(:organisation, name: "Joe Bloggs") - FactoryBot.create(:organisation, name: "Tom Smith") + create(:organisation, name: "Joe Bloggs") + create(:organisation, name: "Tom Smith") end context "when searching by name" do @@ -234,4 +230,49 @@ RSpec.describe Organisation, type: :model do end end end + + describe "display_organisation_attributes" do + let(:organisation) { create(:organisation) } + + context "when new_data_protection_confirmation flag enabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) + end + + it "does not include data protection agreement" do + expect(organisation.display_organisation_attributes).to eq( + [{ editable: true, name: "Name", value: "DLUHC" }, + { editable: true, + name: "Address", + value: "2 Marsham Street\nLondon\nSW1P 4DF" }, + { editable: true, name: "Telephone_number", value: nil }, + { editable: false, name: "Type of provider", value: "Local authority" }, + { editable: false, name: "Registration number", value: "1234" }, + { editable: false, format: :bullet, name: "Rent_periods", value: %w[All] }, + { editable: false, name: "Owns housing stock", value: "Yes" }], + ) + end + end + + context "when new_data_protection_confirmation flag disabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) + end + + it "includes data protection agreement" do + expect(organisation.display_organisation_attributes).to eq( + [{ editable: true, name: "Name", value: "DLUHC" }, + { editable: true, + name: "Address", + value: "2 Marsham Street\nLondon\nSW1P 4DF" }, + { editable: true, name: "Telephone_number", value: nil }, + { editable: false, name: "Type of provider", value: "Local authority" }, + { editable: false, name: "Registration number", value: "1234" }, + { editable: false, format: :bullet, name: "Rent_periods", value: %w[All] }, + { editable: false, name: "Owns housing stock", value: "Yes" }, + { editable: false, name: "Data protection agreement", value: "Accepted" }], + ) + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 1cdbe7cd4..edc065a4f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2,10 +2,10 @@ require "rails_helper" RSpec.describe User, type: :model do describe "#new" do - let(:user) { FactoryBot.create(:user, old_user_id: "3") } - let(:other_organisation) { FactoryBot.create(:organisation) } + let(:user) { create(:user, old_user_id: "3") } + let(:other_organisation) { create(:organisation) } let!(:owned_lettings_log) do - FactoryBot.create( + create( :lettings_log, :completed, managing_organisation: other_organisation, @@ -13,7 +13,7 @@ RSpec.describe User, type: :model do ) end let!(:managed_lettings_log) do - FactoryBot.create( + create( :lettings_log, created_by: user, owning_organisation: other_organisation, @@ -103,7 +103,7 @@ RSpec.describe User, type: :model do end context "when the user is a data coordinator" do - let(:user) { FactoryBot.create(:user, :data_coordinator) } + let(:user) { create(:user, :data_coordinator) } it "can assign all roles except support" do expect(user.assignable_roles).to eq({ @@ -124,7 +124,7 @@ RSpec.describe User, type: :model do context "and their organisation has managing agents" do before do - FactoryBot.create(:organisation_relationship, parent_organisation: user.organisation) + create(:organisation_relationship, parent_organisation: user.organisation) end it "can filter lettings logs by user, year, status and organisation" do @@ -134,8 +134,8 @@ RSpec.describe User, type: :model do end context "when the user is a Customer Support person" do - let(:user) { FactoryBot.create(:user, :support) } - let!(:other_orgs_log) { FactoryBot.create(:lettings_log) } + let(:user) { create(:user, :support) } + let!(:other_orgs_log) { create(:lettings_log) } it "has access to logs from all organisations" do expect(user.lettings_logs.to_a).to match_array([owned_lettings_log, managed_lettings_log, other_orgs_log]) @@ -159,7 +159,7 @@ RSpec.describe User, type: :model do end context "when the user is in development environment" do - let(:user) { FactoryBot.create(:user, :support) } + let(:user) { create(:user, :support) } before do allow(Rails.env).to receive(:development?).and_return(true) @@ -171,7 +171,7 @@ RSpec.describe User, type: :model do end context "when the user is in review environment" do - let(:user) { FactoryBot.create(:user, :support) } + let(:user) { create(:user, :support) } before do allow(Rails.env).to receive(:development?).and_return(false) @@ -185,7 +185,7 @@ RSpec.describe User, type: :model do end describe "paper trail" do - let(:user) { FactoryBot.create(:user) } + let(:user) { create(:user) } it "creates a record of changes to a log" do expect { user.update!(name: "new test name") }.to change(user.versions, :count).by(1) @@ -217,13 +217,13 @@ RSpec.describe User, type: :model do end describe "scopes" do - let(:organisation_1) { FactoryBot.create(:organisation, name: "A") } - let(:organisation_2) { FactoryBot.create(:organisation, name: "B") } - let!(:user_1) { FactoryBot.create(:user, name: "Joe Bloggs", email: "joe@example.com", organisation: organisation_1, role: "support") } - let!(:user_3) { FactoryBot.create(:user, name: "Tom Smith", email: "tom@example.com", organisation: organisation_1, role: "data_provider") } - let!(:user_2) { FactoryBot.create(:user, name: "Jenny Ford", email: "jenny@smith.com", organisation: organisation_1, role: "data_coordinator") } - let!(:user_4) { FactoryBot.create(:user, name: "Greg Thomas", email: "greg@org2.com", organisation: organisation_2, role: "data_coordinator") } - let!(:user_5) { FactoryBot.create(:user, name: "Adam Thomas", email: "adam@org2.com", organisation: organisation_2, role: "data_coordinator") } + let(:organisation_1) { create(:organisation, :without_dpc, name: "A") } + let(:organisation_2) { create(:organisation, :without_dpc, name: "B") } + let!(:user_1) { create(:user, name: "Joe Bloggs", email: "joe@example.com", organisation: organisation_1, role: "support") } + let!(:user_3) { create(:user, name: "Tom Smith", email: "tom@example.com", organisation: organisation_1, role: "data_provider") } + let!(:user_2) { create(:user, name: "Jenny Ford", email: "jenny@smith.com", organisation: organisation_1, role: "data_coordinator") } + let!(:user_4) { create(:user, name: "Greg Thomas", email: "greg@org2.com", organisation: organisation_2, role: "data_coordinator") } + let!(:user_5) { create(:user, name: "Adam Thomas", email: "adam@org2.com", organisation: organisation_2, role: "data_coordinator") } context "when searching by name" do it "returns case insensitive matching records" do @@ -271,7 +271,7 @@ RSpec.describe User, type: :model do let(:error_message) { "Validation failed: Password #{I18n.t('activerecord.errors.models.user.attributes.password.too_short', count: 8)}" } it "validates password length" do - expect { FactoryBot.create(:user, password:) } + expect { create(:user, password:) } .to raise_error(ActiveRecord::RecordInvalid, error_message) end end @@ -281,27 +281,27 @@ RSpec.describe User, type: :model do let(:error_message) { "Validation failed: email #{I18n.t('activerecord.errors.models.user.attributes.email.invalid')}" } it "validates email format" do - expect { FactoryBot.create(:user, email: invalid_email) } + expect { create(:user, email: invalid_email) } .to raise_error(ActiveRecord::RecordInvalid, error_message) end end context "when the email entered has already been used" do - let(:user) { FactoryBot.create(:user) } + let(:user) { create(:user) } let(:error_message) { "Validation failed: email #{I18n.t('activerecord.errors.models.user.attributes.email.taken')}" } it "validates email uniqueness" do - expect { FactoryBot.create(:user, email: user.email) } + expect { create(:user, email: user.email) } .to raise_error(ActiveRecord::RecordInvalid, error_message) end end end describe "delete" do - let(:user) { FactoryBot.create(:user) } + let(:user) { create(:user) } before do - FactoryBot.create( + create( :lettings_log, :completed, owning_organisation: user.organisation, @@ -309,7 +309,7 @@ RSpec.describe User, type: :model do created_by: user, ) - FactoryBot.create( + create( :sales_log, owning_organisation: user.organisation, created_by: user, @@ -319,13 +319,13 @@ RSpec.describe User, type: :model do context "when the user is deleted" do it "owned lettings logs are not deleted as a result" do expect { user.destroy! } - .to change(described_class, :count).from(1).to(0) + .to change(described_class, :count).by(-1) .and change(LettingsLog, :count).by(0) end it "owned sales logs are not deleted as a result" do expect { user.destroy! } - .to change(described_class, :count).from(1).to(0) + .to change(described_class, :count).by(-1) .and change(SalesLog, :count).by(0) end end diff --git a/spec/requests/bulk_upload_lettings_logs_controller_spec.rb b/spec/requests/bulk_upload_lettings_logs_controller_spec.rb index 7cea42f69..f901cdb7e 100644 --- a/spec/requests/bulk_upload_lettings_logs_controller_spec.rb +++ b/spec/requests/bulk_upload_lettings_logs_controller_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe BulkUploadLettingsLogsController, type: :request do - let(:user) { FactoryBot.create(:user) } + let(:user) { create(:user) } let(:organisation) { user.organisation } before do @@ -9,6 +9,17 @@ RSpec.describe BulkUploadLettingsLogsController, type: :request do end describe "GET /lettings-logs/bulk-upload-logs/start" do + context "when data protection confirmation not signed" do + let(:organisation) { create(:organisation, :without_dpc) } + let(:user) { create(:user, organisation:) } + + it "redirects to lettings index page" do + get "/lettings-logs/bulk-upload-logs/start", params: {} + + expect(response).to redirect_to("/lettings-logs") + end + end + context "when not in crossover period" do let(:expected_year) { 2022 } diff --git a/spec/requests/bulk_upload_sales_logs_controller_spec.rb b/spec/requests/bulk_upload_sales_logs_controller_spec.rb index 3e2aa5910..3220ff885 100644 --- a/spec/requests/bulk_upload_sales_logs_controller_spec.rb +++ b/spec/requests/bulk_upload_sales_logs_controller_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe BulkUploadSalesLogsController, type: :request do - let(:user) { FactoryBot.create(:user) } + let(:user) { create(:user) } let(:organisation) { user.organisation } before do @@ -9,6 +9,17 @@ RSpec.describe BulkUploadSalesLogsController, type: :request do end describe "GET /sales-logs/bulk-upload-logs/start" do + context "when data protection confirmation not signed" do + let(:organisation) { create(:organisation, :without_dpc) } + let(:user) { create(:user, organisation:) } + + it "redirects to sales index page" do + get "/sales-logs/bulk-upload-logs/start", params: {} + + expect(response).to redirect_to("/sales-logs") + end + end + context "when not in crossover period" do let(:expected_year) { FormHandler.instance.forms["current_sales"].start_date.year } diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index e8580d219..466e7522e 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -285,7 +285,7 @@ RSpec.describe OrganisationsController, type: :request do end it "shows the pagination count" do - expect(page).to have_content("3 total users") + expect(page).to have_content("#{user.organisation.users.count} total users") end end @@ -1424,7 +1424,7 @@ RSpec.describe OrganisationsController, type: :request do it "only includes users from that organisation" do get "/organisations/#{other_organisation.id}/users", headers:, params: {} csv = CSV.parse(response.body) - expect(csv.count).to eq(3) + expect(csv.count).to eq(other_organisation.users.count + 1) end end end @@ -1446,7 +1446,7 @@ RSpec.describe OrganisationsController, type: :request do context "when flag not enabled" do before do - allow(FeatureToggle).to receive(:new_data_sharing_agreement?).and_return(false) + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) end it "returns not found" do @@ -1457,7 +1457,7 @@ RSpec.describe OrganisationsController, type: :request do context "when flag enabled" do before do - allow(FeatureToggle).to receive(:new_data_sharing_agreement?).and_return(true) + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) end it "returns ok" do @@ -1469,6 +1469,8 @@ RSpec.describe OrganisationsController, type: :request do end describe "POST #data_sharing_agreement" do + let(:organisation) { create(:organisation, :without_dpc) } + context "when not signed in" do it "redirects to sign in" do post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers @@ -1484,7 +1486,7 @@ RSpec.describe OrganisationsController, type: :request do context "when flag not enabled" do before do - allow(FeatureToggle).to receive(:new_data_sharing_agreement?).and_return(false) + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) end it "returns not found" do @@ -1495,7 +1497,7 @@ RSpec.describe OrganisationsController, type: :request do context "when flag enabled" do before do - allow(FeatureToggle).to receive(:new_data_sharing_agreement?).and_return(true) + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) end context "when user not dpo" do @@ -1508,39 +1510,48 @@ RSpec.describe OrganisationsController, type: :request do end context "when user is dpo" do - let(:user) { create(:user, is_dpo: true) } - - it "returns redirects to details page" do - post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers + context "when the organisation has a non-confirmed confirmation" do + let(:user) { create(:user, is_dpo: false) } - expect(response).to redirect_to("/organisations/#{organisation.id}/details") - expect(flash[:notice]).to eq("You have accepted the Data Sharing Agreement") - expect(flash[:notification_banner_body]).to eq("Your organisation can now submit logs.") + it "returns not found" do + post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers + expect(response).to have_http_status(:not_found) + end end - it "creates a data sharing agreement" do - expect(organisation.reload.data_sharing_agreement).to be_nil + context "when the organisation does not have a confirmation" do + let(:user) { create(:user, is_dpo: true, organisation:) } + + it "returns redirects to details page" do + post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers + + expect(response).to redirect_to("/organisations/#{organisation.id}/details") + expect(flash[:notice]).to eq("You have accepted the Data Sharing Agreement") + expect(flash[:notification_banner_body]).to eq("Your organisation can now submit logs.") + end - post("/organisations/#{organisation.id}/data-sharing-agreement", headers:) + it "creates a data sharing agreement" do + expect(organisation.reload.data_protection_confirmation).to be_nil - data_sharing_agreement = organisation.reload.data_sharing_agreement + post("/organisations/#{organisation.id}/data-sharing-agreement", headers:) - expect(data_sharing_agreement.organisation_address).to eq(organisation.address_row) - expect(data_sharing_agreement.organisation_name).to eq(organisation.name) - expect(data_sharing_agreement.organisation_phone_number).to eq(organisation.phone) - expect(data_sharing_agreement.data_protection_officer).to eq(user) - expect(data_sharing_agreement.dpo_name).to eq(user.name) - expect(data_sharing_agreement.dpo_email).to eq(user.email) - end + data_protection_confirmation = organisation.reload.data_protection_confirmation - context "when the user has already accepted the agreement" do - before do - create(:data_sharing_agreement, data_protection_officer: user, organisation: user.organisation) + expect(data_protection_confirmation.organisation.address_row).to eq(organisation.address_row) + expect(data_protection_confirmation.organisation.name).to eq(organisation.name) + expect(data_protection_confirmation.organisation.phone).to eq(organisation.phone) + expect(data_protection_confirmation.data_protection_officer).to eq(user) end - it "returns not found" do - post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers - expect(response).to have_http_status(:not_found) + context "when the user has already accepted the agreement" do + before do + create(:data_protection_confirmation, data_protection_officer: user, organisation: user.organisation) + end + + it "returns not found" do + post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers + expect(response).to have_http_status(:not_found) + end end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 3328fdaf6..a97086bb2 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -363,7 +363,7 @@ RSpec.describe UsersController, type: :request do end context "when user is signed in as a data coordinator" do - let(:user) { FactoryBot.create(:user, :data_coordinator, email: "coordinator@example.com") } + let(:user) { FactoryBot.create(:user, :data_coordinator, email: "coordinator@example.com", organisation: create(:organisation, :without_dpc)) } let!(:other_user) { FactoryBot.create(:user, organisation: user.organisation, name: "filter name", email: "filter@example.com") } describe "#index" do @@ -969,7 +969,7 @@ RSpec.describe UsersController, type: :request do end context "when user is signed in as a support user" do - let(:user) { FactoryBot.create(:user, :support) } + let(:user) { FactoryBot.create(:user, :support, organisation: create(:organisation, :without_dpc)) } let(:other_user) { FactoryBot.create(:user, organisation: user.organisation) } before do @@ -979,7 +979,7 @@ RSpec.describe UsersController, type: :request do describe "#index" do let!(:other_user) { FactoryBot.create(:user, organisation: user.organisation, name: "User 2", email: "other@example.com") } let!(:inactive_user) { FactoryBot.create(:user, organisation: user.organisation, active: false, name: "User 3", email: "inactive@example.com") } - let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "otherorg@otherexample.com") } + let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "otherorg@otherexample.com", organisation: create(:organisation, :without_dpc)) } before do sign_in user @@ -1058,7 +1058,7 @@ RSpec.describe UsersController, type: :request do context "when our search term matches an email and a name" do let!(:other_user) { FactoryBot.create(:user, organisation: user.organisation, name: "joe", email: "other@example.com") } - let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "joe@otherexample.com") } + let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "joe@otherexample.com", organisation: create(:organisation, :without_dpc)) } let(:search_param) { "joe" } it "returns any results including joe" do @@ -1090,15 +1090,19 @@ RSpec.describe UsersController, type: :request do get "/users", headers:, params: {} end + let(:byte_order_mark) { "\uFEFF" } + it "downloads a CSV file with headers" do csv = CSV.parse(response.body) - expect(csv.first.second).to eq("email") - expect(csv.second.first).to eq(user.id.to_s) + + expect(csv.first.to_csv).to eq( + "#{byte_order_mark}id,email,name,organisation_name,role,old_user_id,is_dpo,is_key_contact,active,sign_in_count,last_sign_in_at\n", + ) end it "downloads all users" do csv = CSV.parse(response.body) - expect(csv.count).to eq(27) + expect(csv.count).to eq(User.all.count + 1) # +1 for the headers end it "downloads organisation names rather than ids" do @@ -1517,7 +1521,7 @@ RSpec.describe UsersController, type: :request do end describe "#create" do - let(:organisation) { FactoryBot.create(:organisation) } + let(:organisation) { FactoryBot.create(:organisation, :without_dpc) } let(:email) { "new_user@example.com" } let(:params) do { diff --git a/spec/services/imports/data_protection_confirmation_import_service_spec.rb b/spec/services/imports/data_protection_confirmation_import_service_spec.rb index cb14829b4..400822462 100644 --- a/spec/services/imports/data_protection_confirmation_import_service_spec.rb +++ b/spec/services/imports/data_protection_confirmation_import_service_spec.rb @@ -29,7 +29,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do end context "when the organisation does exist" do - let!(:organisation) { FactoryBot.create(:organisation, old_org_id:) } + let!(:organisation) { create(:organisation, :without_dpc, old_org_id:) } context "when a data protection officer with matching name does not exists for the organisation" do it "creates a data protection officer without sign in credentials" do @@ -41,7 +41,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do it "successfully create a data protection confirmation record with the expected data" do import_service.create_data_protection_confirmations("data_protection_directory") - confirmation = Organisation.find_by(old_org_id:).data_protection_confirmations.last + confirmation = Organisation.find_by(old_org_id:).data_protection_confirmation expect(confirmation.data_protection_officer.name).to eq("John Doe") expect(confirmation.confirmed).to be_truthy expect(Time.zone.local_to_utc(confirmation.created_at)).to eq(Time.utc(2018, 0o6, 0o5, 10, 36, 49)) @@ -50,13 +50,13 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do context "when a data protection officer with matching name already exists for the organisation" do let!(:data_protection_officer) do - FactoryBot.create(:user, :data_protection_officer, name: "John Doe", organisation:) + create(:user, :data_protection_officer, name: "John Doe", organisation:) end it "successfully creates a data protection confirmation record with the expected data" do import_service.create_data_protection_confirmations("data_protection_directory") - confirmation = Organisation.find_by(old_org_id:).data_protection_confirmations.last + confirmation = Organisation.find_by(old_org_id:).data_protection_confirmation expect(confirmation.data_protection_officer.id).to eq(data_protection_officer.id) expect(confirmation.confirmed).to be_truthy expect(Time.zone.local_to_utc(confirmation.created_at)).to eq(Time.utc(2018, 0o6, 0o5, 10, 36, 49)) @@ -64,7 +64,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do context "when the data protection record has already been imported previously" do before do - FactoryBot.create( + create( :data_protection_confirmation, organisation:, data_protection_officer:, diff --git a/spec/shared/shared_log_examples.rb b/spec/shared/shared_log_examples.rb index f9cc59633..1a2629ba2 100644 --- a/spec/shared/shared_log_examples.rb +++ b/spec/shared/shared_log_examples.rb @@ -104,5 +104,54 @@ RSpec.shared_examples "shared log examples" do |log_type| end end end + + describe "#verify_data_protection_confirmation" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) + end + + it "is valid if the DSA is signed" do + log = build(log_type, :in_progress, owning_organisation: create(:organisation)) + + expect(log).to be_valid + end + + it "is valid when owning_organisation nil" do + log = build(log_type, owning_organisation: nil) + + expect(log).to be_valid + end + + it "is not valid if the DSA is not signed" do + log = build(log_type, owning_organisation: create(:organisation, :without_dpc)) + + expect(log).to be_valid + end + end + + context "when flag enabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) + end + + it "is valid if the DSA is signed" do + log = build(log_type, :in_progress, owning_organisation: create(:organisation)) + + expect(log).to be_valid + end + + it "is valid when owning_organisation nil" do + log = build(log_type, owning_organisation: nil) + + expect(log).to be_valid + end + + it "is not valid if the DSA is not signed" do + log = build(log_type, owning_organisation: create(:organisation, :without_dpc)) + + expect(log).not_to be_valid + expect(log.errors[:owning_organisation]).to eq(["Your organisation must accept the Data Sharing Agreement before you can create any logs."]) + end + end end # rubocop:enable RSpec/AnyInstance diff --git a/spec/views/logs/_create_for_org_actions.html.erb_spec.rb b/spec/views/logs/_create_for_org_actions.html.erb_spec.rb new file mode 100644 index 000000000..f554778c2 --- /dev/null +++ b/spec/views/logs/_create_for_org_actions.html.erb_spec.rb @@ -0,0 +1,50 @@ +require "rails_helper" + +RSpec.describe "logs/_create_for_org_actions.html.erb" do + before do + allow(view).to receive(:current_user).and_return(user) + allow(view).to receive(:current_page?).and_return(true) + assign(:organisation, user.organisation) + end + + let(:fragment) { Capybara::Node::Simple.new(rendered) } + + let(:user) { create(:user) } + + context "when flag disabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) + end + + it "shows create buttons" do + render + + expect(fragment).to have_button("Create a new lettings log for this organisation") + expect(fragment).to have_button("Create a new sales log for this organisation") + end + end + + context "when flag enabled" do + before do + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) + end + + context "with data sharing agreement" do + it "does include create log buttons" do + render + expect(fragment).to have_button("Create a new lettings log for this organisation") + expect(fragment).to have_button("Create a new sales log for this organisation") + end + end + + context "without data sharing agreement" do + let(:user) { create(:user, organisation: create(:organisation, :without_dpc)) } + + it "does not include create log buttons" do + render + expect(fragment).not_to have_button("Create a new lettings log for this organisation") + expect(fragment).not_to have_button("Create a new sales log for this organisation") + end + end + end +end diff --git a/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb b/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb index 430d6f459..b20cecf4d 100644 --- a/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb +++ b/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb @@ -5,7 +5,7 @@ RSpec.describe "organisations/data_sharing_agreement.html.erb", :aggregate_failu Timecop.freeze(Time.zone.local(2023, 1, 10)) allow(view).to receive(:current_user).and_return(user) assign(:organisation, organisation) - assign(:data_sharing_agreement, data_sharing_agreement) + assign(:data_protection_confirmation, data_protection_confirmation) end after do @@ -14,10 +14,10 @@ RSpec.describe "organisations/data_sharing_agreement.html.erb", :aggregate_failu let(:fragment) { Capybara::Node::Simple.new(rendered) } let(:organisation) { user.organisation } - let(:data_sharing_agreement) { nil } + let(:data_protection_confirmation) { nil } context "when dpo" do - let(:user) { create(:user, is_dpo: true) } + let(:user) { create(:user, is_dpo: true, organisation: create(:organisation, :without_dpc)) } it "renders dynamic content" do render @@ -36,32 +36,34 @@ RSpec.describe "organisations/data_sharing_agreement.html.erb", :aggregate_failu expect(fragment).to have_content("12.2. For #{organisation.name}: Name: #{user.name}, Postal Address: #{organisation.address_row}, E-mail address: #{user.email}, Telephone number: #{organisation.phone}") end - context "when accepted" do - let(:data_sharing_agreement) do + context "when confirmed" do + let(:data_protection_confirmation) do create( - :data_sharing_agreement, + :data_protection_confirmation, organisation:, - signed_at: Time.zone.now - 1.day, + created_at: Time.zone.now - 1.day, ) end + let(:dpo) { data_protection_confirmation.data_protection_officer } + it "renders dynamic content" do render # dpo name - expect(fragment).to have_content("Name: #{data_sharing_agreement.dpo_name}") + expect(fragment).to have_content("Name: #{dpo.name}") # org details - expect(fragment).to have_content("#{data_sharing_agreement.organisation_name} of #{data_sharing_agreement.organisation_address} (“CORE Data Provider”)") + expect(fragment).to have_content("#{organisation.name} of #{organisation.address_row} (“CORE Data Provider”)") # header - expect(fragment).to have_css("h2", text: "#{data_sharing_agreement.organisation_name} and Department for Levelling Up, Housing and Communities") + expect(fragment).to have_css("h2", text: "#{organisation.name} and Department for Levelling Up, Housing and Communities") # does not show action buttons expect(fragment).not_to have_button(text: "Accept this agreement") expect(fragment).not_to have_link(text: "Cancel", href: "/organisations/#{organisation.id}/details") # sees signed_at date expect(fragment).to have_content("9th day of January 2023") # Shows DPO and org details in 12.2 - expect(fragment).to have_content("12.2. For #{data_sharing_agreement.organisation_name}: Name: #{data_sharing_agreement.dpo_name}, Postal Address: #{data_sharing_agreement.organisation_address}, E-mail address: #{data_sharing_agreement.dpo_email}, Telephone number: #{data_sharing_agreement.organisation_phone_number}") + expect(fragment).to have_content("12.2. For #{organisation.name}: Name: #{dpo.name}, Postal Address: #{organisation.address_row}, E-mail address: #{dpo.email}, Telephone number: #{organisation.phone}") end end end @@ -86,30 +88,32 @@ RSpec.describe "organisations/data_sharing_agreement.html.erb", :aggregate_failu expect(fragment).to have_content("12.2. For #{organisation.name}: Name: [DPO name], Postal Address: #{organisation.address_row}, E-mail address: [DPO email], Telephone number: #{organisation.phone}") end - context "when accepted" do - let(:data_sharing_agreement) do + context "when confirmed" do + let(:data_protection_confirmation) do create( - :data_sharing_agreement, + :data_protection_confirmation, organisation:, - signed_at: Time.zone.now - 1.day, + created_at: Time.zone.now - 1.day, ) end + let(:dpo) { data_protection_confirmation.data_protection_officer } + it "renders dynamic content" do render # sees signed_at date expect(fragment).to have_content("9th day of January 2023") # dpo name placedholder - expect(fragment).to have_content("Name: #{data_sharing_agreement.dpo_name}") + expect(fragment).to have_content("Name: #{dpo.name}") # org details - expect(fragment).to have_content("#{data_sharing_agreement.organisation_name} of #{data_sharing_agreement.organisation_address} (“CORE Data Provider”)") + expect(fragment).to have_content("#{organisation.name} of #{organisation.address_row} (“CORE Data Provider”)") # header - expect(fragment).to have_css("h2", text: "#{data_sharing_agreement.organisation_name} and Department for Levelling Up, Housing and Communities") + expect(fragment).to have_css("h2", text: "#{organisation.name} and Department for Levelling Up, Housing and Communities") # does not show action buttons expect(fragment).not_to have_button(text: "Accept this agreement") expect(fragment).not_to have_link(text: "Cancel", href: "/organisations/#{organisation.id}/details") # Shows filled in details in 12.2 - expect(fragment).to have_content("12.2. For #{data_sharing_agreement.organisation_name}: Name: #{data_sharing_agreement.dpo_name}, Postal Address: #{data_sharing_agreement.organisation_address}, E-mail address: #{data_sharing_agreement.dpo_email}, Telephone number: #{data_sharing_agreement.organisation_phone_number}") + expect(fragment).to have_content("12.2. For #{organisation.name}: Name: #{dpo.name}, Postal Address: #{organisation.address_row}, E-mail address: #{dpo.email}, Telephone number: #{organisation.phone}") end end end diff --git a/spec/views/organisations/show.html.erb_spec.rb b/spec/views/organisations/show.html.erb_spec.rb index ea392d76d..47ec10df4 100644 --- a/spec/views/organisations/show.html.erb_spec.rb +++ b/spec/views/organisations/show.html.erb_spec.rb @@ -4,8 +4,7 @@ RSpec.describe "organisations/show.html.erb" do before do Timecop.freeze(Time.zone.local(2023, 1, 10)) allow(view).to receive(:current_user).and_return(user) - assign(:organisation, organisation) - organisation.update!(data_sharing_agreement:) + assign(:organisation, user.organisation) end after do @@ -13,14 +12,14 @@ RSpec.describe "organisations/show.html.erb" do end let(:fragment) { Capybara::Node::Simple.new(rendered) } - let(:organisation) { user.organisation } - let(:data_sharing_agreement) { nil } + let(:organisation_without_dpc) { create(:organisation, :without_dpc) } + let(:organisation_with_dsa) { create(:organisation) } context "when flag disabled" do - let(:user) { create(:user) } + let(:user) { create(:user, organisation: organisation_without_dpc) } before do - allow(FeatureToggle).to receive(:new_data_sharing_agreement?).and_return(false) + allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) end it "does not include data sharing agreement row" do @@ -31,7 +30,7 @@ RSpec.describe "organisations/show.html.erb" do end context "when dpo" do - let(:user) { create(:user, is_dpo: true) } + let(:user) { create(:user, is_dpo: true, organisation: organisation_without_dpc) } it "includes data sharing agreement row" do render @@ -48,11 +47,11 @@ RSpec.describe "organisations/show.html.erb" do it "shows link to view data sharing agreement" do render - expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation.id}/data-sharing-agreement") + expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation_without_dpc.id}/data-sharing-agreement") end context "when accepted" do - let(:data_sharing_agreement) { create(:data_sharing_agreement, organisation:, signed_at: Time.zone.now - 1.day) } + let(:user) { create(:user, organisation: organisation_with_dsa) } it "includes data sharing agreement row" do render @@ -69,13 +68,13 @@ RSpec.describe "organisations/show.html.erb" do it "shows link to view data sharing agreement" do render - expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation.id}/data-sharing-agreement") + expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation_with_dsa.id}/data-sharing-agreement") end end end context "when support user" do - let(:user) { create(:user, :support) } + let(:user) { create(:user, :support, organisation: organisation_without_dpc) } it "includes data sharing agreement row" do render @@ -98,11 +97,11 @@ RSpec.describe "organisations/show.html.erb" do it "shows link to view data sharing agreement" do render - expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation.id}/data-sharing-agreement") + expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation_without_dpc.id}/data-sharing-agreement") end context "when accepted" do - let(:data_sharing_agreement) { create(:data_sharing_agreement, organisation:, signed_at: Time.zone.now - 1.day) } + let(:user) { create(:user, :support, organisation: organisation_with_dsa) } it "includes data sharing agreement row" do render @@ -113,25 +112,25 @@ RSpec.describe "organisations/show.html.erb" do it "shows data sharing agreement accepted with date" do render - expect(fragment).to have_content("Accepted 09/01/2023") + expect(fragment).to have_content("Accepted 10/01/2023") end it "shows show name of who signed the agreement" do render - expect(fragment).to have_content(data_sharing_agreement.dpo_name) + expect(fragment).to have_content(user.organisation.data_protection_confirmation.data_protection_officer.name) end it "shows link to view data sharing agreement" do render - expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation.id}/data-sharing-agreement") + expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation_with_dsa.id}/data-sharing-agreement") end end end context "when not dpo" do - let(:user) { create(:user) } + let(:user) { create(:user, organisation: organisation_without_dpc) } it "includes data sharing agreement row" do render @@ -151,13 +150,11 @@ RSpec.describe "organisations/show.html.erb" do it "shows link to view data sharing agreement" do render - expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation.id}/data-sharing-agreement") + expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation_without_dpc.id}/data-sharing-agreement") end context "when accepted" do - let(:data_sharing_agreement) do - create(:data_sharing_agreement, organisation:, signed_at: Time.zone.now - 1.day) - end + let(:user) { create(:user, organisation: organisation_with_dsa) } it "includes data sharing agreement row" do render @@ -172,7 +169,7 @@ RSpec.describe "organisations/show.html.erb" do it "shows link to view data sharing agreement" do render - expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation.id}/data-sharing-agreement") + expect(fragment).to have_link(text: "View agreement", href: "/organisations/#{organisation_with_dsa.id}/data-sharing-agreement") end end end