diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 22f284992..d0181ead6 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -1,16 +1,21 @@ class NotificationsController < ApplicationController before_action :authenticate_user!, except: %i[show] before_action :authenticate_scope!, except: %i[show dismiss] + before_action :find_notification, except: %i[new create] + + rescue_from ActiveRecord::RecordNotFound, with: :render_not_found def dismiss - @notification = Notification.find_by(id: params[:notification_id]) @notification.mark_as_read! for: current_user redirect_back(fallback_location: root_path) end def show - @notification = Notification.find_by(id: params[:id]) - if @notification&.page_content && (@notification&.show_on_unauthenticated_pages || current_user) + if !@notification.show_on_unauthenticated_pages && !current_user + render_not_found and return + end + + if @notification.show_additional_page render "show" else redirect_back(fallback_location: root_path) @@ -22,7 +27,7 @@ class NotificationsController < ApplicationController end def create - @notification = Notification.new(notification_params) + @notification = Notification.new(notification_model_params) if @notification.errors.empty? && @notification.save redirect_to notification_check_answers_path(@notification) @@ -31,27 +36,10 @@ class NotificationsController < ApplicationController end end - def check_answers - @notification = Notification.find_by(id: params[:notification_id]) - render_not_found and return unless @notification - - render "notifications/check_answers" - end - - def edit - @notification = Notification.find_by(id: params[:id]) - end - def update - @notification = Notification.find_by(id: params[:id]) - render_not_found and return unless @notification - start_now = params[:notification][:start_now] - update_params = notification_params.except(:start_now) - update_params[:start_date] = Time.zone.now if start_now - - if @notification.errors.empty? && @notification.update(update_params) + if @notification.errors.empty? && @notification.update(notification_model_params) if start_now flash[:notice] = "The notification has been created" redirect_to root_path @@ -65,13 +53,41 @@ class NotificationsController < ApplicationController end end + def delete + @notification.update!(end_date: Time.zone.now) + flash[:notice] = "The notification has been deleted" + redirect_to root_path + end + private def notification_params - params.require(:notification).permit(:title, :show_on_unauthenticated_pages, :start_now) + params.require(:notification).permit(:title, :show_on_unauthenticated_pages, :show_additional_page, :link_text, :page_content, :start_now) end def authenticate_scope! render_not_found unless current_user.support? end + + def notification_model_params + model_params = notification_params.except(:start_now) + + if notification_params[:show_additional_page] == "0" + model_params[:link_text] = nil + model_params[:page_content] = nil + end + + model_params[:start_date] = Time.zone.now if notification_params[:start_now] + + model_params + end + + def find_notification + id = params[:id] || params[:notification_id] + @notification = current_user&.support? ? Notification.find_by(id:) : Notification.active.find_by(id:) + + raise ActiveRecord::RecordNotFound unless @notification + + @notification + end end diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index c9226ad3d..318918134 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -16,17 +16,44 @@ module NotificationsHelper end def render_for_banner(title) - banner_renderer = NotificationTitleRenderer.new({ invert_link_colour: true, bold_all_text: true }) - Redcarpet::Markdown.new(banner_renderer, no_intra_emphasis: true).render(title) + # rubocop:disable Rails/HelperInstanceVariable + @banner_renderer ||= NotificationRenderer.new({ invert_link_colour: true, bold_all_text: true }) + @banner_markdown ||= Redcarpet::Markdown.new(@banner_renderer, no_intra_emphasis: true) + @banner_markdown.render(title) + # rubocop:enable Rails/HelperInstanceVariable end def render_for_summary(title) - plain_title_renderer = NotificationTitleRenderer.new({ invert_link_colour: false, bold_all_text: false }) - Redcarpet::Markdown.new(plain_title_renderer, no_intra_emphasis: true).render(title) + render_normal_markdown(title) + end + + def render_for_page(notification) + content_includes_own_title = /\A\s*#[^#]/.match?(notification.page_content) + return render_normal_markdown(notification.page_content) if content_includes_own_title + + content = "# #{notification.title}\n#{notification.page_content}" + render_normal_markdown(content) + end + + def render_for_home(notification) + return render_normal_markdown(notification.title) unless notification.show_additional_page + + content = "#{notification.title} \n[#{notification.link_text}](#{notification_path(notification)})" + render_normal_markdown(content) + end + +private + + def render_normal_markdown(content) + # rubocop:disable Rails/HelperInstanceVariable + @on_page_renderer ||= NotificationRenderer.new({ invert_link_colour: false, bold_all_text: false }) + @on_page_markdown ||= Redcarpet::Markdown.new(@on_page_renderer, no_intra_emphasis: true) + @on_page_markdown.render(content) + # rubocop:enable Rails/HelperInstanceVariable end end -class NotificationTitleRenderer < Redcarpet::Render::HTML +class NotificationRenderer < Redcarpet::Render::HTML def initialize(options = {}) link_class = "govuk-link" link_class += " govuk-link--inverse" if options[:invert_link_colour] @@ -35,9 +62,38 @@ class NotificationTitleRenderer < Redcarpet::Render::HTML super base_options end + def header(text, header_level) + header_size = case header_level + when 1 + "xl" + when 2 + "l" + when 3 + "m" + else + "s" + end + + %(#{text}) + end + def paragraph(text) return %(

