Feat/looping 2.5 list item progress #1105
Conversation
…ector Question handling many add types by enum.
…r are not posted correctly and navigation not tested.
…block go to repeating_blocks if existing.
…ow on list collector summary.
…_blocks shown on list collector summary underneath the relevant list item.
…is upon block GET.
# Conflicts: # app/questionnaire/questionnaire_store_updater.py # app/views/handlers/list_add_question.py # app/views/handlers/list_repeating_block.py
…stionnaire store updater.
… the list items list of completed locations
There was a problem hiding this comment.
Functionally progress updating as expected, adding from the summary, removing, and then going back into the list collector if adding more, working well
PR description/changes is a bit outdated but rest is good - might be good to run the formatter on it so it doesn't auto fail the linter on merge
…ched property for repeating block ids.
app/views/handlers/list_collector.py
Outdated
|
|
||
| complete_block_ids = self.questionnaire_store_updater.get_completed_block_ids(self.current_location.section_id, list_item_id) | ||
| for repeating_block in repeating_blocks: | ||
| if repeating_block["id"] not in complete_block_ids: |
There was a problem hiding this comment.
Don't think it's walrus-able after latest changes - but maybe I can't figure out the syntax
…ing blocks to evaluate completeness.
app/views/handlers/list_collector.py
Outdated
| list_model = self._questionnaire_store.list_store.get(self.list_name) | ||
| return all( | ||
| self.questionnaire_store_updater.is_section_complete(section_id=self.current_location.section_id, list_item_id=list_item_id) | ||
| for list_item_id in list_model.items) |
There was a problem hiding this comment.
Can this not use get_first_incomplete_repeating_block_location?
There was a problem hiding this comment.
Another nice catch - the method will essentially just be calling that method and returning the inverted bool interpretation of the result so arguably could be removed, but I still think it's worth keeping so the usage of get_first_incomplete_repeating_block_location for the purpose of list collector completeness is clear.
There was a problem hiding this comment.
I think the usage of get_first_incomplete_repeating_block_location doesn't become unclear if we did this?.
I think it is beneficial because:
- Avoid having another bit of logic to get the same end result
- Keeps it consistent with how we already do it. This is what happen in the
is_path_completemethod, which invertsget_first_incomplete_...method. - Personally, I don't think it is unclear either, currently it is, all sections for the list item are complete, it would just become, all sections don't have an incomplete or not any section have incomplete blocks.
If I had to choose one reason it would simply be for consistency. See what others think and go with consensus.
There was a problem hiding this comment.
Sorry I wasn't meaning that I was keeping this method as is, I agree with you that it should be changed to use the inverted result of get_first_incomplete_repeating_block_location, which has now been implemented
There was a problem hiding this comment.
Ah cool, I probably misunderstood. 👍
There was a problem hiding this comment.
Bad phrasing from me, is my bad
…of lists, renamed method for removing list items.
app/views/handlers/question.py
Outdated
| complete_block_ids = self.questionnaire_store_updater.get_completed_block_ids(section_id, list_item_id) | ||
| for repeating_block_id in repeating_block_ids: | ||
| if repeating_block_id not in complete_block_ids: |
There was a problem hiding this comment.
Minor, we currently use is_block_complete method to check this in other places. Non-blocker, happy to keep as is. Would mean you don't need to add get_completed_block_ids to updater.
i.e
for repeating_block_id in repeating_block_ids:
if self.router.is_block_complete(
section_id=section_id,
list_item_id=list_item_id,
block_id=repeating_block_id,
):
return Location(
section_id=section_id,
block_id=repeating_block_id,
list_name=list_name,
list_item_id=list_item_id,
)There was a problem hiding this comment.
Ah didn't catch that method was kicking around in router, have removed mine and used that 👍
There was a problem hiding this comment.
I clearly was testing you by putting if complete instead of if not complete 😄
Looks good 👍
There was a problem hiding this comment.
Hahah github comment box is the best python IDE i heard ;)
…it is duplicate of router.is_block_complete
There was a problem hiding this comment.
Functionally works as expected, I can see the progress for each list item when dump/debug. Also happy with the changes from my level of understanding.
I just have a question just for my own curiosity, in the future are we planning to make it clearer which list items we are completing. For example if I enter car companies one, two, three and four. I complete one and three but do not fully complete two and four. Then I press save and continue, it displays the next sections for car two since its in-complete but it is a generic page that does not have any indication that we are filling it out for car two. Sorry if that did not make any sense.
Yeah this was a concern of mine, but it will be handled via placeholders which will say the item's Add block answer values which will be displayed ✨ somewhere ✨ I'll update my test schema to use placeholders in the next mini PR |
* 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]>
What is the context of this PR?
This PR builds upon the simple repeating blocks iteration of the Looping 2.5 Repeating Blocks feature.
This PR augments the
Progress Storeto capture the completion progress of list items. This allows runner to have context about whether list items have been fully completed between block routings, enabling the ability for the list collector summary to show the correct icon based on completion of items (not yet done), as well as route to incomplete ones (done).Changes
progress_store.py:Updated some type hints.
List items are added to this store keyed by id of the section in which a list item's list collector appears, and the list item id. Their completed locations are the add block and repeating blocks if they exist.
questionnaire_schema:
Used walrus operator to simplify condition check of repeating blocks.
Returning false if answer id does not belong to a block when calling
is_answer_in_repeating_blocksquestionnaire_store_updater.py: Updated the method description for
remove_list_item_and_answersto reflect the changes it has on the progress store for list items and renamed the method toremove_list_item_data.list_remove_question.py: Makes use of newly named
remove_list_item_datalist_repeating_block.py: Added cached property for
repeating_block_id.get_next_location_urlnow makes use ofget_first_incomplete_repeating_block_location_for_list_itemto navigate to the next repeating block.handle_post(self)now callsadd_completed_locationon theQuestionnaireStoreUpdaterand marks the list item complete if all of the repeating blocks have a completed location on the list items progress.list_collector.py: Added cached properties for
repeating_block_idsandlist_namefor the list that it represents.get_next_location_url(self)now navigates to the first incomplete block of the first incomplete list item, if any exist, else navigate to the next block on the path as normal.Added method to evaluate list collector completeness.
handlers/question.py:
capture_dependent_sections_for_list(self, list_name)ensures that only repeating sections and the section within which the list collector for the list item occurs, are captured as dependant sections - filters out the list items progresses as they should not be dependants.Added public methods for use by list collector handlers to get the first incomplete block from a list collector or from a list item, the latter is used by the former, but may be used individually.
How to review
The current implementation of list item progress makes sense and the happy path of completing the Looping 2.5 journey still works while tracking progress between routes - without the list collector summary showing list item completeness of list items, however it should enforcing returning to list items repeating blocks if they were skipped.
Make use of the test schema
list_collector_repeating_blocks_section_summary.jsonwhen testing this.You can visit the dump/debug route to see the list item progress as you complete the list collector and see how it changes as blocks are completed as well as when list items are removed. Once all repeating blocks of a list item are complete, you should see that the list item itself is marked complete in the dump.
Most pre-existing tests should be passing but if they fail (particularly test_with_primary_person and test_without_primary_person) they will be fixed in a future PR.
Future PRs
Displaying list item completeness on list collector summary
Unit testing
Functional testing
Additional test schemas
Type hinting files/methods touched by this feature impl