Skip to content

Conversation

@steventux
Copy link
Contributor

@steventux steventux commented Nov 18, 2025

Description

We currently send an email per generated report, we don't currently need this granularity.
This PR:

  • Refactors smoke test config
  • Refactors some of the report config
  • Amends how we send reports to sending all in one email

Jira link

https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11604

Review notes

Review checklist

  • Check database queries are correctly scoped to current_provider

@steventux steventux requested a review from a team as a code owner November 18, 2025 08:47
logger.info("Report %s created", report_config.report_filename)
logger.info("Report %s created", report_config.name)

if not options.get("smoke_test", False):
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe my not getting it, but should we keep the logic behind sending an email to one thing, e.g the report_config.send_email boolean? otherwise we check two different things for whether to send an email?
Or can we get rid of report_config.send_email and only check whether it's a smoke test or not?

Copy link
Contributor Author

@steventux steventux Nov 18, 2025

Choose a reason for hiding this comment

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

I think you're right, this check isn't necessary anymore. I'll make sure the tests cover the smoke test config adequately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Harriethw thanks for catching that 0a41866

@steventux steventux merged commit 9af62b7 into main Nov 19, 2025
12 checks passed
@steventux steventux deleted the send-notification-reports-in-one-email branch November 19, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants