Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DWPF-704 update dw usage of feedback package and implement custom notifications #433

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7d3ae76
updates django-feedback-govuk to latest
nicopicchio Aug 22, 2023
d95cf6c
Merge branch 'main' into DWPF-704-update-dw-usage-of-feedback-package…
nicopicchio Aug 29, 2023
7aa0c24
creates method to retrieve feedback from the last 24hrs
nicopicchio Aug 29, 2023
491d9bd
linter changes
nicopicchio Aug 29, 2023
3ce3502
updates django-feedback-govuk version in requirements.txt
nicopicchio Aug 29, 2023
bc42c78
defines method to send email notifications, adds env variables and task
nicopicchio Aug 29, 2023
ff40ae1
linter fix
nicopicchio Aug 29, 2023
39afe4f
schedules feedback email notification task to be sent daily at 06:00
nicopicchio Aug 30, 2023
939dfcb
makes changes based on Cam's suggestions
nicopicchio Aug 30, 2023
fc95bf4
Merge branch 'main' into DWPF-704-update-dw-usage-of-feedback-package…
nicopicchio Sep 4, 2023
911d93e
adds tests to check that feedback_received_within() works as expected
nicopicchio Sep 4, 2023
14765d3
renames method in test file
nicopicchio Sep 4, 2023
0d65dea
linter fix
nicopicchio Sep 4, 2023
c8f2c68
updates WAGTAIL_BASE_URL in .env.ci and .env.example files
nicopicchio Sep 4, 2023
48369ab
Merge branch 'main' into DWPF-704-update-dw-usage-of-feedback-package…
nicopicchio Sep 4, 2023
32c42c2
adds code and tests to check for settings and email recipients
nicopicchio Sep 6, 2023
6f8b609
Merge remote-tracking branch 'origin/DWPF-704-update-dw-usage-of-feed…
nicopicchio Sep 6, 2023
bb9e938
shuffles code and moves comments inside test methods
nicopicchio Sep 6, 2023
929b946
send_feedback_notification refactor and adds more tests
nicopicchio Sep 6, 2023
1805f09
adds test to check that send_email_notification is called once per email
nicopicchio Sep 6, 2023
32d7388
renames calls to expected_calls
nicopicchio Sep 6, 2023
9c21fa3
Merge branch 'main' into DWPF-704-update-dw-usage-of-feedback-package…
nicopicchio Sep 6, 2023
4e14c87
modifies tests for conciseness
nicopicchio Sep 7, 2023
44c675c
linter fix
nicopicchio Sep 7, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .env.ci
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ ALLOWED_HOSTS=*
AUTHBROKER_URL=https://test.gov.uk
AUTHBROKER_CLIENT_ID=test
AUTHBROKER_CLIENT_SECRET=test
WAGTAIL_BASE_URL=http://localhost:8000
WAGTAIL_BASE_URL=http://localhost:8000/
AWS_STORAGE_BUCKET_NAME=test
AWS_REGION=eu-west-2
AWS_ACCESS_KEY_ID=test
Expand Down
2 changes: 1 addition & 1 deletion .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ DJANGO_DEBUG=true
DJANGO_SECRET_KEY=this_is_an_example_use_a_proper_key_in_production

# Wagtail
WAGTAIL_BASE_URL=http://localhost:8000
WAGTAIL_BASE_URL=http://localhost:8000/

# Container connections
DATABASE_URL=psql://postgres:postgres@db:5432/digital_workspace
Expand Down
38 changes: 33 additions & 5 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ django-extensions = "^3.2.1"
werkzeug = "^2.2.3"
blessings = "^1.7"
pytest-mock = "^3.11.1"
pytest-freezer = "^0.4.8"

