Add ADR for storing responses in the query string #5212

theseanything · 2021-03-01T18:31:13Z

View rendered 🖼️

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

ChrisBAshton

Sounds good to me 👍 I particularly like that it will unify the controller logic (benefits devs), and that we can be smarter about letting users edit their early responses (better user experience).

docs/arch/002-store-responses-in-query-string.md

kevindew

This sounds a good proposal for me 👍 there's a few areas I've got extra questions and opinions.

docs/arch/002-store-responses-in-query-string.md

kevindew · 2021-03-02T19:43:18Z

docs/arch/002-store-responses-in-query-string.md

+1. Rename `SessionAnswersController` to `FlowController` (as will be used to handle all flows)
+1. Change `use_session` configuration option in flow definition to specify different response store types. e.g. `response_store :session`
+1. Add functionality to store responses in query string
+1. Migrate existing flows incrementally to use query string by specifying config option e.g. `response_store :query_string`


Will there be any backwards compatibility? i.e. once say https://www.gov.uk/marriage-abroad/y/afghanistan/uk/partner_british/opposite_sex is migrated would this 404? redirect me to start of a results page? back to start of Smart Answer?

Good catch. We need to cover those who are in the middle of doing a flow, and old links that might be saved.

Feels like there's scale of effort vs disruption for users and depends on the amount of traffic (how many user's would be effected).

Most effort, minimal disruption:
Add logic to translate old urls into new query string based ones.

Medium effort, medium disruption:
Redirect all old URLs to the start page.

Low effort, high disruption:
Drop the old routes for old URLs and 404.

Safest approach would be to:

Initially keep the existing controller to handle old URL requests (we can distinguish them by the extra /y/ vs /s/ segment after the flow name).

Add routes for both URL types in the content item (for router).

Update the start page to point to the query string based flow.

Wait a period of time to let previous active users finish their current flow.

Keep an eye on traffic to the old URLs

Then decide on level of effort thats needed?

My gut feeling is we don't actually have that many people save URLs or link directly to results, and that we'd probably be fine to just redirect to the start page.

Yeah I think it's fine to have something that is intermediary and then remove. I think that could be neater too as we could set redirects in Publishing API for all the /y URL's at the point of removal and then not need any /y handling code in this app.

How do you feel about using this as a chance to change /s/ and /y/ to something more understandable? a quick idea I had is /flow/ ? Just a thought given it's going to be a bit disruptive anyway.

Yup agree - should try and add the redirect in Publishing API. Would let us really clean everything up.

I think that's a good idea about renaming /s/ - I'm slightly worried about adding extra work to implement this change - (would have to migrating the existing session based answer (find-coronavirus-support) as well). I also wonder if it's worth, outside this ADR, thinking about whether we can remove it all together.

Great re: redirect

I think that's a good idea about renaming /s/ - I'm slightly worried about adding extra work to implement this change - (would have to migrating the existing session based answer (find-coronavirus-support) as well). I also wonder if it's worth, outside this ADR, thinking about whether we can remove it all together.

Yeah getting rid altogether sounds ideal but I agree that's out of the scope of this.

I think the renaming should be in the scope of this as /s/ will leave a bit of a smell behind. The s stands for session as I recall? Since every other Smart Answer needs migrating anyway it'd seem the extra work is very low since it'd be a different application of the same process. Only tricky thing seems to be agreeing on what the rename should be 😁

Sure, you are right it'll less work now than later. /flow/ does make sense - it's short and descriptive. Struggling to think of any alternatives, maybe /nodes/ (but prefer flow)?

Another thing is to make sure it's not confusing users who won't be familiar with these concepts.

Sounds good - yeah it seems a step up from /y/ anyway for confusion.

I've added a section talking about the prerequisite work to update /s/ to /flow/ - including suggested step of implementation.

docs/arch/002-store-responses-in-query-string.md

gclssvglx

Hi @theseanything - looks great. Just a couple of suggestions 👍

docs/arch/002-store-responses-in-query-string.md

This updates the background to give more context over how smart answers resolves the current node with responses in the URL path.

theseanything · 2021-03-05T12:42:27Z

@kevindew I hope I've been able to clarify your questions. Let me know if there is anything else blocking this from going ahead.

gclssvglx

Fantastic work and an excellent description of what and why this change should be made.

kevindew

Sounds good to me, nice one @theseanything

