Add DataStores class to questionnaire store #1228

petechd · 2023-10-17T07:35:30Z

What is the context of this PR?

This PR encapsulates all our questionnaire store's data stores into single DataStores class. I've added the optional methods to initialise ValueSourceResolver, RuleEvaluator,PlaceholderRenderer to further reduce repetition. New class is kept in questionnaire store module for now, just to get it working in the first pass but happy to move it out.

How to review

Running functional and unit tests plus some manual checks in live schemas and visual code checks should be sufficient.

Checklist

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

petechd · 2023-10-17T11:32:38Z

Encapsulate questionnaire store data stores into a single class (3 SP)

katie-gardner

Started looking though this but some changes look quite strange, like in placeholder_parser was this done with a tool? I think it's had some unintended effects

app/data_models/questionnaire_store.py

app/views/contexts/summary/question.py

app/data_models/questionnaire_store.py

app/questionnaire/rules/rule_evaluator.py

app/questionnaire/value_source_resolver.py

app/submitter/converter_v2.py

app/questionnaire/placeholder_parser.py

app/questionnaire/value_source_resolver.py

app/data_models/data_stores.py

app/data_models/questionnaire_store.py

app/forms/questionnaire_form.py

app/questionnaire/placeholder_renderer.py

app/data_models/data_stores.py

katie-gardner

Will do a second pass but from a first look only found two small things 👍

app/views/contexts/summary/question.py

app/views/contexts/summary/block.py

app/views/contexts/summary/question.py

katie-gardner

Finished going through now - just a few more small things

app/questionnaire/router.py

app/questionnaire/rules/rule_evaluator.py

app/views/contexts/calculated_summary_context.py

tests/app/forms/test_questionnaire_form.py

tests/app/questionnaire/test_placeholder_renderer.py

tests/app/questionnaire/test_router.py

tests/app/questionnaire/conftest.py

tests/app/questionnaire/test_value_source_resolver.py

liamtoozer

Looks great from what I can see & understand! Just had a couple of learning questions but no worries if there's not enough time

app/views/contexts/calculated_summary_context.py

app/questionnaire/placeholder_renderer.py

katie-gardner

Everything resolved except one minor thing off a previous comment

tests/app/questionnaire/test_placeholder_renderer.py

tests/app/questionnaire/test_value_source_resolver.py

katie-gardner

My comments all fixed 👍

app/data_models/data_stores.py

berroar

Looks good 👍

MebinAbraham

Not done an in-depth re-review, only a quick scan, mostly looks all okay. Noticed QS is inconsistent with attribute access, some private, some public, metadata has a setter, others doesn't, one to sort out in a future PR.

app/forms/questionnaire_form.py

app/questionnaire/questionnaire_store_updater.py

tests/app/data_model/test_questionnaire_store.py

MebinAbraham

Do have mixed feelings about this change but in general I think the change is for the better. I think we've accepted that we have sacrificed some readability for maintainability/extendibility.

LGTM 👍

petechd added 2 commits October 16, 2023 12:12

Add initial changes

22ea452

Fix remaining tests

cfc0537

petechd changed the title ~~Add data stores class to questionnaire store~~ Add DataStores class to questionnaire store Oct 17, 2023

petechd added 2 commits October 17, 2023 09:28

Remove logic from can_access_location

afdadfc

Add data stores to summary question

698a6d8

katie-gardner suggested changes Oct 17, 2023

View reviewed changes

petechd added 3 commits October 17, 2023 15:25

Use data stores placeholder renderer

c279dd2

Refactor converter and its tests

e34553b

Use default factory on all in DataStores class

f6b1b51

Yuyuutsu reviewed Oct 18, 2023

View reviewed changes

app/questionnaire/value_source_resolver.py Outdated Show resolved Hide resolved

MebinAbraham reviewed Oct 18, 2023

View reviewed changes

petechd force-pushed the add-data-stores-class-to-questionnaire-store branch from a31f5c1 to f6b1b51 Compare October 23, 2023 10:26

petechd and others added 5 commits October 23, 2023 12:01

Move data stores to separate module

58f26a2

Use datastore in remaining places, remove helper methods

a2d9bfd

Fix progress store import

7fb217a

Merge branch 'main' into add-data-stores-class-to-questionnaire-store

6208c21

Revert can_access_location condition

f0e09cb

katie-gardner reviewed Oct 24, 2023

View reviewed changes

