Skip to content

Conversation

@steventux
Copy link
Contributor

@steventux steventux commented Nov 17, 2025

Description

Refactors the reconciliation report to give detail on all appointments for the given period. Notifications timestamps and some additional status information is included.

image

(This image does not contain real data)

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 force-pushed the merge-aggregate-and-reconciliation-reports branch from 68aebcc to a886e81 Compare November 18, 2025 14:40
@steventux steventux marked this pull request as ready for review November 18, 2025 15:31
@steventux steventux requested a review from a team as a code owner November 18, 2025 15:31
GROUP BY clin.code, appt.episode_type, appt.status
SELECT appt.nhs_number::text AS "NHS number",
clin.name || ' (' || clin.code || ')' AS "Clinic name and code",
CASE
Copy link
Contributor

Choose a reason for hiding this comment

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

amazing SQL-fu here 🧙

df(now, 7),
df(now, 7),
df(now, 7),
None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm starting to get a bit overwhelmed by the range of data/columns and it's hard to parse without seeing the actual file outputted.... and I've actually never seen a report with "real" data in it 😬
do you think an integration test would be useful? if only to provide an easy way to populate data and run locally 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, this didn't feel great when I was refactoring. I think I can make the test a bit clearer than this. Probably being a bit lazy in how I am comparing the results to the expectations, will amend.

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 I've tried to make the test assertions clearer and reduced the test data to the smallest possible selection of data.

Copy link
Contributor Author

@steventux steventux Nov 21, 2025

Choose a reason for hiding this comment

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

Regarding realistic end reports, I wonder if there is something we can do when we come to load testing around this, we will be creating a large data import for load testing which could help us generate realistic reports.

@Harriethw
Copy link
Contributor

This looks good! I think it just needs rebasing against main to get rid of the first three commits?

@steventux steventux force-pushed the merge-aggregate-and-reconciliation-reports branch 2 times, most recently from 4722d0c to 460cdda Compare November 20, 2025 09:23
@steventux steventux force-pushed the merge-aggregate-and-reconciliation-reports branch from 460cdda to 412a8e7 Compare November 21, 2025 18:04
@steventux steventux merged commit ee8823e into main Nov 24, 2025
12 checks passed
@steventux steventux deleted the merge-aggregate-and-reconciliation-reports branch November 24, 2025 10:40
@steventux steventux mentioned this pull request Nov 24, 2025
1 task
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