[build-system]
requires = ["poetry-core"]
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ django-cogwheels==0.3 ; python_version >= "3.11" and python_version < "4.0"
django-crispy-forms==1.14.0 ; python_version >= "3.11" and python_version < "4.0"
django-elasticsearch-dsl==7.3 ; python_version >= "3.11" and python_version < "4.0"
django-environ==0.10.0 ; python_version >= "3.11" and python_version < "4"
django-feedback-govuk==0.2.7 ; python_version >= "3.11" and python_version < "4.0"
django-feedback-govuk==0.2.8 ; python_version >= "3.11" and python_version < "4.0"
django-filter==22.1 ; python_version >= "3.11" and python_version < "4.0"
django-hawk-drf==1.1.1 ; python_version >= "3.11" and python_version < "4.0"
django-hawk==1.2.1 ; python_version >= "3.11" and python_version < "4.0"
Expand Down
5 changes: 5 additions & 0 deletions src/config/celery.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,9 @@
"task": "core.tasks.ingest_uk_staff_locations",
"schedule": crontab(minute="0", hour="3"),
},
# Daily feedback email task
"schedule-feedback-email-notification": {
"task": "core.tasks.schedule_feedback_email_notification",
"schedule": crontab(hour=6, minute=0),
},
}
12 changes: 6 additions & 6 deletions src/config/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -596,15 +596,15 @@
CRISPY_ALLOWED_TEMPLATE_PACKS = ["gds"]
CRISPY_TEMPLATE_PACK = "gds"

# Feedback notifications email
FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID = env("FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID")
FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS = env.list(
"FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS", default=[]
)

# Feedback
DJANGO_FEEDBACK_GOVUK = {
"SERVICE_NAME": "the beta experience",
"FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID": env(
"FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID"
),
"FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS": env.list(
"FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS", default=[]
),
"COPY": {
"SUBMIT_TITLE": "Providing feedback on your experience will help us improve the service",
"FIELD_SATISFACTION_LEGEND": "How satisfied are you with Digital Workspace?",
Expand Down
4 changes: 1 addition & 3 deletions src/config/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,7 @@
path("peoplefinder/api/", include(api_urlpatterns)),
path("sitemap.xml", sitemap),
# Feedback
path(
"feedback/", include(feedback_urls), name="feedback"
), # @TODO [DWPF-454] remove feedback after beta
path("feedback/", include(feedback_urls), name="feedback"),
# Wagtail
path("", include(wagtail_urls)),
]
Expand Down
10 changes: 10 additions & 0 deletions src/core/tasks.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from config.celery import celery_app
from peoplefinder.services.uk_staff_locations import UkStaffLocationService
from feedback import utils


@celery_app.task(bind=True)
Expand All @@ -21,3 +22,12 @@ def ingest_uk_staff_locations(self):
f"Updated: {updated}\n"
f"Deleted: {deleted}\n"
)


@celery_app.task(bind=True)
def schedule_feedback_email_notification():
feedback_received = utils.feedback_received_within()
if not feedback_received:
return
message = utils.send_feedback_notification()
print(f"Message: {message}")
107 changes: 107 additions & 0 deletions src/feedback/test/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import pytest
from unittest.mock import call
from django.test import override_settings
from django_feedback_govuk.models import BaseFeedback

from feedback.utils import feedback_received_within, send_feedback_notification


def test_no_feedback_submitted_and_feedback_submitted_within_24hrs(db):
# feedback_received_within() returns False if no feedback was submitted and True if feedback was submitted within the last 24hrs.
assert not feedback_received_within()
nicopicchio marked this conversation as resolved.
Show resolved Hide resolved
BaseFeedback.objects.create()
assert feedback_received_within()


def test_feedback_submitted_over_24hrs_ago(db, freezer):
# feedback_received_within() returns False if feedback was submitted over 24hrs ago
freezer.move_to("2023-09-01")
BaseFeedback.objects.create()
assert feedback_received_within()

nicopicchio marked this conversation as resolved.
Show resolved Hide resolved
freezer.move_to("2023-09-03")
assert not feedback_received_within()


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the following ones are all fine, but very repetitive - if you want you could consider parameterising them to make it a bit more concise and (IMO) easier to see what's happening between the tests. There's examples of this in DW somewhere if you search around.