theseanything · 2021-03-09T09:27:20Z

Thanks everyone for you comments and reviews 🎉

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 1, 2021 18:31 Inactive

theseanything force-pushed the adr-to-use-query-string branch from a00301d to 4b3f5f8 Compare March 1, 2021 18:48

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 1, 2021 18:48 Inactive

ChrisBAshton approved these changes Mar 2, 2021

View reviewed changes

docs/arch/002-store-responses-in-query-string.md Outdated Show resolved Hide resolved

theseanything force-pushed the adr-to-use-query-string branch from 4b3f5f8 to f32980a Compare March 2, 2021 11:39

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 2, 2021 11:39 Inactive

Add ADR for storing responses in the query string

ba8e35e

theseanything force-pushed the adr-to-use-query-string branch from f32980a to ba8e35e Compare March 2, 2021 11:41

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 2, 2021 11:42 Inactive

kevindew reviewed Mar 3, 2021

View reviewed changes

gclssvglx reviewed Mar 3, 2021

View reviewed changes

docs/arch/002-store-responses-in-query-string.md Show resolved Hide resolved

docs/arch/002-store-responses-in-query-string.md Outdated Show resolved Hide resolved

docs/arch/002-store-responses-in-query-string.md Show resolved Hide resolved

Clarify URL structure and invalid parameter behavior

cc00778

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 3, 2021 15:36 Inactive

Clarify how Smart Answers store repsonses in URL path

e6cd8f9

This updates the background to give more context over how smart answers resolves the current node with responses in the URL path.

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 3, 2021 15:54 Inactive

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 3, 2021 16:04 Inactive

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 5, 2021 10:47 Inactive

theseanything mentioned this pull request Mar 5, 2021

Improve organisation and naming of controllers and methods #5225

Open

Add renaming of prefix routes to /flow/

e8abb3d

theseanything force-pushed the adr-to-use-query-string branch from 0f371b8 to e8abb3d Compare March 5, 2021 15:41

bevanloon temporarily deployed to smart-answer-adr-to-use-ia9h7f March 5, 2021 15:42 Inactive

gclssvglx approved these changes Mar 8, 2021

View reviewed changes

kevindew approved these changes Mar 8, 2021

View reviewed changes

theseanything merged commit 158391f into master Mar 9, 2021

theseanything deleted the adr-to-use-query-string branch March 9, 2021 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ADR for storing responses in the query string #5212

Add ADR for storing responses in the query string #5212

theseanything commented Mar 1, 2021 •

edited

ChrisBAshton left a comment

kevindew left a comment

kevindew Mar 2, 2021

theseanything Mar 3, 2021 •

edited

kevindew Mar 3, 2021

theseanything Mar 4, 2021

kevindew Mar 4, 2021

theseanything Mar 4, 2021 •

edited

kevindew Mar 4, 2021

theseanything Mar 5, 2021

gclssvglx left a comment •

edited

theseanything commented Mar 5, 2021

gclssvglx left a comment

kevindew left a comment

theseanything commented Mar 9, 2021

Add ADR for storing responses in the query string #5212

Add ADR for storing responses in the query string #5212

Conversation

theseanything commented Mar 1, 2021 • edited

ChrisBAshton left a comment

Choose a reason for hiding this comment

kevindew left a comment

Choose a reason for hiding this comment

kevindew Mar 2, 2021

Choose a reason for hiding this comment

theseanything Mar 3, 2021 • edited

Choose a reason for hiding this comment

kevindew Mar 3, 2021

Choose a reason for hiding this comment

theseanything Mar 4, 2021

Choose a reason for hiding this comment

kevindew Mar 4, 2021

Choose a reason for hiding this comment

theseanything Mar 4, 2021 • edited

Choose a reason for hiding this comment

kevindew Mar 4, 2021

Choose a reason for hiding this comment

theseanything Mar 5, 2021

Choose a reason for hiding this comment

gclssvglx left a comment • edited

Choose a reason for hiding this comment

theseanything commented Mar 5, 2021

gclssvglx left a comment

Choose a reason for hiding this comment

kevindew left a comment

Choose a reason for hiding this comment

theseanything commented Mar 9, 2021

theseanything commented Mar 1, 2021 •

edited

theseanything Mar 3, 2021 •

edited

theseanything Mar 4, 2021 •

edited

gclssvglx left a comment •

edited