#{text}

) if @bold # rubocop:disable Rails/HelperInstanceVariable - %(

#{text}

) + %(

#{text}

) + end + + def list(contents, list_type) + return %(
    #{contents}
) if list_type == :ordered + + %() + end + + def hrule + %(
) + end + + def block_quote(quote) + %(
#{quote}
) end end diff --git a/app/models/notification.rb b/app/models/notification.rb index 7811c7a4b..cd7c9972a 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -1,7 +1,10 @@ class Notification < ApplicationRecord acts_as_readable + has_paper_trail + validates :title, presence: { message: I18n.t("activerecord.errors.models.notification.attributes.title.blank") } + validate :validate_additional_page_information scope :active, -> { where("start_date <= ? AND (end_date >= ? OR end_date is null)", Time.zone.now, Time.zone.now) } scope :unauthenticated, -> { where(show_on_unauthenticated_pages: true) } @@ -13,4 +16,18 @@ class Notification < ApplicationRecord def self.newest_active_unauthenticated_notification active_unauthenticated_notifications.last end + +private + + def validate_additional_page_information + return unless show_additional_page + + if link_text.blank? + errors.add :link_text, I18n.t("activerecord.errors.models.notification.attributes.link_text.blank_when_additional_page_set") + end + + if page_content.blank? + errors.add :page_content, I18n.t("activerecord.errors.models.notification.attributes.page_content.blank_when_additional_page_set") + end + end end diff --git a/app/presenters/homepage_presenter.rb b/app/presenters/homepage_presenter.rb index 1cb784e3a..3b3314667 100644 --- a/app/presenters/homepage_presenter.rb +++ b/app/presenters/homepage_presenter.rb @@ -2,7 +2,7 @@ class HomepagePresenter include Rails.application.routes.url_helpers include CollectionTimeHelper - attr_reader :current_year_in_progress_lettings_data, :current_year_completed_lettings_data, :current_year_in_progress_sales_data, :current_year_completed_sales_data, :last_year_in_progress_lettings_data, :last_year_completed_lettings_data, :last_year_in_progress_sales_data, :last_year_completed_sales_data, :incomplete_schemes_data + attr_reader :current_year_in_progress_lettings_data, :current_year_completed_lettings_data, :current_year_in_progress_sales_data, :current_year_completed_sales_data, :last_year_in_progress_lettings_data, :last_year_completed_lettings_data, :last_year_in_progress_sales_data, :last_year_completed_sales_data, :incomplete_schemes_data, :active_notifications def initialize(user) @user = user @@ -25,6 +25,7 @@ class HomepagePresenter path: schemes_path(status: [:incomplete], owning_organisation_select: "all"), } end + @active_notifications = Notification.active if @user.support? end def title_text_for_user diff --git a/app/views/home/index.html.erb b/app/views/home/index.html.erb index bb4792e68..e55dc6c99 100644 --- a/app/views/home/index.html.erb +++ b/app/views/home/index.html.erb @@ -7,9 +7,7 @@ <% if @current_user.support? %> -
- <%= govuk_button_link_to "Create a new notification", new_notification_path %> -
+ <%= render partial: "notifications/notification_home_section", locals: { active_notifications: @homepage_presenter.active_notifications } %> <% end %>
diff --git a/app/views/notifications/_form.html.erb b/app/views/notifications/_form.html.erb index 2a46998f7..76a275234 100644 --- a/app/views/notifications/_form.html.erb +++ b/app/views/notifications/_form.html.erb @@ -18,11 +18,15 @@ label: { text: "Title", size: "m" }, hint: { text: "Use markdown for links to existing pages" } %> - <%= f.govuk_check_boxes_fieldset :show_on_unauthenticated_pages, multiple: false, legend: { text: "Display Options" } do %> + <%= f.govuk_check_boxes_fieldset :display_options, multiple: false, legend: { text: "Display Options" } do %> <%= f.govuk_check_box :show_on_unauthenticated_pages, 1, 0, multiple: false, label: { text: "Show this notification on unauthenticated pages, for example the start page" } %> + <%= f.govuk_check_box :show_additional_page, 1, 0, multiple: false, label: { text: "Include a link to a separate page with additional information" } do %> + <%= f.govuk_text_field :link_text, label: { text: "Link text" }, hint: { text: "Use descriptive language and relevant terms. The link text should make sense out of context." } %> + <%= f.govuk_text_area :page_content, label: { text: "Page content" }, hint: { text: "Use markdown to format the page content. The page title will be the notification title by default. Use a heading level one if you want to override it." } %> + <% end %> <% end %> - View <%= govuk_link_to "markdown syntax guide at markdownguide.org", "https://www.markdownguide.org/basic-syntax/" %> + <%= govuk_link_to "Find out more about using Markdown at Markdown Guide", "https://www.markdownguide.org/basic-syntax/" %> <%= f.govuk_submit "Continue" %>
diff --git a/app/views/notifications/_notification_banner.html.erb b/app/views/notifications/_notification_banner.html.erb index 512582109..a9cc8bdff 100644 --- a/app/views/notifications/_notification_banner.html.erb +++ b/app/views/notifications/_notification_banner.html.erb @@ -7,7 +7,7 @@

