PC-1463: Improve UX for when no address found #410

samyou-softwire · 2024-12-04T15:31:12Z

Description

adds routing logic to send directly to address manual if no addresses are found

also adds extra logic on the address page to check whether it will find any addresses. this does mean that each flow that uses OS API will send twice the number of requests. if this is too undesirable then can look into caching the result here

Checklist

I have made any necessary updates to the documentation
I have checked there are no unnecessary IDE warnings in this PR
I have checked there are no unintentional line ending changes
I have added tests where applicable
If I have made any changes to the code, I have used the IDE auto-formatter on it
If I have made any changes to Python files (even if not changing any content strings), I have run make extract-translations
If I have made any changes to website flow, I have updated the Flow Miro Board
If I have made any changes to website flow, I have checked forward and back behaviour is still consistent

Screenshots

samyou-softwire · 2024-12-04T15:43:33Z

help_to_heat/templates/frontdoor/address-manual.html

@@ -13,6 +13,22 @@ <h1 class="govuk-heading-l">
      {{_("What is the property's address?")}}
    </h1>
  </legend>
+
+  {% if show_address_manually_error %}


this isn't an error in the normal sense in GBIS (it has no linked field, is not a validation error) so decided to add a specific case here rather than trying to extend the error showing logic

samyou-softwire · 2024-12-05T11:05:05Z

tests/test_frontdoor.py

@@ -2032,41 +2031,6 @@ def test_on_epc_select_page_enter_manually_can_be_selected():
    assert page.has_one('h1:contains("What is the property\'s address?")')


-@unittest.mock.patch("help_to_heat.frontdoor.interface.EPCApi", MockNotFoundEPCApi)


this test is no longer meaningful; the user no longer needs to click the select manual link

jdgage

I'm puzzled by the whole approach: we call the API to check the number of results, then set a flag in the session, then immediately read from that flag again to determine the next page - have I got that right? Why do we need that extra machinery? Can we not pass values through to navigation without saving them to the session?

jdgage · 2024-12-05T18:39:39Z

help_to_heat/frontdoor/views.py

+                    check_address_has_results_in_address_api()
            else:
                data[address_choice_field] = address_choice_field_write_address
+                check_address_has_results_in_address_api()
        except Exception as e:  # noqa: B902
            logger.exception(f"An error occurred: {e}")
            data[address_choice_field] = address_choice_field_epc_api_fail
+            check_address_has_results_in_address_api()


Doing this feels brittle. I think I'd rather have one call after the try-catch. I don't follow why we don't call it in the one branch - why's that?

in some cases we don't care for what the OS API response is, the only case being if the epc api returns results. could rewrite the logic to include just one call by checking afterwards

jdgage

Another general approach point: are we now making an API call to check how many results, then throwing it away and making the exact same API call on the next page? That feels silly - it's worth interrogating why we can't just fetch the list once.

jdgage · 2024-12-06T16:33:38Z

help_to_heat/frontdoor/views.py

+        will_use_os_api = (
+            data[address_choice_field] == address_choice_field_epc_api_fail
+            or country == country_field_scotland
+            and data[address_choice_field] == address_choice_field_write_address
+        )


Are we now re-implementing logic from above (which could be equally brittle)? This is why I'd consider returning early in the case(s) where we don't want to do it.

samyou-softwire · 2024-12-09T10:14:23Z

Another general approach point: are we now making an API call to check how many results, then throwing it away and making the exact same API call on the next page? That feels silly - it's worth interrogating why we can't just fetch the list once.

this is something that I think is possible and there is time to do; can look into tweaking this to request the os api just the one time

as well as respective tests, including this new route into flow generators

samyou-softwire · 2024-12-09T12:36:33Z

@jdgage this should be ready for re-review now, code now only makes one api call, & the logic should be less brittle (no more re-expression)

jdgage · 2024-12-09T17:09:13Z

This looks better: still refer back to the first comment: why do we need this intermediate field that we write to and then immediately read from?

samyou-softwire · 2024-12-10T09:47:23Z

I'm puzzled by the whole approach: we call the API to check the number of results, then set a flag in the session, then immediately read from that flag again to determine the next page - have I got that right? Why do we need that extra machinery? Can we not pass values through to navigation without saving them to the session?

in general this is our mechanism for sending information to navigation, saving to session makes the routing reproducible. it's not too different from saving the answer to session and immediately querying that

jdgage · 2024-12-10T12:27:30Z

in general this is our mechanism for sending information to navigation, saving to session makes the routing reproducible. it's not too different from saving the answer to session and immediately querying that

Sure. I think the reason it feels weird is that we've got these 'navigation-only' fields all together with actual user input fields? Can we make the distinction clearer?

samyou-softwire · 2024-12-11T12:09:12Z

in general this is our mechanism for sending information to navigation, saving to session makes the routing reproducible. it's not too different from saving the answer to session and immediately querying that

Sure. I think the reason it feels weird is that we've got these 'navigation-only' fields all together with actual user input fields? Can we make the distinction clearer?

have renamed these as 'journey fields' to make the distinction clearer

jdgage · 2024-12-09T15:36:47Z

help_to_heat/frontdoor/consts.py

@@ -149,7 +149,9 @@
 address_choice_field_enter_manually = "enter manually"
 address_building_name_or_number_field = "building_name_or_number"
 address_postcode_field = "postcode"