@override_settings(
GOVUK_NOTIFY_API_KEY=None,
FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID=None,
WAGTAILADMIN_BASE_URL=None,
)
def test_send_feedback_notification_when_no_settings_are_set():
# send_feedback_notification() raises an error when called with no settings provided
with pytest.raises(ValueError) as raised_execption:
send_feedback_notification()
assert (
raised_execption.value.args[0]
== "Missing required settings for sending feedback notifications"
)


@override_settings(
GOVUK_NOTIFY_API_KEY="test-api-key",
FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS=[],
FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID="test-template-id",
WAGTAILADMIN_BASE_URL="https://test.example.com/",
)
def test_send_feedback_notification_with_no_email_recipients():
# send_feedback_notification() raises an error when the email_recipients list is empty
with pytest.raises(ValueError) as raised_exception:
send_feedback_notification()
assert (
raised_exception.value.args[0]
== "Missing required settings for sending feedback notifications"
)


@override_settings(
GOVUK_NOTIFY_API_KEY="this-is-my-really-long-api-key-because-gov-uk-notify-expects-it-to-be-long-when-you-create-a-service",
FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS=["[email protected]"],
FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID="test-template-id",
WAGTAILADMIN_BASE_URL="https://test.example.com/",
)
def test_send_feedback_notification_with_valid_settings(mocker):
# send_feedback_notification() does not raise an error when all the settings provided are valid
mock_send_email_notification = mocker.patch(
"feedback.utils.NotificationsAPIClient.send_email_notification"
)
send_feedback_notification()
mock_send_email_notification.assert_called_once_with(
email_address="[email protected]",
template_id="test-template-id",
personalisation={
"feedback_url": "https://test.example.com/feedback/submitted/"
},
)


@override_settings(
GOVUK_NOTIFY_API_KEY="this-is-my-really-long-api-key-because-gov-uk-notify-expects-it-to-be-long-when-you-create-a-service",
FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS=["[email protected]", "[email protected]"],
FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID="test-template-id",
WAGTAILADMIN_BASE_URL="https://test.example.com/",
)
def test_send_feedback_notification_with_multiple_emails(mocker):
# send_email_notification() is called once per email address
mock_send_email_notification = mocker.patch(
"feedback.utils.NotificationsAPIClient.send_email_notification"
)
expected_calls = [
call(
email_address="[email protected]",
template_id="test-template-id",
personalisation={
"feedback_url": "https://test.example.com/feedback/submitted/"
},
),
call(
email_address="[email protected]",
template_id="test-template-id",
personalisation={
"feedback_url": "https://test.example.com/feedback/submitted/"
},
),
]
send_feedback_notification()
assert mock_send_email_notification.call_count == len(expected_calls)
mock_send_email_notification.assert_has_calls(expected_calls)
44 changes: 44 additions & 0 deletions src/feedback/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
from datetime import timedelta

from django.conf import settings
from django.urls import reverse
from django.utils import timezone
from django_feedback_govuk.models import BaseFeedback
from notifications_python_client.notifications import NotificationsAPIClient


def feedback_received_within(days=1):
return BaseFeedback.objects.filter(
submitted_at__gte=timezone.now() - timedelta(days=days)
).exists()


def send_feedback_notification():
email_recipients = settings.FEEDBACK_NOTIFICATION_EMAIL_RECIPIENTS
govuk_notify_template_id = settings.FEEDBACK_NOTIFICATION_EMAIL_TEMPLATE_ID
base_url = settings.WAGTAILADMIN_BASE_URL
notify_api_key = settings.GOVUK_NOTIFY_API_KEY

if not all(
[
notify_api_key,
govuk_notify_template_id,
base_url,
email_recipients,
]
):
raise ValueError("Missing required settings for sending feedback notifications")

if base_url[-1] == "/":
base_url = base_url[:-1]

notification_client = NotificationsAPIClient(
settings.GOVUK_NOTIFY_API_KEY,
)

for email_recipient in email_recipients:
notification_client.send_email_notification(
email_address=email_recipient,
template_id=govuk_notify_template_id,
personalisation={"feedback_url": base_url + reverse("submitted-feedback")},
)