Request IR by Post from Hub #238

iwootten · 2020-08-14T13:26:09Z

What is the context of this PR?

Added a person selector to the individual response. The selector is only shown when requested via the hub and 2 or more non-primary householders exist. The selector is skipped on paths from the hub where conditions aren't met.

How to review

Check to see if you agree with the approach.

Checklist

New static content marked up for translation

LJBabbage

Works as expected, a few minor points, nothing major

LJBabbage · 2020-08-15T21:25:15Z

app/routes/individual_response.py

@@ -63,10 +65,10 @@ def request_individual_response(schema, questionnaire_store):
        list_item_id=list_item_id,
    )

-    if request.method == "GET":
-        return individual_response_handler.handle_get()
+    if request.method == "POST":


I couldn't immediate see a reason for it to change to POST, I am sure there must be. I only raise it as in other routes we usually do GET first

The default fallback option should really always be the call to the get. The only other place I can see this is 3 cases in questionnaire.py, where the logic on the get condition is confusing because it involves a call to the post form. They should probably be changed too.

LJBabbage · 2020-08-15T21:28:26Z

app/routes/individual_response.py

+def get_individual_response_who(schema, questionnaire_store):
+    language_code = get_session_store().session_data.language_code
+
+    member_selector_handler = IndividualResponseWhoHandler(


Reading the card I understand why you have called it member_selector_handler, just looks a little odd with all the others called individual_response_handler(not sure this is right either) and the name of your handler. I think either we name all of them better or be consistent across all of them.

LJBabbage · 2020-08-15T21:31:22Z

app/views/contexts/hub_context.py

@@ -41,6 +44,26 @@ class HubContext(Context):
        },
    }

