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

fix: Failing regression test #2918

Merged
merged 5 commits into from
Mar 25, 2024
Merged

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Mar 25, 2024

Regression tests passing locally ✅

Running against this branch in CI here - https://github.com/theopensystemslab/planx-new/actions/runs/8419529120

Problem

The regression test for "send to BOPS" was failing for a number of reasons - please see comments and changes on PR.

The main / original issue was that the mock flow was outdated - meaning that payload generation was failing.

Solution

  • Update mock flow data with snapshot from this morning
  • Longer term, it would be nice to avoid this and keep mocks in a single place - I think it should be possible to have a bunch of mock flows in planx-core (and a script to pull latest version?). We could then expose these via imports into planx-new and just need to update a single flow once as opposed to keeping multiple copies. Something to discuss next dev call!

Copy link

github-actions bot commented Mar 25, 2024

Removed vultr server and associated DNS entries

@@ -10,6 +10,7 @@ export function createTeam(
slug: "E2E",
submissionEmail: TEST_EMAIL,
homepage: "planx.uk",
referenceCode: "ABCD",
Copy link
Contributor Author

@DafyddLlyr DafyddLlyr Mar 25, 2024

Choose a reason for hiding this comment

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

This is a mandatory field to generate a valid payload.

planx-core now makes this required with this change - theopensystemslab/planx-core#337

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy/pasted the most recent version of the flow here to resolve schema issues.

I needed to manually update the Pay component with the govPayMetadata field - I'll open a PR shortly to either properly handle defaults or to make a data migration.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused by this - is it actually an issue of outdated flow mocks or the mock session data referenced by the graphql query mocks (or both)? Eg I'm surprised to not see these updates repeated here https://github.com/theopensystemslab/planx-new/pull/2872/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only errors were coming from outdated flow mocks, which is what I've updated here. I was also surprised by this, but I guess the variables in question are optional?

DafyddLlyr added a commit to theopensystemslab/planx-core that referenced this pull request Mar 25, 2024
This field is actually required in order to generate a valid
DigitalPlanning payload. Please see
theopensystemslab/planx-new#2918 for context.
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this one - one comment /question re: mock flows versus sessions here - but if CI is satisfied then happy for this fix to merge whenever!

Copy link

github-actions bot commented Mar 25, 2024

🤖 Hasura Change Summary compared a subset of table metadata including permissions:

Updated Tables (1)

  • public.payment_requests permissions:

    insert select update delete
    api / / /
    3 added column permissions
    insert select update
    api ➕ govpay_metadata ➕ govpay_metadata ➕ govpay_metadata

@DafyddLlyr DafyddLlyr force-pushed the dp/fix-regression-test-itp-bops branch from 48a2646 to cc84887 Compare March 25, 2024 11:45
@DafyddLlyr DafyddLlyr marked this pull request as ready for review March 25, 2024 12:24
@DafyddLlyr DafyddLlyr merged commit c2b9069 into main Mar 25, 2024
17 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/fix-regression-test-itp-bops branch March 25, 2024 12:24
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