-
Notifications
You must be signed in to change notification settings - Fork 1
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
surveys: add a handler for the URL we sent in the letters #152
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.
Concept is 👍🏻
@@ -142,7 +142,11 @@ | |||
end | |||
end | |||
|
|||
Given('I see the summary {string} with') do |summary, table| | |||
When('I open a survey link for UPRN {int}') do |uprn| | |||
visit root_path + "?uprn=#{uprn}" |
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.
Use visit "/?uprn=#{uprn}"
- I prefer to avoid url helpers in tests so that bugs in routing don't get masked.
Scenario: Alex opens a survey link | ||
Given a building exists with UPRN 777 | ||
When I open a survey link for UPRN 777 | ||
Then the page contains "Your details" |
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.
Add a scenario for failure as well
</div> | ||
</div> | ||
</div> | ||
<% end %> |
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.
Not sure this is the right component here - should probably use a notification banner:
app/controllers/pages_controller.rb
Outdated
building = Building.find_by(uprn: params[:uprn]) | ||
|
||
if building.nil? | ||
redirect_to root_path, alert: I18n.t("errors.unknown_uprn", uprn: params[:uprn]) |
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.
Could we not use flash.now
rather than redirecting?
app/controllers/pages_controller.rb
Outdated
|
||
@survey = Survey::Record.find_or_create_by( | ||
building_id: building.id, | ||
session_id: session.id.to_s, |
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.
Is the to_s
necessary?
app/controllers/pages_controller.rb
Outdated
return | ||
end | ||
|
||
@survey = Survey::Record.find_or_create_by( |
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.
We can create the record through the building and avoid having to set building_id
:
begin
survey = building.find_or_create_by!(session_id: session.id)
rescue ActiveRecord::RecordNotUnique
retry
end
redirect_to goto_url("ownership")
The rescue
is to handle any race conditions.
app/controllers/pages_controller.rb
Outdated
stage: "ownership" | ||
) | ||
|
||
redirect_to survey_path(@survey, format: "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.
Not sure why the format
is needed but we can use goto_url
instead.
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 was getting an UnknownFormatError but I can't seem to reproduce that anymore.
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.
it happens in the Cucumber tests, presumably because it's a headless browser?
6b09d86
to
5dbd443
Compare
5dbd443
to
426760a
Compare
426760a
to
2189f9d
Compare
No description provided.