app/views/contexts/summary/question.py Outdated Show resolved Hide resolved

app/views/contexts/summary/block.py Outdated Show resolved Hide resolved

app/views/contexts/summary/question.py Show resolved Hide resolved

petechd and others added 2 commits October 25, 2023 08:49

Remove commented out code, redundant vars

977db5f

Merge branch 'main' into add-data-stores-class-to-questionnaire-store

b6b5b0b

katie-gardner suggested changes Oct 25, 2023

View reviewed changes

petechd and others added 6 commits October 25, 2023 11:28

Fix merge conflicts issues after introduction of return location

c325439

Fix missing test code logic, sort some imports and some more tests

fff4632

Add misc review changes

626772c

Format files

82d61d0

Initialise data stores only once in test_router

60f8019

Merge branch 'main' into add-data-stores-class-to-questionnaire-store

e2476aa

MebinAbraham added the dont merge Don't merge this PR label Oct 26, 2023

liamtoozer reviewed Oct 26, 2023

View reviewed changes

app/views/contexts/calculated_summary_context.py Outdated Show resolved Hide resolved

liamtoozer reviewed Oct 26, 2023

View reviewed changes

app/questionnaire/placeholder_renderer.py Show resolved Hide resolved

Remove type ignore

e1f73fe

katie-gardner reviewed Oct 26, 2023

View reviewed changes

tests/app/questionnaire/test_placeholder_renderer.py Show resolved Hide resolved

tests/app/questionnaire/test_value_source_resolver.py Show resolved Hide resolved

petechd and others added 2 commits October 27, 2023 09:23

Fix param calls in test methods

e487099

Merge branch 'main' into add-data-stores-class-to-questionnaire-store

830d0fa

katie-gardner approved these changes Oct 30, 2023

View reviewed changes

katie-gardner reviewed Oct 30, 2023

View reviewed changes

app/data_models/data_stores.py Show resolved Hide resolved

Fix comment

9b3cca9

MebinAbraham reviewed Nov 1, 2023

View reviewed changes

app/data_models/data_stores.py Show resolved Hide resolved

Remove comment

df90526

berroar approved these changes Nov 6, 2023

View reviewed changes

petechd added 2 commits November 6, 2023 09:52

Rename data_stores to stores in QS

2be9dd5

Rename stores

b3add18

MebinAbraham reviewed Nov 8, 2023

View reviewed changes

app/forms/questionnaire_form.py Outdated Show resolved Hide resolved

app/questionnaire/questionnaire_store_updater.py Outdated Show resolved Hide resolved

tests/app/data_model/test_questionnaire_store.py Outdated Show resolved Hide resolved

Add review fixes

990214f

MebinAbraham approved these changes Nov 9, 2023

View reviewed changes

Merge branch 'main' into add-data-stores-class-to-questionnaire-store

1890362

MebinAbraham removed the dont merge Don't merge this PR label Nov 13, 2023

Merge branch 'main' into add-data-stores-class-to-questionnaire-store

74d8ce7

petechd merged commit ff52496 into main Nov 14, 2023
15 checks passed

petechd deleted the add-data-stores-class-to-questionnaire-store branch November 14, 2023 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DataStores class to questionnaire store #1228

Add DataStores class to questionnaire store #1228

petechd commented Oct 17, 2023 •

edited

Loading

petechd commented Oct 17, 2023

katie-gardner left a comment

katie-gardner left a comment

katie-gardner left a comment

liamtoozer left a comment

katie-gardner left a comment

katie-gardner left a comment

berroar left a comment

MebinAbraham left a comment

MebinAbraham left a comment

Add DataStores class to questionnaire store #1228

Add DataStores class to questionnaire store #1228

Conversation

petechd commented Oct 17, 2023 • edited Loading

What is the context of this PR?

How to review

Checklist

petechd commented Oct 17, 2023

katie-gardner left a comment

Choose a reason for hiding this comment

katie-gardner left a comment

Choose a reason for hiding this comment

katie-gardner left a comment

Choose a reason for hiding this comment

liamtoozer left a comment

Choose a reason for hiding this comment

katie-gardner left a comment

Choose a reason for hiding this comment

katie-gardner left a comment

Choose a reason for hiding this comment

berroar left a comment

Choose a reason for hiding this comment

MebinAbraham left a comment

Choose a reason for hiding this comment

MebinAbraham left a comment

Choose a reason for hiding this comment

petechd commented Oct 17, 2023 •

edited

Loading