Notification 1 of <%= notification_count %>

<% end %> <%== render_for_banner(notification.title) %> - <% if notification.page_content.present? %> + <% if notification.show_additional_page %>
<%= govuk_link_to notification.link_text, notification_path(notification), class: "govuk-link--inverse govuk-!-font-weight-bold" %>
diff --git a/app/views/notifications/_notification_home_section.html.erb b/app/views/notifications/_notification_home_section.html.erb new file mode 100644 index 000000000..522bc2840 --- /dev/null +++ b/app/views/notifications/_notification_home_section.html.erb @@ -0,0 +1,17 @@ +
+

<%== I18n.t("active_notifications", count: active_notifications.count) %>

+ <% active_notifications.each do |notification| %> +
+
+ <%== render_for_home(notification) %> +
+
+ <%= govuk_link_to("Delete notification", notification_delete_confirmation_path(notification), class: "app-!-colour-red") %> +
+
+ <% end %> +
+ +
+ <%= govuk_button_link_to "Create a new notification", new_notification_path %> +
diff --git a/app/views/notifications/check_answers.html.erb b/app/views/notifications/check_answers.html.erb index 78a04e2b0..8ddbc0abe 100644 --- a/app/views/notifications/check_answers.html.erb +++ b/app/views/notifications/check_answers.html.erb @@ -31,6 +31,25 @@ <% row.with_value { @notification.show_on_unauthenticated_pages ? "Yes" : "No" } %> <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> <% end %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Link to additional information?" } %> + <% row.with_value { @notification.show_additional_page ? "Yes" : "No" } %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% if @notification.show_additional_page %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Link text" } %> + <% row.with_value { @notification.link_text } %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Page content" } %> + <% row.with_value do %> + <%= govuk_link_to "View page preview", notification_path(@notification), new_tab: true %> + <% end %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% end %> <% end %> diff --git a/app/views/notifications/delete_confirmation.html.erb b/app/views/notifications/delete_confirmation.html.erb new file mode 100644 index 000000000..4107c3050 --- /dev/null +++ b/app/views/notifications/delete_confirmation.html.erb @@ -0,0 +1,27 @@ +<% content_for :before_content do %> + <% content_for :title, "Are you sure you want to delete this notification?" %> + <%= govuk_back_link(href: :back) %> +<% end %> + +
+
+

+ <%= content_for(:title) %> +

+ +

<%== render_for_summary("**Notification:** #{@notification.title}") %>

+ +

Users will no longer see this notification.

+ + <%= govuk_warning_text(text: "You will not be able to undo this action.") %> + +
+ <%= govuk_button_to( + "Delete notification", + notification_delete_path(@notification), + method: :delete, + ) %> + <%= govuk_button_link_to "Cancel", root_path, html: { method: :get }, secondary: true %> +
+
+
diff --git a/app/views/notifications/show.html.erb b/app/views/notifications/show.html.erb index abdc77044..a3933db55 100644 --- a/app/views/notifications/show.html.erb +++ b/app/views/notifications/show.html.erb @@ -5,10 +5,7 @@
-

<%= @notification.title %>

-

- <%= sanitize @notification.page_content %> -

