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

Implement "progress" value source #1044

Merged
merged 105 commits into from
May 26, 2023
Merged

Conversation

ONS-Guilhem-Forey
Copy link
Contributor

@ONS-Guilhem-Forey ONS-Guilhem-Forey commented Feb 22, 2023

Since the progress value source is being added to the validator here, this PR implements it.

  • Added logic to the value_resolver
  • Added logic to capture new dependencies, such as sections being tracked for their progress
  • Added schemas to test progress value sources with:
    • blocks within a section
    • Sections, with section enabled
    • Sections, in both linear and hub flows
    • Mix of both
    • Rule in skip_conditions as well as routing_rule
    • Rule inside repeating sections

How to review functionally

Inspect the new schemas, they start with `test_progress_value_source*"
Test them from the launcher, look at the integration tests and ensure behaviour is expected.
In particular:

  • test_progress_value_source_calculated_summary.json
  • test_progress_value_source_repeating_sections.json
  • test_progress_value_source_blocks_chained.json
  • test_progress_value_source_blocks.json

@ONS-Guilhem-Forey ONS-Guilhem-Forey force-pushed the feat/questionnaire-progress-route branch 3 times, most recently from b496e9e to ba0f564 Compare February 24, 2023 09:31
@ONS-Guilhem-Forey ONS-Guilhem-Forey changed the title progress Add "progress" value source Feb 24, 2023
@ONS-Guilhem-Forey ONS-Guilhem-Forey changed the title Add "progress" value source Implement "progress" value source Feb 24, 2023
@ONS-Guilhem-Forey
Copy link
Contributor Author

@ONS-Guilhem-Forey ONS-Guilhem-Forey force-pushed the feat/questionnaire-progress-route branch 2 times, most recently from 73b466c to f299b2b Compare February 24, 2023 13:07
@ONS-Guilhem-Forey ONS-Guilhem-Forey marked this pull request as ready for review February 24, 2023 13:10
@ONS-Guilhem-Forey ONS-Guilhem-Forey force-pushed the feat/questionnaire-progress-route branch from 720180c to 392872b Compare February 24, 2023 13:16
@MebinAbraham MebinAbraham self-requested a review March 1, 2023 15:50
@ONS-Guilhem-Forey ONS-Guilhem-Forey force-pushed the feat/questionnaire-progress-route branch from 1ce7d3f to 370dd16 Compare March 3, 2023 12:54
berroar
berroar previously requested changes Mar 9, 2023
app/questionnaire/questionnaire_schema.py Outdated Show resolved Hide resolved
app/data_models/progress_store.py Outdated Show resolved Hide resolved
app/questionnaire/questionnaire_schema.py Outdated Show resolved Hide resolved
app/questionnaire/questionnaire_store_updater.py Outdated Show resolved Hide resolved
app/questionnaire/questionnaire_schema.py Outdated Show resolved Hide resolved
app/questionnaire/questionnaire_schema.py Outdated Show resolved Hide resolved
app/questionnaire/questionnaire_store_updater.py Outdated Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved
app/questionnaire/value_source_resolver.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.

Almost there, looking good.

Can you link me to tests that cover:

  • progress condition based on a block inside a repeating section while inside a repeat
  • progress condition based on a block outside a repeating section while inside a repeat

app/questionnaire/questionnaire_store_updater.py Outdated Show resolved Hide resolved
@ONS-Guilhem-Forey ONS-Guilhem-Forey force-pushed the feat/questionnaire-progress-route branch from 66b2aca to a71fb1c Compare March 16, 2023 09:34
@ONS-Guilhem-Forey
Copy link
Contributor Author

Almost there, looking good.

Can you link me to tests that cover:

  • progress condition based on a block inside a repeating section while inside a repeat
  • progress condition based on a block outside a repeating section while inside a repeat
  1. We can't refer to blocks in repeating sections?
  2. This schema, these tests

@ONS-Guilhem-Forey
Copy link
Contributor Author

Was also added this week the compatibility with calculated summary blocks.

@MebinAbraham
Copy link
Contributor

MebinAbraham commented Mar 22, 2023

  1. We can't refer to blocks in repeating sections?

You can if you are inside the repeat itself. You can't refer to a repeating section from outside the current repeat.

@ONS-Guilhem-Forey ONS-Guilhem-Forey force-pushed the feat/questionnaire-progress-route branch from 2b914a5 to 9f7b3a2 Compare March 28, 2023 13:10
app/data_models/progress_store.py Outdated Show resolved Hide resolved
app/questionnaire/path_finder.py Outdated Show resolved Hide resolved
app/questionnaire/questionnaire_schema.py Outdated Show resolved Hide resolved
app/questionnaire/questionnaire_store_updater.py Outdated Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved
app/views/handlers/question.py 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.

Code looks good, just need to finish functional tests.

Pipfile Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved
app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved
@kylelawsonAND
Copy link
Contributor

Screenshot 2023-05-22 at 15 33 27 Functionally tested and all seems good + can't find anything else in the codebase to point out. But getting 3 failures on TestQuestionnaireLanguage. I think maybe I'm missing doing something on my end?

@berroar
Copy link
Contributor

berroar commented May 23, 2023

Screenshot 2023-05-22 at 15 33 27 Functionally tested and all seems good + can't find anything else in the codebase to point out. But getting 3 failures on TestQuestionnaireLanguage. I think maybe I'm missing doing something on my end?

Are your dependencies up to date with this branch? Should be fine if you've done pipenv install --dev

@kylelawsonAND
Copy link
Contributor

Screenshot 2023-05-22 at 15 33 27 Functionally tested and all seems good + can't find anything else in the codebase to point out. But getting 3 failures on TestQuestionnaireLanguage. I think maybe I'm missing doing something on my end?

Are your dependencies up to date with this branch? Should be fine if you've done pipenv install --dev

Yep tried that, no joy 🤔

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.

Apart from the reopened comment from Kyle and one other just on the schema descriptions, have functionally tested and cannot break it - all working as I'd expect and will approve on resolution of the descriptions 👍

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.

👍 🎉

As discussed, answer dependencies don't support chained dependencies, that is something we need to look at.

@MebinAbraham MebinAbraham added the dont merge Don't merge this PR label May 24, 2023
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.

All gooooood

@MebinAbraham MebinAbraham removed the dont merge Don't merge this PR label May 26, 2023
@berroar berroar merged commit cc12488 into main May 26, 2023
@berroar berroar deleted the feat/questionnaire-progress-route branch May 26, 2023 13:16
katie-gardner added a commit that referenced this pull request Jun 5, 2023
* Make response expiry date mandatory (#1104)

* Bind additional contexts to flush requests (#1108)

* Schemas v3.56.0 (#1110)

* Schemas v3.57.0 (#1113)

* Schemas v3.57.1 (#1115)

* Schemas v3.58.0 (#1116)

* Implement "progress" value source (#1044)

Co-authored-by: Rhys Berrow <[email protected]

* Fix handling of invalid values in form numerical inputs  (#1111)

* Update Chromedriver to version 113 (#1118)

* update chromedriver

* temporarily comment out line, to be fixed separately

* Feat/Grand calculated summary (#1107)

* initial ideas

* Implementation closer to calculated summaries

* better test schemas and routing evaluation

* get routing working

* dont need section id

* make routing better, start adding tests

* start fixing test schemas

* Two further schema fixes

* Fix remaining schema issue

* Add integration test

* Add functional test

* Functional tests for routing and answers not on the path

* Changing an answer updates GCS progress

* Add schema for overlapping answers

* Functionally test skip-question in grand calculated summary

* Add tests for GCS routing

* Fix errors caused by merge

* type hints, comments, remove some duplication

* fix formatting

* Update translation templates

* remove accidental update

* Fix some type hints

* Use validator branch and update test description

* refactor GCS answer format

* Fix lint and test errors

* Address PR comments

* improve context and coverage of test schemas

* Fix invalid schema and add invalid routing test

* Fix line length in test

* test schema linting

* Fix routing bug

* Make schema easier to follow

* Linting

* partial fix to routing

* Add addtional routing test case

* Remove arg from new test case

* Routing to next incomplete block for summaries

* linting error

* remove arg from router tests

* PR comments

* PR Comment

* Amend comment and improve dependencies function

* Handle merge errors

* fix gcs routing within same section

* Add test for GCS with progress value source

* Remove backtick

* update chromedriver

* Revert "update chromedriver"

This reverts commit a2e3698.

* Fix schema labels

* Revert validator branch to latest

---------

Co-authored-by: petechd <[email protected]>
Co-authored-by: Mebin Abraham <[email protected]>
Co-authored-by: Guilhem <[email protected]>
kylelawsonAND added a commit that referenced this pull request Jun 16, 2023
* Make response expiry date mandatory (#1104)

* Bind additional contexts to flush requests (#1108)

* Schemas v3.56.0 (#1110)

* Schemas v3.57.0 (#1113)

* Schemas v3.57.1 (#1115)

* Schemas v3.58.0 (#1116)

* Implement "progress" value source (#1044)

Co-authored-by: Rhys Berrow <[email protected]

* Fix handling of invalid values in form numerical inputs  (#1111)

* Update Chromedriver to version 113 (#1118)

* update chromedriver

* temporarily comment out line, to be fixed separately

* Feat/Grand calculated summary (#1107)

* initial ideas

* Implementation closer to calculated summaries

* better test schemas and routing evaluation

* get routing working

* dont need section id

* make routing better, start adding tests

* start fixing test schemas

* Two further schema fixes

* Fix remaining schema issue

* Add integration test

* Add functional test

* Functional tests for routing and answers not on the path

* Changing an answer updates GCS progress

* Add schema for overlapping answers

* Functionally test skip-question in grand calculated summary

* Add tests for GCS routing

* Fix errors caused by merge

* type hints, comments, remove some duplication

* fix formatting

* Update translation templates

* remove accidental update

* Fix some type hints

* Use validator branch and update test description

* refactor GCS answer format

* Fix lint and test errors

* Address PR comments

* improve context and coverage of test schemas

* Fix invalid schema and add invalid routing test

* Fix line length in test

* test schema linting

* Fix routing bug

* Make schema easier to follow

* Linting

* partial fix to routing

* Add addtional routing test case

* Remove arg from new test case

* Routing to next incomplete block for summaries

* linting error

* remove arg from router tests

* PR comments

* PR Comment

* Amend comment and improve dependencies function

* Handle merge errors

* fix gcs routing within same section

* Add test for GCS with progress value source

* Remove backtick

* update chromedriver

* Revert "update chromedriver"

This reverts commit a2e3698.

* Fix schema labels

* Revert validator branch to latest

* Fix dynamic answers functional test (#1121)

* Schemas v3.59.0 (#1130)

* Schemas v3.60.0 (#1133)

* Update to chromedriver v114 (#1134)

* Add DESNZ theme (#1131)

* Schemas v3.61.0 (#1139)

* Linting, formatting and cleaning up merge with main

---------

Co-authored-by: petechd <[email protected]>
Co-authored-by: Mebin Abraham <[email protected]>
Co-authored-by: Guilhem <[email protected]>
Co-authored-by: Katie Gardner_ONS <[email protected]>
Co-authored-by: Rhys Berrow <[email protected]>
kylelawsonAND added a commit that referenced this pull request Jun 26, 2023
* Initial repeating blocks test schema

* Schema for repeating blocks and section summary incomplete + ListCollector Question handling many add types by enum.

* Repeating blocks are presented to the user and can be answered however are not posted correctly and navigation not tested.

* List Collector now always enters add_block & both add_block and edit_block go to repeating_blocks if existing.

* make format

* edit_block does not lead to repeating_blocks. All repeating_blocks show on list collector summary.

* Blocks in repeating_blocks calculated upfront. Answers from repeating_blocks shown on list collector summary underneath the relevant list item.

* Reverting list_collector handler and fixing invalid use of set

* Return to summary reused throughout ListActions

* Adding list item completion progress to progress_store and loading this upon block GET.

* Looping 2.5 - Simple Repeating Blocks in List Collector (No Item Progress Tracking) (#1100)

* Schema for repeating blocks and section summary incomplete + ListCollector Question handling many add types by enum.

* Repeating blocks are presented to the user and can be answered however are not posted correctly and navigation not tested.

* List Collector now always enters add_block & both add_block and edit_block go to repeating_blocks if existing.

* make format

* edit_block does not lead to repeating_blocks. All repeating_blocks show on list collector summary.

* Blocks in repeating_blocks calculated upfront. Answers from repeating_blocks shown on list collector summary underneath the relevant list item.

* Reverting list_collector handler and fixing invalid use of set

* Return to summary reused throughout ListActions

* Fixing existing questionnaire store tests broken by adding list item progress.

* Added remove list item progress to progress store and called from questionnaire store updater.

* List Item Progress tracked with the section and list item id keyed progresses.

* List Item Progress starts with list item add, adding the add block to the list items list of completed locations

* Make response expiry date mandatory (#1104)

* Reverting change to serialize() method naming + removing dev only chnage.

* Bind additional contexts to flush requests (#1108)

* Addressing comments to simplify progress evaluation logic + adding cached property for repeating block ids.

* Schemas v3.56.0 (#1110)

* Addressing comments regarding common usage of first incomplete repeating blocks to evaluate completeness.

* Augmenting comment in capture_dependent_sections_for_list(self, list_name)

* Bug fix to repeating block cache, list section deps reverted to dict of lists, renamed method for removing list items.

* Removing get_completed_block_ids from questionnaire store updater as it is duplicate of router.is_block_complete

* Preliminary impl for list item row icons - breaks tests

* Feat/looping 2.5 list item progress (#1105)

* Schema for repeating blocks and section summary incomplete + ListCollector Question handling many add types by enum.

* Repeating blocks are presented to the user and can be answered however are not posted correctly and navigation not tested.

* List Collector now always enters add_block & both add_block and edit_block go to repeating_blocks if existing.

* make format

* edit_block does not lead to repeating_blocks. All repeating_blocks show on list collector summary.

* Blocks in repeating_blocks calculated upfront. Answers from repeating_blocks shown on list collector summary underneath the relevant list item.

* Reverting list_collector handler and fixing invalid use of set

* Return to summary reused throughout ListActions

* Adding list item completion progress to progress_store and loading this upon block GET.

* Fixing existing questionnaire store tests broken by adding list item progress.

* Added remove list item progress to progress store and called from questionnaire store updater.

* List Item Progress tracked with the section and list item id keyed progresses.

* List Item Progress starts with list item add, adding the add block to the list items list of completed locations

* Reverting change to serialize() method naming + removing dev only chnage.

* Addressing comments to simplify progress evaluation logic + adding cached property for repeating block ids.

* Addressing comments regarding common usage of first incomplete repeating blocks to evaluate completeness.

* Augmenting comment in capture_dependent_sections_for_list(self, list_name)

* Bug fix to repeating block cache, list section deps reverted to dict of lists, renamed method for removing list items.

* Removing get_completed_block_ids from questionnaire store updater as it is duplicate of router.is_block_complete

* Fixing tests broken by list collector icon render changes

* Fixing incorrect formatting and rewording a type hint

* Cleaned up evaluation of list item icon.

* Schemas v3.57.0 (#1113)

* Reverting accidental reversion of a file

* Schemas v3.57.1 (#1115)

* Schemas v3.58.0 (#1116)

* Implement "progress" value source (#1044)

Co-authored-by: Rhys Berrow <[email protected]

* Fix handling of invalid values in form numerical inputs  (#1111)

* Passing only section id and bool for repeating blocks for list context and using kwargs where possible.

* Adding missing list context constructor arg

* Enforcing kwargs on _build_list_items_context

* Merge conflict resolution with main.

* Running make format

* Adding list collector with repeating blocks fixture and testing the repeating blocks are mapped by id correctly when schema loads.

* Completing unit testing of additions to QuestionnaireSchema

* Update Chromedriver to version 113 (#1118)

* update chromedriver

* temporarily comment out line, to be fixed separately

* ListContext unit tests

* Updating list collector summary to correctly acquire related answers and avoid breaks when list collector with same name appears twice.

* Enforcing kwargs on update_section_status and make format

* Feat/Grand calculated summary (#1107)

* initial ideas

* Implementation closer to calculated summaries

* better test schemas and routing evaluation

* get routing working

* dont need section id

* make routing better, start adding tests

* start fixing test schemas

* Two further schema fixes

* Fix remaining schema issue

* Add integration test

* Add functional test

* Functional tests for routing and answers not on the path

* Changing an answer updates GCS progress

* Add schema for overlapping answers

* Functionally test skip-question in grand calculated summary

* Add tests for GCS routing

* Fix errors caused by merge

* type hints, comments, remove some duplication

* fix formatting

* Update translation templates

* remove accidental update

* Fix some type hints

* Use validator branch and update test description

* refactor GCS answer format

* Fix lint and test errors

* Address PR comments

* improve context and coverage of test schemas

* Fix invalid schema and add invalid routing test

* Fix line length in test

* test schema linting

* Fix routing bug

* Make schema easier to follow

* Linting

* partial fix to routing

* Add addtional routing test case

* Remove arg from new test case

* Routing to next incomplete block for summaries

* linting error

* remove arg from router tests

* PR comments

* PR Comment

* Amend comment and improve dependencies function

* Handle merge errors

* fix gcs routing within same section

* Add test for GCS with progress value source

* Remove backtick

* update chromedriver

* Revert "update chromedriver"

This reverts commit a2e3698.

* Fix schema labels

* Revert validator branch to latest

* Integration tests started

* Fix dynamic answers functional test (#1121)

* Repeating blocks integration test suite complete

* Schemas v3.59.0 (#1130)

* Make format

* running and fixing mypy

* Merge with main and format and lint

* Fixing linting errors and pointing to relevant validator

* Fixing validator tag typo

* Schemas v3.60.0 (#1133)

* Fixing mocker patch of renamed method

* make format

* Update to chromedriver v114 (#1134)

* Functional test progress

* Add DESNZ theme (#1131)

* Schemas v3.61.0 (#1139)

* Adding functional test for editing repeating block answers from section summary

* Asserting completing list items in func tests and editing from section summary.

* Misc type hinting

* Misc type hinting

* Formatting

* Consolidating duplicate func test helper method and fixing typos.

* Linting, formatting and merging with feat prepop.

* Fixing broken tests

* Linting, formatting and cleaning up merge with main

* ListRepeatingBlock now extends to ListAction and does not invoke parent handle_post.

* Addressing PR comments 16-6-23

* Reverting local test changes.

* Reverting local test changes by formatting

* Reverting local test changes by formatting

* Adding additional docs and type hints to ProgressStore

* Addressing PR comments.

* Fixing list collector (no RP) icons

* Removing person icon change.

* Adding repeating blocks to the submission payload and renaming list name in test schema.

* Adding test coverage for repeating block answers in submission payload.

* Reformat

* Adding func test and schema for a repeating blocks LC on a hub questionnaire.

* Fixing test broken in prev commit.

* Reformatting.

* Updating naming of progress store methods follwoing convos.

* Updating comment

* Updating comment

* Formatting

* Addressing comments

* Remvovign whitespace from schema

* Amending naming and type hinting in list collector block  summary and adding context to list collector RP schema.

* Changing to unit and currency in simple RB schema.

* Formatting

* Removing redundant test schema

* Using latest validator

---------

Co-authored-by: petechd <[email protected]>
Co-authored-by: Mebin Abraham <[email protected]>
Co-authored-by: Guilhem <[email protected]>
Co-authored-by: Katie Gardner_ONS <[email protected]>
Co-authored-by: Rhys Berrow <[email protected]>
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.

7 participants