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

Integrate with SDS endpoint to support loading Prepop data #1114

Merged
merged 26 commits into from
Jun 9, 2023

Conversation

berroar
Copy link
Contributor

@berroar berroar commented May 25, 2023

What is the context of this PR?

Adds the ability to integrate with the future SDS endpoint in order to be able to load Pre-populated Questionnaire data into runner.

Adds validation of the Prepop data returned by the SDS endpoint, based on the definitions outlined here: ONSdigital/sds-schema-definitions#1

As this PR only covers the initial integration, the response payload from the SDS endpoint is being validated and returned but not being used.

How to review

Set up a mock SDS endpoint using the new dev script added: python -m scripts.mock_sds_endpoint

Ensure the new env var SDS_API_BASE_URL is set with http://localhost:5003/v1/unit_data.

Use the add-sds-dataset-id branch of launcher which allows you to launch with the sds_dataset_id field that is required in order to request prepop data, and attempt to open the newly added test_supplementary_data survey.

Using sds_data_set_id: 34a80231-c49a-44d0-91a6-8fe1fb190e64 will return mocked supplementary data with a repeat and c067f6de-6d64-42b1-8b02-431a3486c178 will return mocked supplementary data without a repeat.

Check the tests added.

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

@berroar
Copy link
Contributor Author

berroar commented May 25, 2023

@berroar berroar force-pushed the integrate-with-sds branch from a8c2627 to 566a526 Compare May 25, 2023 15:50
@berroar berroar marked this pull request as ready for review May 25, 2023 16:00
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Still need to test functionally but some initial comments/questions

app/routes/session.py Outdated Show resolved Hide resolved
app/routes/session.py Outdated Show resolved Hide resolved
app/routes/session.py Outdated Show resolved Hide resolved
app/utilities/supplementary_data_parser.py Show resolved Hide resolved
tests/app/conftest.py Show resolved Hide resolved
scripts/mock_sds_endpoint.py Outdated Show resolved Hide resolved
app/routes/session.py Outdated Show resolved Hide resolved
app/routes/session.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

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

A PR should be raised to merge main into the base branch.

app/routes/session.py Outdated Show resolved Hide resolved
app/routes/session.py Outdated Show resolved Hide resolved
app/routes/session.py Outdated Show resolved Hide resolved
app/routes/session.py Outdated Show resolved Hide resolved
app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved
app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Have now tested functionally and working as I would expect except for survey_id validation

scripts/mock_sds_endpoint.py Outdated Show resolved Hide resolved
scripts/mock_sds_endpoint.py Outdated Show resolved Hide resolved
app/supplementary_data.py Outdated Show resolved Hide resolved
app/supplementary_data.py Outdated Show resolved Hide resolved
app/supplementary_data.py Outdated Show resolved Hide resolved
app/supplementary_data.py Outdated Show resolved Hide resolved
app/supplementary_data.py Outdated Show resolved Hide resolved
app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved
app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved
app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved
tests/app/parser/test_supplementary_data_parser.py Outdated Show resolved Hide resolved
tests/app/test_request_supplementary_data.py Outdated Show resolved Hide resolved
app/routes/session.py Show resolved Hide resolved
app/utilities/supplementary_data_parser.py Outdated Show resolved Hide resolved
tests/integration/create_token.py Outdated Show resolved Hide resolved
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Generally changes looking good 👍
I found it quite hard to functionally test as I missed a few things - now we need to update our .env file with the SDS url and if running the mock sds endpoint script in debug mode, remembering to set the working directory to runner so it can find the files
My bad really but might be useful tips to add to run instructions?

Also are the run instructions anywhere else? Thinking a readme in the doc folder or somewhere else might be good to preserve whats on the PR so people have a reference once its merged

Ah - I think the .development.env and functional tests equivalent need updating for the new env var

@kylelawsonAND
Copy link
Contributor

now we need to update our .env file with the SDS url and if running the mock sds endpoint script in debug mode, remembering to set the working directory to runner so it can find the files My bad really but might be useful tips to add to run instructions?

Also are the run instructions anywhere else? Thinking a readme in the doc folder or somewhere else might be good to preserve whats on the PR so peo

Agree would be good to update the .env files

@berroar
Copy link
Contributor Author

berroar commented Jun 5, 2023

Generally changes looking good 👍 I found it quite hard to functionally test as I missed a few things - now we need to update our .env file with the SDS url and if running the mock sds endpoint script in debug mode, remembering to set the working directory to runner so it can find the files My bad really but might be useful tips to add to run instructions?

Also are the run instructions anywhere else? Thinking a readme in the doc folder or somewhere else might be good to preserve whats on the PR so people have a reference once its merged

Ah - I think the .development.env and functional tests equivalent need updating for the new env var

Updated now 👍

Copy link
Contributor

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Will functionally test once i fix my runner 👍

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Two minor non-blocking comments but happy with readme and how its all working, and test updates, nice one

Copy link
Contributor

@kylelawsonAND kylelawsonAND left a comment

Choose a reason for hiding this comment

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

Looks great now - nice one 👌

doc/run-mock-sds-endpoint.md Outdated Show resolved Hide resolved
@mgdon650
Copy link
Contributor

mgdon650 commented Jun 9, 2023

Changes look good to me. Runs OK!

@berroar berroar merged commit 3f5ac90 into feature-prepop Jun 9, 2023
@berroar berroar deleted the integrate-with-sds branch June 9, 2023 13:16
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.

8 participants