+ <%== render_for_page(@notification) %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index 6a37a81ad..66d7d2531 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -41,6 +41,11 @@ en: create_password: "Create a password to finish setting up your account" reset_password: "Reset your password" + active_notifications: + zero: "There are no active notifications" + one: "There is one active notification:" + other: "There are %{count} active notifications:" + activemodel: errors: models: @@ -190,6 +195,10 @@ en: attributes: title: blank: "Enter a title" + link_text: + blank_when_additional_page_set: "Enter the link text" + page_content: + blank_when_additional_page_set: "Enter the page content" notification: logs_deleted: diff --git a/config/routes.rb b/config/routes.rb index 507a8da2a..73a7239ca 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -141,6 +141,8 @@ Rails.application.routes.draw do resources :notifications do get "dismiss", to: "notifications#dismiss" get "check-answers", to: "notifications#check_answers" + get "delete-confirmation", to: "notifications#delete_confirmation" + delete "delete", to: "notifications#delete" end resources :organisations do diff --git a/db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb b/db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb new file mode 100644 index 000000000..c0992bb62 --- /dev/null +++ b/db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb @@ -0,0 +1,5 @@ +class AddShowAdditionalPageColumnToNotifications < ActiveRecord::Migration[7.0] + def change + add_column :notifications, :show_additional_page, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 00159931b..ce07c8570 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do +ActiveRecord::Schema[7.0].define(version: 2024_09_04_135306) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -54,7 +54,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do t.datetime "last_accessed" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.check_constraint "log_type::text = ANY (ARRAY['lettings'::character varying::text, 'sales'::character varying::text])", name: "log_type_check" + t.check_constraint "log_type::text = ANY (ARRAY['lettings'::character varying, 'sales'::character varying]::text[])", name: "log_type_check" t.check_constraint "year >= 2000 AND year <= 2099", name: "year_check" end @@ -205,14 +205,14 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do t.integer "hb" t.integer "hbrentshortfall" t.integer "property_relet" - t.datetime "mrcdate", precision: nil + t.datetime "mrcdate" t.integer "incref" - t.datetime "startdate", precision: nil + t.datetime "startdate" t.integer "armedforces" t.integer "first_time_property_let_as_social_housing" t.integer "unitletas" t.integer "builtype" - t.datetime "voiddate", precision: nil + t.datetime "voiddate" t.bigint "owning_organisation_id" t.bigint "managing_organisation_id" t.integer "renttype" @@ -453,6 +453,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do t.boolean "show_on_unauthenticated_pages" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "show_additional_page" end create_table "organisation_relationships", force: :cascade do |t| @@ -773,8 +774,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do t.string "name" t.bigint "organisation_id" t.integer "sign_in_count", default: 0, null: false - t.datetime "current_sign_in_at", precision: nil - t.datetime "last_sign_in_at", precision: nil + t.datetime "current_sign_in_at" + t.datetime "last_sign_in_at" t.string "current_sign_in_ip" t.string "last_sign_in_ip" t.integer "role" diff --git a/spec/factories/notification.rb b/spec/factories/notification.rb index 05d4faa3f..92d4bf4c9 100644 --- a/spec/factories/notification.rb +++ b/spec/factories/notification.rb @@ -6,5 +6,6 @@ FactoryBot.define do start_date { Time.zone.yesterday } end_date { Time.zone.tomorrow } show_on_unauthenticated_pages { false } + show_additional_page { true } end end diff --git a/spec/models/lettings_log_derived_fields_spec.rb b/spec/models/lettings_log_derived_fields_spec.rb index 54dc438bc..aae1396d7 100644 --- a/spec/models/lettings_log_derived_fields_spec.rb +++ b/spec/models/lettings_log_derived_fields_spec.rb @@ -981,9 +981,9 @@ RSpec.describe LettingsLog, type: :model do end describe "deriving voiddate from startdate" do - let(:startdate) { Time.zone.now } + let(:startdate) { Time.zone.now.beginning_of_day } - it "correctly derives voiddate if the letting is a renewal and clears it if it is not" do + it "correctly derives voiddate if the letting is a renewal" do log.assign_attributes(renewal: 1, startdate:) expect { log.set_derived_fields! }.to change(log, :voiddate).to startdate diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb new file mode 100644 index 000000000..86c02476f --- /dev/null +++ b/spec/models/notification_spec.rb @@ -0,0 +1,37 @@ +require "rails_helper" + +RSpec.describe Notification, type: :model do + describe "#valid?" do + context "when show additional page is true" do + context "and page_content is blank" do + let(:notification) { build(:notification, show_additional_page: true, page_content: "") } + + it "adds an error to page_content" do + notification.valid? + + expect(notification.errors[:page_content]).to include("Enter the page content") + end + end + + context "and link_text is blank" do + let(:notification) { build(:notification, show_additional_page: true, link_text: nil) } + + it "adds an error to link_text" do + notification.valid? + + expect(notification.errors[:link_text]).to include("Enter the link text") + end + end + end + + context "when show additional page is false" do + context "and page_content and link_text are blank" do + let(:notification) { build(:notification, show_additional_page: false, link_text: nil, page_content: nil) } + + it "is valid" do + expect(notification).to be_valid + end + end + end + end +end diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb index fbd6f55cd..0982057ab 100644 --- a/spec/requests/notifications_controller_spec.rb +++ b/spec/requests/notifications_controller_spec.rb @@ -13,12 +13,15 @@ RSpec.describe NotificationsController, type: :request do let(:request) { post notifications_path, params: params } context "with valid parameters" do - let(:params) { { "notification": { title: "Test Create", show_on_unauthenticated_pages: true } } } + let(:params) { { "notification": { title: "Test Create", show_on_unauthenticated_pages: "1", show_additional_page: "1", link_text: "link", page_content: "page" } } } it "creates a new notification with no start date set" do request notification = Notification.find_by(title: "Test Create") - expect(notification.show_on_unauthenticated_pages).to be_truthy + expect(notification.show_on_unauthenticated_pages).to be(true) + expect(notification.show_additional_page).to be(true) + expect(notification.link_text).to eq("link") + expect(notification.page_content).to eq("page") expect(notification.start_date).to be_nil end @@ -30,13 +33,24 @@ RSpec.describe NotificationsController, type: :request do end context "with invalid parameters" do - let(:params) { { "notification": { title: "", show_on_unauthenticated_pages: true } } } + let(:params) { { "notification": { title: "", show_on_unauthenticated_pages: "1" } } } it "gives an error response" do request expect(response).to have_http_status(:unprocessable_entity) end end + + context "when show additional page is false" do + let(:params) { { "notification": { title: "No Additional Page", show_on_unauthenticated_pages: "1", show_additional_page: "0", link_text: "text", page_content: "content" } } } + + it "ignores values for link_text and page_content" do + request + notification = Notification.find_by(title: "No Additional Page") + expect(notification.link_text).to be_nil + expect(notification.page_content).to be_nil + end + end end describe "#update" do @@ -60,7 +74,7 @@ RSpec.describe NotificationsController, type: :request do end context "when start_now is not set" do - let(:params) { { "notification": { title: "Updated Title", show_on_unauthenticated_pages: true } } } + let(:params) { { "notification": { title: "Updated Title", show_on_unauthenticated_pages: "1" } } } it "sets the relevant values on the notification" do request @@ -74,6 +88,29 @@ RSpec.describe NotificationsController, type: :request do expect(response).to redirect_to(notification_check_answers_path(notification)) end end + + context "when show additional page is false" do + let(:notification) { create(:notification, show_additional_page: "0", link_text: "link", page_content: "page") } + let(:params) { { "notification": { show_additional_page: "0", link_text: "text", page_content: "content" } } } + + it "removes values for link_text and page_content" do + request + notification.reload + expect(notification.link_text).to be_nil + expect(notification.page_content).to be_nil + end + end + end + + describe "#delete" do + let(:notification) { create(:notification, end_date: nil) } + let(:request) { delete notification_delete_path(notification) } + + it "sets end_date on the notification" do + request + notification.reload + expect(notification.end_date).to be < Time.zone.now + end end end @@ -85,7 +122,7 @@ RSpec.describe NotificationsController, type: :request do end describe "#create" do - let(:request) { post notifications_path, params: { "notification": { title: "Test Create", show_on_unauthenticated_pages: true } } } + let(:request) { post notifications_path, params: { "notification": { title: "Test Create" } } } it "returns not found" do request @@ -99,12 +136,28 @@ RSpec.describe NotificationsController, type: :request do describe "#update" do let(:notification) { create(:notification) } - let(:request) { patch notification_path(notification), params: { "notification": { title: "Test Create", show_on_unauthenticated_pages: true } } } + let(:request) { patch notification_path(notification), params: { "notification": { title: "Test Update" } } } it "returns not found" do request expect(response).to have_http_status(:not_found) end end + + describe "#delete" do + let(:notification) { create(:notification, end_date: nil) } + let(:request) { delete notification_delete_path(notification) } + + it "returns not found" do + request + expect(response).to have_http_status(:not_found) + end + + it "does not set end_date on the notification" do + request + notification.reload + expect(notification.end_date).to be_nil + end + end end end