Browse Source

CLDC-3614: Allow markdown page content when creating notifications (#2622)

* CLDC-3614: Allow markdown page content when creating notifications

* Try using beginning of day for time in failing test

* Remove ignored breaks

* Add paper trail to notifications

* CLDC-3607: Allow support users to delete notifications (by setting their end date) (#2630)

* CLDC-3607: Allow support users to delete notifications (by setting their end date)

* Refactor render_for_page helper

* Move db queries to homepage presenter

* Fix lint

* Use a before_action to find notification in controller

* Make delete notification links red

* Tweak link to markdown guide
pull/2613/head
Rachael Booth 2 years ago committed by GitHub
parent
commit
03a6b83d9a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
  1. 62
      app/controllers/notifications_controller.rb
  2. 68
      app/helpers/notifications_helper.rb
  3. 17
      app/models/notification.rb
  4. 3
      app/presenters/homepage_presenter.rb
  5. 4
      app/views/home/index.html.erb
  6. 8
      app/views/notifications/_form.html.erb
  7. 2
      app/views/notifications/_notification_banner.html.erb
  8. 17
      app/views/notifications/_notification_home_section.html.erb
  9. 19
      app/views/notifications/check_answers.html.erb
  10. 27
      app/views/notifications/delete_confirmation.html.erb
  11. 5
      app/views/notifications/show.html.erb
  12. 9
      config/locales/en.yml
  13. 2
      config/routes.rb
  14. 5
      db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb
  15. 15
      db/schema.rb
  16. 1
      spec/factories/notification.rb
  17. 4
      spec/models/lettings_log_derived_fields_spec.rb
  18. 37
      spec/models/notification_spec.rb
  19. 65
      spec/requests/notifications_controller_spec.rb

62
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

68
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
%(<h#{header_level} class="govuk-heading-#{header_size}">#{text}</h#{header_level}>)
end
def paragraph(text)
return %(<p class="govuk-!-font-weight-bold">#{text}</p>) if @bold # rubocop:disable Rails/HelperInstanceVariable
%(<p>#{text}</p>)
%(<p class="govuk-body-m">#{text}</p>)
end
def list(contents, list_type)
return %(<ol class="govuk-list govuk-list--number">#{contents}</ol>) if list_type == :ordered
%(<ul class="govuk-list govuk-list--bullet">#{contents}</ul>)
end
def hrule
%(<hr class="govuk-section-break govuk-section-break--xl govuk-section-break--visible">)
end
def block_quote(quote)
%(<div class="govuk-inset-text">#{quote}</div>)
end
end

17
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

3
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

4
app/views/home/index.html.erb

@ -7,9 +7,7 @@
</div>
<% if @current_user.support? %>
<div class="govuk-grid-row">
<%= govuk_button_link_to "Create a new notification", new_notification_path %>
</div>
<%= render partial: "notifications/notification_home_section", locals: { active_notifications: @homepage_presenter.active_notifications } %>
<% end %>
<div class="app-data-box-section govuk-grid-row">

8
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 %>
<span class="govuk-caption-m govuk-!-margin-bottom-6">View <%= govuk_link_to "markdown syntax guide at markdownguide.org", "https://www.markdownguide.org/basic-syntax/" %></span>
<span class="govuk-caption-m govuk-!-margin-bottom-6"><%= govuk_link_to "Find out more about using Markdown at Markdown Guide", "https://www.markdownguide.org/basic-syntax/" %></span>
<%= f.govuk_submit "Continue" %>
</div>

2
app/views/notifications/_notification_banner.html.erb

@ -7,7 +7,7 @@
<p>Notification 1 of <%= notification_count %></p>
<% end %>
<%== render_for_banner(notification.title) %>
<% if notification.page_content.present? %>
<% if notification.show_additional_page %>
<div class="govuk-body">
<%= govuk_link_to notification.link_text, notification_path(notification), class: "govuk-link--inverse govuk-!-font-weight-bold" %>
</div>

17
app/views/notifications/_notification_home_section.html.erb

@ -0,0 +1,17 @@
<div class="govuk-grid-row">
<p class="govuk-!-font-weight-bold"><%== I18n.t("active_notifications", count: active_notifications.count) %></p>
<% active_notifications.each do |notification| %>
<div class="govuk-grid-row">
<div class="govuk-grid-column-three-quarters">
<%== render_for_home(notification) %>
</div>
<div class="govuk-grid-column-one-quarter">
<%= govuk_link_to("Delete notification", notification_delete_confirmation_path(notification), class: "app-!-colour-red") %>
</div>
</div>
<% end %>
</div>
<div class="govuk-grid-row">
<%= govuk_button_link_to "Create a new notification", new_notification_path %>
</div>

19
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 %>
</div>
</div>

27
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 %>
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds-from-desktop">
<h1 class="govuk-heading-xl">
<%= content_for(:title) %>
</h1>
<p class="govuk-body"><%== render_for_summary("**Notification:** #{@notification.title}") %></p>
<p class="govuk-body">Users will no longer see this notification.</p>
<%= govuk_warning_text(text: "You will not be able to undo this action.") %>
<div class="govuk-button-group">
<%= govuk_button_to(
"Delete notification",
notification_delete_path(@notification),
method: :delete,
) %>
<%= govuk_button_link_to "Cancel", root_path, html: { method: :get }, secondary: true %>
</div>
</div>
</div>

5
app/views/notifications/show.html.erb

@ -5,10 +5,7 @@
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<h1 class="govuk-heading-xl"><%= @notification.title %></h1>
<p>
<%= sanitize @notification.page_content %>
</p>
<%== render_for_page(@notification) %>
</div>
</div>
<br>

9
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:

2
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

5
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

15
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"

1
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

4
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

37
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

65
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

Loading…
Cancel
Save