Skip to content
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

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Conversation

Nitemaeric
Copy link
Contributor

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 and claim-funding-for-general-mentors.service.gov.uk.

  • School Placements have confirmed that "Placements" is their desired top-level namespace.
  • Track & Pay have confirmed that "Claims" is their desired top-level namespace.

Changes proposed in this pull request

  • Add 2 routing files for the separate services, config/routes/claims.rb and config/routes/placements.rb
  • Update app/views/layouts/application.html.erb to use service name header based on current_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:

127.0.0.1 claims.localhost
127.0.0.1 placements.localhost

Link to Trello cards

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist
  • API release notes have been updated if necessary
  • If it adds a significant user-facing change, is it documented in the CHANGELOG?
  • Required environment variables have been updated or added to the Azure KeyVault

Copy link
Contributor

@CatalinVoineag CatalinVoineag left a 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

.env.example Outdated Show resolved Hide resolved
app/controllers/claims/pages_controller.rb Show resolved Hide resolved
@@ -0,0 +1,45 @@
require "rails_helper"
Copy link
Contributor

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?

Copy link
Contributor

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? ☝🏻

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@elceebee elceebee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, I got it to work locally as expected, see images. Good to see some tests as well so we can start establishing some patterns / good practice for that.

I agree that env vars values should go in the .env.example file unless they are secret. Just helps a new dev get started more quickly.

image image

@Nitemaeric Nitemaeric requested a review from elceebee December 11, 2023 16:17
@Nitemaeric
Copy link
Contributor Author

Turns out @CatalinVoineag caught a bug! Please see the changes @elceebee.

Copy link
Contributor

@ollietreend ollietreend left a 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.

@Nitemaeric
Copy link
Contributor Author

Turns out I was setting ENV["..._HOST"] to http://... :/

@@ -2,7 +2,7 @@

RSpec.describe "Pages", type: :request do
describe "GET /home" do
it "returns http success" do
xit "returns http success" do
Copy link
Contributor Author

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.

Comment on lines 20 to 32
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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Nitemaeric Nitemaeric requested a review from SocFoot December 14, 2023 09:59
This allows us to separate routing between the School Placements service and the Track & Pay service.
@Nitemaeric Nitemaeric force-pushed the host-routing-constraint branch from f6af1dd to c453198 Compare December 14, 2023 10:12
Copy link
Contributor

@elceebee elceebee left a 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.
@Nitemaeric Nitemaeric force-pushed the host-routing-constraint branch from d4cb351 to 34a079c Compare December 14, 2023 12:42
@Nitemaeric Nitemaeric requested a review from elceebee December 14, 2023 12:43
@Nitemaeric
Copy link
Contributor Author

It appears this breaks logging in as a persona -- root_path is not defined.

no_root_path.mov

😅 Yup. It's fixed now :)

@Nitemaeric Nitemaeric merged commit 0c33b6b into main Dec 14, 2023
4 checks passed
@Nitemaeric Nitemaeric deleted the host-routing-constraint branch December 14, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants