-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LGA-3319: Redirect user to cla_public #52
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is mind blowing! I need more time to look at this but left some comments. Would be good to run through in detail if not done already, and record it as part of documentation.
class ExistingQuestionForm(QuestionForm): | ||
routing_logic = { | ||
"existing-choice": ExistingNextStep, | ||
"new-choice": NewQuestionForm # Add your new form here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this say MyQuestionForm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Yes, you're right!
@@ -0,0 +1,100 @@ | |||
{% extends "base.html" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have robot/nofollow tags here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should, however, I think we should do this as a general ticket rather than for this specific case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more questions only. Lot of these are just questions to help with my own learning so doesn't mean changes are needed. Probably worth running through in dev retro if time allows.
|
||
previous_answer = request.args.get("previous_answer") | ||
|
||
# TODO: Make an override of the GOV.UK WTForms radio field class to support divisors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need some help understanding what divisor is in this context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this content the divisor is the "Or" text in the radio button drop down.
https://design-system.service.gov.uk/components/radios/
See the "Radio items with a text divider" section.
|
||
@bp.get("/contact") | ||
def contact(): | ||
return redirect("127.0.0.1:5000/contact") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming it's Check contact page, maybe use env var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, this code wasn't meant to be here.
app/config/__init__.py
Outdated
@@ -21,3 +21,7 @@ class Config(object): | |||
SENTRY_DSN = os.environ.get("SENTRY_DSN") | |||
LANGUAGES = {"en": "English", "cy": "Welsh"} | |||
SERVICE_UNAVAILABLE = os.environ.get("MAINTENANCE_MODE", "False").lower() == "true" | |||
CLA_PUBLIC_URL = os.environ.get( | |||
"CLA_PUBLIC_URL", "https://staging.checklegalaid.service.gov.uk" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe default to production in case env var goes missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated this to prod. It would be good to come up with a consistent rule/ approach for this!
What does this pull request do?
Adds the category traversal system
The category traversal system is a finite state machine responsible for handling routing the user through the category diagnosis questions.
It is designed to
Whilst keeping the service stateless and without requiring session data to be stored by the user.
Redirect the user to CLA Public
When the user reaches the end of traversal they can be redirected to CLA Public either to:
A signed JWT payload will be sent to CLA Public containing:
CLA Public will set the user's session data as required before redirecting them to their intended destination, this is implemented in: ministryofjustice/cla_public#1279
Debug routing map
A page consisting of a user journey map has been added to the
/routing-map
route.This allows you to navigate to any point within the category diagnosis section of the service and indicates the result of the user journey.
You can filter the table by the resulting destination.
Any other changes that would benefit highlighting?
Increases the rate limiting to 5 per second. This is because the question pages are all using the same Flask route so it is easy for a user to hit this limit without trying to cause harm.
Checklist