-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add host-based routing constraints #13
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.
Can you include a video of how this should work? I couldn't get it working on my local. It doesn't hit the individual placements and claims controllers and I'm not sure if this is correct, depending on what we want the host to be I guess.
Can jump on a call if it's easier
@@ -0,0 +1,45 @@ | |||
require "rails_helper" |
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.
Would it be useful to have something like this to test the claims and placements index pages?
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.
Do you still think there is value in this? ☝🏻
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.
+1 this covers the translations are working correctly, but it doesn't cover the router host-based config.
Is there some way we can set the 'host' so we cover the router's behaviour too?
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'll add in a case to check the page content.
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.
Added a temp case while we don't have the login page setup.
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.
Turns out @CatalinVoineag caught a bug! Please see the changes @elceebee. |
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.
@Nitemaeric I get three failures when running the test suite locally.
$ rake
[...]
Failures:
1) Pages GET /home returns http success
Failure/Error: get "/"
ActionController::RoutingError:
No route matches [GET] "/"
# ./spec/requests/pages_spec.rb:6:in `block (3 levels) in <main>'
2) Home Page User visits the claims homepage
Failure/Error: visit "/"
ActionController::RoutingError:
No route matches [GET] "/"
# ./spec/system/home_page_spec.rb:31:in `and_i_am_on_the_start_page'
# ./spec/system/home_page_spec.rb:6:in `block (2 levels) in <main>'
3) Home Page User visits the placements homepage
Failure/Error: visit "/"
ActionController::RoutingError:
No route matches [GET] "/"
# ./spec/system/home_page_spec.rb:31:in `and_i_am_on_the_start_page'
# ./spec/system/home_page_spec.rb:12:in `block (2 levels) in <main>'
I know we don't have CI hooked up to the repo yet, but we should try and avoid committing failing tests to main.
Turns out I was setting |
@@ -2,7 +2,7 @@ | |||
|
|||
RSpec.describe "Pages", type: :request do | |||
describe "GET /home" do | |||
it "returns http success" do | |||
xit "returns http success" do |
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.
Skipping this test for now. I'm not sure if we'll need to drop it entirely. Might be the case.
spec/system/home_page_spec.rb
Outdated
def given_i_am_on_the_claims_site | ||
ENV["CLAIMS_HOST"] = "claims.localhost" | ||
Capybara.app_host = "http://#{ENV["CLAIMS_HOST"]}" | ||
end | ||
|
||
def given_i_am_on_the_placements_site | ||
ENV["PLACEMENTS_HOST"] = "placements.localhost" | ||
Capybara.app_host = "http://#{ENV["PLACEMENTS_HOST"]}" | ||
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.
1. Redefining environment variable
Do we need to set ENV["CLAIMS_HOST"] =
and ENV["PLACEMENTS_HOST"] =
here?
I see you've updated the .env.example
file too – so I'd expect these environment variables to already be present, without needing to re-define them here.
2. Polluting global scope
I think we need a teardown to reset Capybara.app_host
to nil
again. Right now it looks like this will leak out and persist in the global scope.
We may want to consider a global setup/teardown and helper methods to set & reset the Capybara.app_host
value, because this is likely going to be something we use across most feature specs.
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.
Do we need to set ENV["CLAIMS_HOST"] = and ENV["PLACEMENTS_HOST"] = here?
I see you've updated the .env.example file too – so I'd expect these environment variables to already be present, without needing to re-define them here.
.env.example
isn't a part of the dotenv
spec. It's only there for reference for copying over values to the .env
or .env.local
file which is in the .gitignore
. To have these in the test environment, we'd have to add them to a .env.test
file.
I think we need a teardown to reset Capybara.app_host to nil again.
Ah yes, 👍. Updated.
We may want to consider a global setup/teardown and helper methods to set & reset the Capybara.app_host value, because this is likely going to be something we use across most feature specs.
I'll leave it minimal for now. Once we see it again, let's extract the pattern out.
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.
You may recall that I raised the .env.test
issue a couple days ago. I sorted it locally by renaming my .env.local
file to just .env
. This makes it accessible to both the local and test environments. I think eventually we will want a .env.test
file as there will be environment that is not shared across test and local, but this works for now.
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.
Ah – I've been using a .env
file all along. I didn't realise others had named theirs .env.local
. 🙈
Yep – all makes sense. It would definitely be nice to have environment variables for tests defined in one place, rather than manually setting them within the test scenarios.
But all good if you'd rather keep it this way for now.
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.
With patching the auth specs, I've added the .env.test
file into the fray.
This allows us to separate routing between the School Placements service and the Track & Pay service.
f6af1dd
to
c453198
Compare
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 appears this breaks logging in as a persona -- root_path
is not defined.
no_root_path.mov
Move host ENV setup into a `.env.test` file for more simple setup. Use the placements service for auth specs.
d4cb351
to
34a079c
Compare
😅 Yup. It's fixed now :) |
Context
We want to split routing between the "School Placements" and "Track & Pay" services within the Rails application. In production, these URLs are likely to be
manage-school-placements.service.gov.uk
andclaim-funding-for-general-mentors.service.gov.uk
.Changes proposed in this pull request
config/routes/claims.rb
andconfig/routes/placements.rb
app/views/layouts/application.html.erb
to use service name header based oncurrent_service
helper.Guidance to review
By updating your local
/etc/hosts
file with the following lines, you can access both services without running a proxy server:Link to Trello cards
Things to check