Skip to content

Commit

Permalink
Rename ChangeNote to Notification
Browse files Browse the repository at this point in the history
Change notes have been used in the application as a log of changes we will notify subscribers about.
Each ones also has an optional 'note' that content can use to describe the nature of the change to subscribers.
This has generated historic confusion for developers and makes these concepts difficult for new starters to instantly grasp.
  • Loading branch information
huwd committed Oct 4, 2019
1 parent a0d9822 commit e1e194e
Show file tree
Hide file tree
Showing 11 changed files with 52 additions and 52 deletions.
4 changes: 2 additions & 2 deletions app/helpers/brexit_checker_helper.rb
Expand Up @@ -52,12 +52,12 @@ def previous_question_index(all_questions:, criteria_keys: [], current_question_
previous_questions.rindex { |question| question.show?(criteria_keys) }
end

def change_note_email_link(non_tracked_url, change_note)
def notification_email_link(non_tracked_url, notification)
url = Addressable::URI.parse(non_tracked_url)
return non_tracked_url unless url.host == "www.gov.uk"

url.query_values = (url.query_values || {}).merge(
utm_source: change_note.id,
utm_source: notification.id,
utm_medium: "email",
utm_campaign: "govuk-brexit-checker",
)
Expand Down
6 changes: 3 additions & 3 deletions app/mailers/brexit_checker_mailer.rb
@@ -1,9 +1,9 @@
class BrexitCheckerMailer < ApplicationMailer
add_template_helper(BrexitCheckerHelper)

def change_notification(change_note)
@change_note = change_note
@action = @change_note.action
def change_notification(notification)
@notification = notification
@action = @notification.action
mail(subject: subject)
end

Expand Down
12 changes: 6 additions & 6 deletions app/views/brexit_checker_mailer/change_notification.text.erb
@@ -1,7 +1,7 @@
<%= t("brexit_checker_mailer.change_notification.#{@change_note.type}") %>
<%= t("brexit_checker_mailer.change_notification.#{@notification.type}") %>
<% if @action.title_url.present? %>
[<%= @action.title %>](<%= change_note_email_link(@action.title_url, @change_note) %>)
[<%= @action.title %>](<%= notification_email_link(@action.title_url, @notification) %>)
<% else %>
<%= @action.title %>
<% end %>
Expand All @@ -12,13 +12,13 @@
<% if @action.guidance_url.present? %>
<%= @action.guidance_prompt || t("brexit_checker_mailer.change_notification.guidance_prompt") %>
[<%= @action.guidance_link_text %>](<%= change_note_email_link(@action.guidance_url, @change_note) %>)
[<%= @action.guidance_link_text %>](<%= notification_email_link(@action.guidance_url, @notification) %>)
<% end %>

Updated
<%= Date.parse(@change_note.date).strftime("%-d %B %Y") %>
<%= Date.parse(@notification.date).strftime("%-d %B %Y") %>
<% if @change_note.note.present? %>
<% if @notification.note.present? %>
Changes made
<%= @change_note.note %>
<%= @notification.note %>
<% end %>
16 changes: 8 additions & 8 deletions docs/make-changes-to-brexit-checker-content.md
Expand Up @@ -8,13 +8,13 @@ Zendesk.
### Contents:

- [Updates to actions](#updates-to-actions)
- [Adding change notes](#adding-change-notes)
- [Adding notifications](#adding-notifications)

## Updates to actions

Actions are defined in [an `actions.yaml` file](https://github.com/alphagov/finder-frontend/blob/master/lib/brexit_checker/actions.yaml). To add or change an action, you'll need to run one of the following rake tasks. When the file has changed in multiple ways, only commit the changes that were requested.

**NOTE: Additions or changes to actions may require a [change note](#adding-change-notes). Check with the person who requested the change.**
**NOTE: Additions or changes to actions may require a [notification](#adding-notifications). Check with the person who requested the change.**

**NOTE: It's a good idea to run `bundle exec rspec spec/integration/brexit_checker_spec.rb` to validate the Yaml locally, before raising a PR.**

Expand Down Expand Up @@ -43,21 +43,21 @@ Run this take task:
bundle exec rake brexit_checker:convert_csv_to_yaml:actions[path/to/actions.csv]`
```

## Adding change notes
## Adding notifications

Additions or changes to actions may require a change note, which is used to send a notification about the change. Change notes should only be created for this purpose.
Additions or changes to actions may require a notification, which sends an email to alert subscribers to a change. Notifications should only be created for this purpose.

Change notes are defined [in a `change_notes.yaml` file](https://github.com/alphagov/finder-frontend/blob/master/lib/brexit_checker/change_notes.yaml). You should check with the person who requested the change, to determine if a change note is appropriate.
Notifications are defined [in notifications.yaml` file](https://github.com/alphagov/finder-frontend/blob/master/lib/brexit_checker/notifications.yaml). You should check with the person who requested the change, to determine if a notification is appropriate.

When a new change note has been deployed to production, you need to run a rake task to send the notification. The notification will be sent to all subscribers who would see this action on their results page.
When a new Notification has been deployed to production, you need to run a rake task to send the notification email. The notification email will be sent to all subscribers who would see this action on their results page.

https://deploy.blue.production.govuk.digital/job/run-rake-task/parambuild/?TARGET_APPLICATION=finder-frontend&MACHINE_CLASS=calculators_frontend&RAKE_TASK=brexit_checker:change_notification[UUID]

### Notifying a subset of subscribers

Sometimes you will only want to notify a subset of subscribers. For example when content remains the same but the criteria for an action has changed.

Change notes have an optional attribute `criteria`. The structure and format of change note criteria, is the same as criteria from an action.
Notifications have an optional attribute `criteria`. The structure and format of this criteria, is the same as criteria from an action.

You can add criteria to a change note in `change_notes.yaml`. If a record has criteria the rake file will [use these values](https://github.com/alphagov/finder-frontend/blob/0979c94ec51ba38f8d574569ffd51ffea55f13a6/lib/tasks/brexit_checker/change_notifications.rake#L10) to notify subscribers. This will override the default of notifying users based on criteria from the action.
You can add criteria to a notification in `notifications.yaml`. If a record has criteria the rake file will [use these values](https://github.com/alphagov/finder-frontend/blob/0979c94ec51ba38f8d574569ffd51ffea55f13a6/lib/tasks/brexit_checker/change_notifications.rake#L10) to notify subscribers. This will override the default of notifying users based on criteria from the action.

@@ -1,7 +1,7 @@
class BrexitChecker::ChangeNote
class BrexitChecker::Notification
include ActiveModel::Validations

CONFIG_PATH = Rails.root.join("lib", "brexit_checker", "change_notes.yaml")
CONFIG_PATH = Rails.root.join("lib", "brexit_checker", "notifications.yaml")

validates_presence_of :action_id
validates_inclusion_of :type, in: %w(addition content_change)
Expand All @@ -28,7 +28,7 @@ def self.load(params)
end

def self.load_all
@load_all ||= YAML.load_file(CONFIG_PATH)["change_notes"].to_a
@load_all ||= YAML.load_file(CONFIG_PATH)["notifications"].to_a
.map { |c| load(c) }
end

Expand Down
@@ -1,5 +1,5 @@
---
change_notes:
notifications:
- uuid: "5fe018d7-edb1-4d7a-858e-065e46d0917e"
type: addition
action_id: T092
Expand Down
16 changes: 8 additions & 8 deletions lib/tasks/brexit_checker/change_notifications.rake
@@ -1,19 +1,19 @@
namespace :brexit_checker do
desc "Notify Email Alert API subscribers about a change"
task :change_notification, [:change_note_id] => :environment do |_, args|
id = args[:change_note_id]
change_note = BrexitChecker::ChangeNote.find_by_id(id)
raise "Change note not found" if change_note.nil?
task :change_notification, [:notification_id] => :environment do |_, args|
id = args[:notification_id]
notification = BrexitChecker::Notification.find_by_id(id)
raise "Notification not found" if notification.nil?

mail = BrexitCheckerMailer.change_notification(change_note)
mail = BrexitCheckerMailer.change_notification(notification)

criteria = change_note.criteria.presence || change_note.action.criteria
criteria = notification.criteria.presence || notification.action.criteria

GdsApi.email_alert_api.create_message(
title: mail.subject,
url: change_note.action.title_url,
url: notification.action.title_url,
body: mail.body.raw_source,
sender_message_id: change_note.id,
sender_message_id: notification.id,
criteria_rules: criteria_rules(criteria),
)

Expand Down
@@ -1,10 +1,10 @@
FactoryBot.define do
factory :brexit_checker_change_note, class: BrexitChecker::ChangeNote do
factory :brexit_checker_notification, class: BrexitChecker::Notification do
id { SecureRandom.uuid }
action_id { SecureRandom.uuid }
type { "addition" }
date { "2019-08-07" }

initialize_with { BrexitChecker::ChangeNote.new(attributes) }
initialize_with { BrexitChecker::Notification.new(attributes) }
end
end
10 changes: 5 additions & 5 deletions spec/helpers/brexit_checker_helper_spec.rb
Expand Up @@ -192,17 +192,17 @@
end
end

describe "#change_note_email_link" do
let(:change_note) { FactoryBot.build :brexit_checker_change_note }
describe "#notification_email_link" do
let(:notification) { FactoryBot.build :brexit_checker_notification }

it "returns the unchanged link when it's external" do
link = change_note_email_link("http://foo.bar", change_note)
link = notification_email_link("http://foo.bar", notification)
expect(link).to eq("http://foo.bar")
end

it "adds tracking attributes for internal links" do
link = change_note_email_link("http://www.gov.uk", change_note)
expect(link).to match("utm_source=#{change_note.id}")
link = notification_email_link("http://www.gov.uk", notification)
expect(link).to match("utm_source=#{notification.id}")
expect(link).to match("utm_medium=email")
expect(link).to match("utm_campaign=govuk-brexit-checker")
expect(link).to match("http://www.gov.uk?")
Expand Down
10 changes: 5 additions & 5 deletions spec/integration/brexit_checker_spec.rb
Expand Up @@ -15,11 +15,11 @@
end
end

it "has change notes that reference valid actions" do
it "has notifications that reference valid actions" do
ids = BrexitChecker::Action.load_all.map(&:id)

BrexitChecker::ChangeNote.load_all.each do |change_note|
expect(ids).to include(change_note.action_id)
BrexitChecker::Notification.load_all.each do |notification|
expect(ids).to include(notification.action_id)
end
end

Expand All @@ -44,8 +44,8 @@
expect(keys.uniq.count).to eq(keys.count)
end

it "has change notes with unique IDs" do
keys = BrexitChecker::ChangeNote.load_all.map(&:id)
it "has notifications with unique IDs" do
keys = BrexitChecker::Notification.load_all.map(&:id)
expect(keys.uniq.count).to eq(keys.count)
end

Expand Down
18 changes: 9 additions & 9 deletions spec/lib/tasks/brexit_checker/change_notifications_spec.rb
Expand Up @@ -10,13 +10,13 @@
end

let(:addition) do
FactoryBot.build(:brexit_checker_change_note,
FactoryBot.build(:brexit_checker_notification,
type: "addition",
action_id: "addition")
end

let(:content_change) do
FactoryBot.build(:brexit_checker_change_note,
FactoryBot.build(:brexit_checker_notification,
type: "content_change",
note: "Something has changed",
action_id: "content_change")
Expand All @@ -25,7 +25,7 @@
before do
stub_email_alert_api_accepts_message
Rake::Task["brexit_checker:change_notification"].reenable
allow(BrexitChecker::ChangeNote).to receive(:load_all) { [addition, content_change] }
allow(BrexitChecker::Notification).to receive(:load_all) { [addition, content_change] }

allow(BrexitChecker::Action).to receive(:load_all) do
[
Expand Down Expand Up @@ -150,22 +150,22 @@
end
end

describe "when change note has criteria rules" do
describe "when a notification has criteria rules" do
let(:content_note_criteria_change) do
FactoryBot.build(:brexit_checker_change_note,
FactoryBot.build(:brexit_checker_notification,
type: "content_change",
note: "Something has changed",
action_id: "content_change",
criteria: [{ any_of: %w[forestry] }])
end

before do
allow(BrexitChecker::ChangeNote).to receive(:load_all) {
allow(BrexitChecker::Notification).to receive(:load_all) {
[content_note_criteria_change]
}
end

it "should notify subscribers based on the change note's criteria rules" do
it "should notify subscribers based on the notification's criteria rules" do
Rake::Task["brexit_checker:change_notification"].invoke(content_note_criteria_change.id)
assert_requested(:post, "#{endpoint}/messages") do |request|
payload = JSON.parse(request.body)
Expand All @@ -192,9 +192,9 @@
.to raise_error("Notification already sent")
end

it "raises an error if the change note cannot be found" do
it "raises an error if the notification cannot be found" do
expect { Rake::Task["brexit_checker:change_notification"].invoke("missing") }
.to raise_error("Change note not found")
.to raise_error("Notification not found")
end
end
end

0 comments on commit e1e194e

Please sign in to comment.