+    def __init__(


for_list is only used in _individual_response_enabled as far as I can tell, so I am not sure it needs to be add to the init just _individual_response_enabled, which naturally means we could get rid of the whole init .

Also once in there you could get rid of the if statement as its already checked

LJBabbage · 2020-08-15T22:01:50Z

app/views/contexts/hub_context.py


        if count_household_members == 0:
            return False

-        if count_household_members == 1 and self._list_store[for_list].primary_person:
+        if (


Might be worth putting it on one line

if count_household_members == 0 or ( count_household_members == 1 and self._list_stor[for_list].primary_person ): return False

LJBabbage · 2020-08-15T22:17:59Z

app/views/handlers/individual_response.py

@@ -225,16 +237,19 @@ def __init__(
        )

    def handle_get(self):
+        previous_location_url = url_for(


Rather than call url_for potentially twice, perhaps just else the return_to_hub?

LJBabbage · 2020-08-16T08:01:03Z

app/views/handlers/individual_response.py

+        return self.form.get_data(answer_id)
+
+    @cached_property
+    def selected_list_item(self):


Very minor, but I would absorb selected_option in here, I understood what select_list_item meant without the extra method and with it not being used anywhere else I feel it could sit as one.

This is the approach adopted in the previous class, I think it's clearer to separate them.

LJBabbage · 2020-08-16T08:09:09Z

app/views/handlers/individual_response.py

+                    )
+                )
+
+        self.people_names = {


I think we lose a bit of context with the var name, when I got to the handle_get and looking at len(self.people_names) > 1 I couldn't tell if this included primary or not. On checking back I can see its has been removed, the name should perhaps reflect that

LJBabbage · 2020-08-16T08:15:53Z

tests/integration/individual_response/test_individual_response.py

+        self.assertInUrl("/how")
+
+    def test_goes_to_who_selector(self):
+        # Given I add a household member


2 household members / or just members

LJBabbage · 2020-08-16T08:16:09Z

tests/integration/individual_response/test_individual_response.py

+        self.assertInUrl("/who")
+
+    def test_previous_returns_to_hub(self):
+        # Given I add a household member


Same as above

LJBabbage · 2020-08-16T08:21:54Z

app/views/handlers/individual_response.py

+
+class IndividualResponseWhoHandler(IndividualResponseHandler):
+    def __init__(self, schema, questionnaire_store, language, request_args, form_data):
+        self._list_name = schema.json.get("individual_response", {}).get("for_list")


As we are calling schema.json.get("individual_response", {}) a few times now, would it be best to have its own method in QuestionnaireSchema?

LJBabbage · 2020-08-17T09:18:14Z

app/views/contexts/hub_context.py

-        count_household_members = len(self._list_store[self._for_list])
+        for_list = (
+            self._schema.json["individual_response"]["for_list"]
+            if self._schema.json.get("individual_response")


At this stage you know individual_response is true, due to the if above it

MebinAbraham · 2020-08-17T11:19:34Z

app/routes/individual_response.py

+    )
+
+
+@individual_response_blueprint.route("/who", methods=["GET", "POST"])


The previous link for this journey should back to the previous page. It should only go to the hub when on you are back on the IR request launcher page.

Scenarios:

1 non-primary person only:

The previous link on the how page is also adding list item id to the url which is taking it back to the individual section from IR request launcher page instead of the hub. Regardless of the number of people, the previous should go back to the hub if we know they came from the hub?.

at least 2 non-primary person

The previous link from any IR page is going straight to the hub, instead of back sequentially to the IR request launcher page then the hub.

Hopefully that is clear, if not, happy to discuss further.

I think the card has overstated this somewhat which has made it confusing "Previous link should always go back to hub no matter how far along the IR journey you are (ensure alternate journey goes to individual section introduction page)."

In all honesty, I'm not sure why the this requirement was specified as it would be the expected behaviour?

I agree that comment has overstated it @iwootten - I think it was added to differentiate between the hub originated journey and the individual section originated journey, but that would be default behaviour as you pointed out 😄

MebinAbraham

Apart from the previous journey, the feature to select who to request IR for is working as expected.

MebinAbraham · 2020-08-18T17:07:00Z

app/views/handlers/individual_response.py

+        if self._return_to_hub:
            previous_location_url = url_for(
-                "individual_response.get_individual_response_change",
-                list_item_id=self._list_item_id,
+                "individual_response.get_individual_response_who",
+                return_to=self._return_to,
            )
-        else:
+        elif self._request_args.get("journey") == "change":


I think we should use journey instead of return_to when the immediate page isn't the return to location. return to makes sense if the next page or previous page would be the return to page, but journey was recently introduced to track the journey between multiple screens.

MebinAbraham · 2020-08-18T17:18:41Z

app/views/handlers/individual_response.py

+            self._list_name = self._schema.get_individual_response_list()
+            list_model = self._questionnaire_store.list_store[self._list_name]
+            non_primary_members = 0
+
+            for list_item in list_model.items:
+                if list_item != list_model.primary_person:
+                    non_primary_members += 1
+
+            if non_primary_members > 1:


This isn't needed if we persist the request args, because that would tell you if you came from the hub.

This is able to be edited though?

Do you mean the query param can be edited?. If so, yes but that shouldn't matter as long as it is one of the ones we are expecting, as we need to handle that scenario anyway.

As discussed offline, it's likely this logic doesn't belong in this handler if we skip over it when there is only a single member.

MebinAbraham · 2020-08-18T17:21:29Z

app/views/handlers/individual_response.py

+    def handle_post(self):
+        if self._list_item_id:
+            return redirect(
+                url_for(".get_individual_response_how", list_item_id=self._list_item_id)
+            )
+        return redirect(url_for(".get_individual_response_who", return_to="hub"))


At the moment, when there is only 1 non-primary person, this is redirecting to the who route which then redirects to how route, but we should be avoiding that extra redirect as we can work out at this point whether to go to the who route or the how route based on the count of non-primary list items.

templates/hub.html

Co-authored-by: Mebin Abraham <[email protected]>

MebinAbraham

The previous links are not working correctly. If you go to the request IR page, the previous goes to a 404 page. If you step forward to the how screen, then clicking previous twice goes back to the individual section even though for both journeys I started from the hub.

MebinAbraham · 2020-08-20T09:47:27Z

app/views/handlers/individual_response.py

+        non_primary_members = []
+
+        for list_item in list_model.items:
+            if list_item != list_model.primary_person:
+                non_primary_members.append(list_item)
+
+        if len(non_primary_members) == 1:


I think we should have a property on list model to return the non primary list item ids. Which can then be reused here, as well as in:

https://github.com/ONSdigital/eq-questionnaire-runner/pull/238/files#diff-1b7aa7bc01cf9df6f320a4fd4a937b8aR544-R545

https://github.com/ONSdigital/eq-questionnaire-runner/pull/238/files#diff-c8d1a13e851293923c3a04e3f7d0d1c0R186-R188

app/data_model/list_store.py

MebinAbraham · 2020-08-20T12:17:31Z

app/views/contexts/hub_context.py

        count_household_members = len(self._list_store[for_list])

-        if count_household_members == 0:
-            return False
-
-        if count_household_members == 1 and self._list_store[for_list].primary_person:
+        if count_household_members == 0 or (
+            count_household_members == 1 and self._list_store[for_list].primary_person
+        ):


This can now be simplied to:

if not self._list_store[for_list].non_primary_people: return False

MebinAbraham · 2020-08-20T12:19:04Z

app/views/handlers/individual_response.py

+        non_primary_members = []
+
+        for list_item in list_model.items:
+            if list_item != list_model.primary_person:
+                non_primary_members.append(list_item)


This should be replaced by list_model.non_primary_people.

Co-authored-by: Mebin Abraham <[email protected]>

MebinAbraham · 2020-08-20T14:56:33Z

app/views/handlers/individual_response.py

+        if self._is_hub_journey:
+            previous_location_url = url_for("questionnaire.get_questionnaire")
+        else:
            previous_location_url = url_for(
                "questionnaire.block",
                list_name=self._list_name,
                list_item_id=self._list_item_id,
                block_id=individual_section_first_block_id,
            )


This logic means if you manually visit the url, which is possible, you would get a 404 because there is no query param for list item id. Therefore, we should by default go to the hub if there is no query param. Right now it would be safe to assume if there is no list item id, then you should go to the hub. So it could be:

if self._list_item_id: previous_location_url = url_for( "questionnaire.block", list_name=self._list_name, list_item_id=self._list_item_id, block_id=individual_section_first_block_id, ) else: previous_location_url = url_for("questionnaire.get_questionnaire")

MebinAbraham

Apart from my last comment, everything seems to be as expected. 👍

Add response from hub

d9af537

iwootten force-pushed the request-hub-post-response branch from 7abc92b to d9af537 Compare August 14, 2020 13:28

Reinsert accidental condition removal

4656daa

LJBabbage suggested changes Aug 16, 2020

View reviewed changes

iwootten added 2 commits August 17, 2020 10:07

Update against feedback

bff1c34

Merge branch 'master' into request-hub-post-response

f0e3a87

LJBabbage suggested changes Aug 17, 2020

View reviewed changes

Remove unneccessary condition

9e7bdab

MebinAbraham requested changes Aug 17, 2020

View reviewed changes

MebinAbraham reviewed Aug 17, 2020

View reviewed changes

LJBabbage approved these changes Aug 17, 2020

View reviewed changes

iwootten added 3 commits August 17, 2020 17:12

Fix previous routing

0365cb4

Make tests more explicit

84b86ad

Merge branch 'master' into request-hub-post-response

dd5e376

MebinAbraham reviewed Aug 18, 2020

View reviewed changes

iwootten and others added 4 commits August 19, 2020 09:42

Update templates/hub.html

fae5c27

Co-authored-by: Mebin Abraham <[email protected]>

Prevent redirect by moving into previous stage

860a4d6

Use journey arg over return_to

bf038c8

Merge branch 'master' into request-hub-post-response

693ee5e

pricem14pc added this to the v3.48.0 milestone Aug 19, 2020

Merge branch 'master' into request-hub-post-response

83921be

MebinAbraham reviewed Aug 20, 2020

View reviewed changes

iwootten added 2 commits August 20, 2020 12:41

Add non_primary property to list store

da5ad48

Don't call .keys()

62853c6

MebinAbraham reviewed Aug 20, 2020

View reviewed changes

iwootten and others added 2 commits August 20, 2020 13:54

Update app/data_model/list_store.py

67846eb

Co-authored-by: Mebin Abraham <[email protected]>

Update to use new property

f94cf74

MebinAbraham reviewed Aug 20, 2020

View reviewed changes

Flip fallback previous url

8187657

MebinAbraham approved these changes Aug 20, 2020

View reviewed changes

iwootten merged commit 9f8415c into master Aug 20, 2020

iwootten deleted the request-hub-post-response branch August 20, 2020 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request IR by Post from Hub #238

Request IR by Post from Hub #238

iwootten commented Aug 14, 2020

LJBabbage left a comment

LJBabbage Aug 15, 2020

iwootten Aug 17, 2020

LJBabbage Aug 15, 2020

LJBabbage Aug 15, 2020

LJBabbage Aug 15, 2020

LJBabbage Aug 15, 2020

LJBabbage Aug 16, 2020

iwootten Aug 17, 2020

LJBabbage Aug 16, 2020 •

edited

Loading

LJBabbage Aug 16, 2020

LJBabbage Aug 16, 2020

LJBabbage Aug 16, 2020

LJBabbage Aug 17, 2020

MebinAbraham Aug 17, 2020 •

edited

Loading

iwootten Aug 17, 2020 •

edited

Loading

JamesGardiner Aug 18, 2020

MebinAbraham left a comment

MebinAbraham Aug 18, 2020

MebinAbraham Aug 18, 2020

iwootten Aug 19, 2020

MebinAbraham Aug 19, 2020

iwootten Aug 19, 2020

MebinAbraham Aug 18, 2020

MebinAbraham left a comment

MebinAbraham Aug 20, 2020

MebinAbraham Aug 20, 2020

MebinAbraham Aug 20, 2020

MebinAbraham Aug 20, 2020 •

edited

Loading

MebinAbraham left a comment •

edited

Loading

		)


		@individual_response_blueprint.route("/who", methods=["GET", "POST"])

Request IR by Post from Hub #238

Request IR by Post from Hub #238

Conversation

iwootten commented Aug 14, 2020

What is the context of this PR?

How to review

Checklist

LJBabbage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LJBabbage Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MebinAbraham Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

iwootten Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MebinAbraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MebinAbraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MebinAbraham Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

MebinAbraham left a comment • edited Loading

Choose a reason for hiding this comment

LJBabbage Aug 16, 2020 •

edited

Loading

MebinAbraham Aug 17, 2020 •

edited

Loading

iwootten Aug 17, 2020 •

edited

Loading

MebinAbraham Aug 20, 2020 •

edited

Loading

MebinAbraham left a comment •

edited

Loading