-address_all_address_and_lmk_details_field = "address_and_lmk_details"
+# PC-1463: removed lmk from field name as it stores info from both flows (lmk & uprn)


Commit message should have the ticket number, so don't need that here. Otherwise I think it's missing half of the why: why does the value still refer to 'LMK'?

changing the value would mean the key used in the users' session would change and we'd need to consider not breaking existing sessions. the constant name can change for free. I think it's fine for a small semantic change here, though if we'd rather change the value too can look into that

Yes, that's fine, but the comment should say this

have updated this

jdgage · 2024-12-09T15:37:12Z

help_to_heat/frontdoor/consts.py

-address_all_address_and_lmk_details_field = "address_and_lmk_details"
+# PC-1463: removed lmk from field name as it stores info from both flows (lmk & uprn)
+address_all_address_and_details_field = "address_and_lmk_details"
+address_no_results_field = "no_results"  # yes/no options


What are these comments adding?

#yes/no options just to mark that that field takes a yes or a no. can go and update the rest of such fields to the same effect

jdgage · 2024-12-11T18:16:14Z

Some comments about comments, otherwise all fine

to make their use distinct from user input fields

jdgage · 2024-12-13T12:18:27Z

This is good to go

samyou-softwire requested review from jdgage and Glenn-Clarke December 4, 2024 15:32

samyou-softwire self-assigned this Dec 4, 2024

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from 55d1d46 to 57f2031 Compare December 4, 2024 15:33

samyou-softwire commented Dec 4, 2024

View reviewed changes

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch 2 times, most recently from eb6db71 to cf829d9 Compare December 5, 2024 10:36

samyou-softwire commented Dec 5, 2024

View reviewed changes

jdgage requested changes Dec 5, 2024

View reviewed changes

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from cf829d9 to ab1f9bd Compare December 6, 2024 15:49

jdgage requested changes Dec 6, 2024

View reviewed changes

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from ab1f9bd to 08deb05 Compare December 9, 2024 12:32

PC-1463: add routing logic for no results found on address enter page

dfcb73e

as well as respective tests, including this new route into flow generators

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from 08deb05 to 041dce9 Compare December 9, 2024 12:34

samyou-softwire added 2 commits December 9, 2024 13:46

PC-1463: implement view logic to send to new page

def95f6

PC-1463: add enter manually error to view & template

cb0cc67

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from 041dce9 to f585966 Compare December 9, 2024 13:48

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from f585966 to a15074b Compare December 11, 2024 12:08

jdgage approved these changes Dec 11, 2024

View reviewed changes

samyou-softwire added 3 commits December 12, 2024 11:11

PC-1463: make use of address cache on OS API flows

e0b2266

PC-1463: add verifying test

13bbcd8

PC-1463: mark fields used for journey calculation

5e8d838

to make their use distinct from user input fields

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from a15074b to 3898568 Compare December 12, 2024 11:11

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from 3898568 to 4ddd68b Compare December 13, 2024 11:02

samyou-softwire added 2 commits December 13, 2024 11:05

PC-1463: mark fields used as yes/no options

12492e0

PC-1463: add translations

0c27ae9

samyou-softwire force-pushed the PC-1463-improve-ux-for-no-address-found branch from 4ddd68b to 0c27ae9 Compare December 13, 2024 11:34

samyou-softwire merged commit fbb2380 into develop Dec 13, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PC-1463: Improve UX for when no address found #410

PC-1463: Improve UX for when no address found #410

samyou-softwire commented Dec 4, 2024 •

edited

Loading

samyou-softwire Dec 4, 2024

samyou-softwire Dec 5, 2024

jdgage left a comment

jdgage Dec 5, 2024

samyou-softwire Dec 6, 2024

jdgage left a comment

jdgage Dec 6, 2024

samyou-softwire commented Dec 9, 2024

samyou-softwire commented Dec 9, 2024

jdgage commented Dec 9, 2024

samyou-softwire commented Dec 10, 2024

jdgage commented Dec 10, 2024

samyou-softwire commented Dec 11, 2024

jdgage Dec 9, 2024

samyou-softwire Dec 12, 2024

jdgage Dec 12, 2024

samyou-softwire Dec 12, 2024

jdgage Dec 9, 2024

samyou-softwire Dec 12, 2024

jdgage commented Dec 11, 2024

jdgage commented Dec 13, 2024

		@@ -2032,41 +2031,6 @@ def test_on_epc_select_page_enter_manually_can_be_selected():
		assert page.has_one('h1:contains("What is the property\'s address?")')


		@unittest.mock.patch("help_to_heat.frontdoor.interface.EPCApi", MockNotFoundEPCApi)

PC-1463: Improve UX for when no address found #410

PC-1463: Improve UX for when no address found #410

Conversation

samyou-softwire commented Dec 4, 2024 • edited Loading

Description

Checklist

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdgage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdgage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samyou-softwire commented Dec 9, 2024

samyou-softwire commented Dec 9, 2024

jdgage commented Dec 9, 2024

samyou-softwire commented Dec 10, 2024

jdgage commented Dec 10, 2024

samyou-softwire commented Dec 11, 2024

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

jdgage commented Dec 11, 2024

jdgage commented Dec 13, 2024

samyou-softwire commented Dec 4, 2024 •

edited

Loading