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

Feat: Grand calculated summary value sources #1215

Merged
merged 35 commits into from
Oct 12, 2023

Conversation

katie-gardner
Copy link
Contributor

@katie-gardner katie-gardner commented Sep 26, 2023

What is the context of this PR?

This PR adds support for using Grand Calculated Summaries as value sources. It goes alongside the corresponding validator PR which allows for this within a schema.

To do this, value_source_resolver has been updated and the existing method _resolve_calculated_summary_value_source has been used for grand calculated summaries too, as it worked directly out of the box without GCS needing its own. For clarity, some parameters have been renamed

placeholder transforms also needed to be updated so the custom logic for decimal places applies on a non grand calculated summary page that references a grand calculated summary value

Two schemas updated with a GCS inside and outside of a repeat to cover both cases, and functional tests updated accordingly

Note

As with validator, this builds on the PR for GCS in repeating sections, so the PR has been pointed at main for the test suite, and now pointed at that branch for review, it will be merged into main on approval

How to review

Use test_grand_calculated_summary_inside_repeating_section.json and test_grand_calculated_summary_repeating_answers.json to test and make sure the grand calculated summary values are piped in correctly. In particular on the repeating answers one, make sure that if you remove calculated summaries from the path using the options in section 2 that the GCS being used as a value source is correctly updated.

Checklist

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

@katie-gardner katie-gardner added dont merge Don't merge this PR New Feature labels Sep 26, 2023
@katie-gardner
Copy link
Contributor Author

@katie-gardner katie-gardner changed the base branch from main to grand-calculated-summary-in-repeating-section September 26, 2023 16:03
@katie-gardner katie-gardner marked this pull request as ready for review September 26, 2023 16:04
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.

I see that "dependencies" weren't handled, was that on purpose?. Right now, GCS values that aren't on the path would still be used. If you check for the reference to calculated_summary in the code base, you can see all the places we need to update to handle dependencies.

However, regardless, we should do it in a separate PR/card, to make the review easier. The card does mention only handling GCS values that are on the path, however we probably didn't I think it through and the sizing on the card should be higher even if tackled in a separate PR.

I think this PR to add the initial feature is good. We can decide whether we do it as part of the same card via multiple PR or whether we create a separate card for the dependency work.

app/questionnaire/value_source_resolver.py Show resolved Hide resolved
Base automatically changed from grand-calculated-summary-in-repeating-section to main October 4, 2023 16:42
@katie-gardner katie-gardner removed the dont merge Don't merge this PR label Oct 5, 2023
@katie-gardner
Copy link
Contributor Author

I see that "dependencies" weren't handled, was that on purpose?. Right now, GCS values that aren't on the path would still be used. If you check for the reference to calculated_summary in the code base, you can see all the places we need to update to handle dependencies.

However, regardless, we should do it in a separate PR/card, to make the review easier. The card does mention only handling GCS values that are on the path, however we probably didn't I think it through and the sizing on the card should be higher even if tackled in a separate PR.

I think this PR to add the initial feature is good. We can decide whether we do it as part of the same card via multiple PR or whether we create a separate card for the dependency work.

Ah no, not intentional, I had read it as grand calculated summaries should only include CS on the path just as CS only includes answers on the path, which it already does, missed the other case, follow up for this sounds good 👍

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.

Happy with the initial build.

Note for other reviewers: Dependencies have not been implemented in this PR, so you may get an answer not on the path, and validation won't get revisted etc.

scripts/run_validator.sh Outdated Show resolved Hide resolved
@MebinAbraham MebinAbraham added the dont merge Don't merge this PR label Oct 10, 2023
@katie-gardner katie-gardner removed the dont merge Don't merge this PR label Oct 12, 2023
@katie-gardner katie-gardner merged commit 4b4e76f into main Oct 12, 2023
15 checks passed
@katie-gardner katie-gardner deleted the grand-calculated-summary-value-source branch October 12, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants