Skip to content

NPA-6546: fix end to end tests#123

Open
ehallam wants to merge 19 commits intomainfrom
task/NPA-6546/fix-end-to-end-tests
Open

NPA-6546: fix end to end tests#123
ehallam wants to merge 19 commits intomainfrom
task/NPA-6546/fix-end-to-end-tests

Conversation

@ehallam
Copy link
Copy Markdown

@ehallam ehallam commented Apr 16, 2026

Pull Request

🧾 Ticket Link

https://nhsd-jira.digital.nhs.uk/browse/NPA-6546


📄 Description/Summary of Changes

  • We have two classes, Demographics, and specific Patient models for TPP and EMIS that inherit from Demographics

  • Since we were using Demographics in the forwards_response.py file, pydantic was dropping all the extra fields we have on the specific TPP and EMIS Patient classes because it was using the types declared in forwards_response.py not the runtime types

  • Using SerializeAsAny makes Pydantic serialise the actual runtime types and not drop the extra fields

  • There are also new additions to enable the new proxygen deployments to be fixed

  • These include adding the new keys to GitHub secrets and adding the PTL_ and PROD_ variables to the GitHub actions

  • We also now have split out Prod and PTL actions/config files


🧪 Developer Testing Carried Out


📋 PR Principles

  • Keep PRs Small and Focused: Ensure the PR addresses a single task or feature to make it easier to review.
  • Multiple PRs for one Ticket: When splitting work into multiple PRs, clearly describe what this PR addresses and outline the remaining work to complete the ticket.
  • Ensure Tests Are Included: Add or update unit, integration, or end-to-end tests to cover the changes made.
  • Follow Coding Standards: Ensure the code adheres to the team's coding guidelines and best practices.
  • Resolve Comments Promptly: If you raise a comment, ensure you follow up and resolve it before approving the PR to maintain clarity and ensure comments are addressed.
  • Foster Learning: PR reviews are an opportunity to share knowledge, provide constructive feedback, and encourage a collaborative environment.

🏷️ Naming Conventions Reminder

Please ensure the following naming conventions are followed:

  • PR title follows the format: NPA-XXXX: <short-description>
  • Branch name follows the convention: <type>/NPA-XXXX/<short-description>
  • Commit messages follow the template: NPA-XXXX: <short-description>

Copilot AI review requested due to automatic review settings April 16, 2026 12:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to fix end-to-end test failures caused by Pydantic serializing ForwardResponse.user/patients using the declared base type (Demographics), which drops subtype-specific fields for EMIS/TPP patient models.

Changes:

  • Update ForwardResponse to use SerializeAsAny so runtime subtype fields are preserved during serialization.
  • Rename the EMIS-specific Patient model to Person and update EMIS client/tests accordingly.
  • Update Proxygen credential placeholders and GitHub Actions workflow/action inputs (currently inconsistent and likely to break CI).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
app/api/domain/forward_response_model.py Uses SerializeAsAny on user/patients to preserve subtype serialization.
app/api/infrastructure/emis/models.py Renames EMIS Patient model to Person; updates SessionResponse typing.
app/api/infrastructure/emis/client.py Updates EMIS client to construct Person and fixes docstring typing/spelling.
app/api/infrastructure/emis/tests/test_client.py Updates EMIS tests to use Person.
app/api/infrastructure/tpp/client.py Fixes docstring spelling (“structual” → “structural”).
.github/workflows/pull-request-checks.yml Switches Proxygen secret names to NEW_PROXYGEN_* (currently mismatched with reusable workflow/action).
.github/actions/setup-proxygen/action.yaml Updates sed/echo steps to inputs.NEW_PROXYGEN_* (but inputs are still declared as PROXYGEN_*).
proxygen/credentials.yaml Updates placeholder strings used for sed substitution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/actions/setup-proxygen/action.yaml Outdated
Comment thread .github/workflows/pull-request-checks.yml Outdated
Comment thread .github/workflows/pull-request-checks.yml Outdated
Comment thread app/api/domain/forward_response_model.py
@ehallam ehallam temporarily deployed to internal-dev-sandbox April 23, 2026 10:06 — with GitHub Actions Inactive
@ehallam ehallam temporarily deployed to internal-dev-sandbox April 24, 2026 08:38 — with GitHub Actions Inactive
Comment thread Makefile
@ehallam ehallam temporarily deployed to internal-dev-sandbox April 27, 2026 09:46 — with GitHub Actions Inactive
@ehallam ehallam temporarily deployed to internal-dev-sandbox April 27, 2026 09:52 — with GitHub Actions Inactive
Copy link
Copy Markdown
Contributor

@davesmallnhs davesmallnhs left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants