Re-organise routing in preparation for Maths & Physics - #597
Merged
Conversation
These pages are for claimants primarily, they are not really relevant to internal service operator users. Besides which we've made them policy-aware, so soon it won't be clear which version they should even link to when Maths & Physics is introduced.
A lot of the previous Maths & Physics preparation work was leading to this moment where we can remove the default policy in the routing and be explicit about the current active `:policy` in the app code. Now it will be possible to introduce additional policy routing without breaking any of the existing views, controllers or spec code. To keep the rendering of the maintenace page working, current_policy_routing_name had to be overriden in StaticPagesController as the catch-all route for maintenance mode won't include a value for :policy in the params. Note that at the moment there is little to distinguish the specs that have to be coupled to the policy, for example spec/features/student_loans_claim_spec.rb, and those that shoud be more generic as they cover behaviour that is policy-agnostic, such as spec/requests/claims_spec.rb. At some point very soon these specs should be rationalised to make it obvious which are testing behaviour specific to a given policy, and which are policy-agnostic.
tekin
force-pushed
the
maths-and-physics-routing
branch
2 times, most recently
from
November 7, 2019 15:20
795b7e2 to
e4c34a1
Compare
mec
reviewed
Nov 7, 2019
mec
left a comment
mec
left a comment
Contributor
There was a problem hiding this comment.
All looks good. I just had one question…
mec
approved these changes
Nov 7, 2019
This makes it such that introducing routing for Maths & Physics will simply be a case of defining a `MathsAndPhysics::SlugSequence` and adding the `MathsAndPhysics` module to the array of `policies`. I've made use of a routing constraint object here as I struggled to get the routing to work just using the default constraints and defaults when a second policy was introduced.
Because the routes for the verify actions are not namespaced to a particular policy it won't be possible for the controller to redirect users to a start page if they visit the URLs without an active claim. We can redirect these requests to the root, which will be handled appropriately, either as it is now by redirecting to the Student Loans start page, or in the future when we introduce a page for lost users pointing at both policies once Maths & Physics is introduced.
This gives us a global place where we can perform policy-based lookups.
To now PartOfClaimJourney was coupled to the StudentLoans policy. This updates it so that it will correctly initialise new claims and redirect unstarted claimants according to the actual policy defined by the routing parameters. Note that current_claim needs to be able to behave even when there is no `params[:policy]` because Verify::AuthenticationsController is not namespaced by policy.
This allows us to keep each policy's views separate from each other, whilst also sharing those that are common between them. Because current_policy_routing_name is now being called in a before_action, we need to explicitly unset the instance variable that stores current_claim as it gets set when `current_policy_routing_name` is called.
tekin
force-pushed
the
maths-and-physics-routing
branch
from
November 7, 2019 16:29
e4c34a1 to
6509d7c
Compare
pezholio
approved these changes
Nov 7, 2019
pezholio
left a comment
pezholio
left a comment
Contributor
There was a problem hiding this comment.
Looks really good - just one suggestion on the routes, but not a blocker
| resources :claims, only: [:show, :update], param: :slug, path: "/" | ||
| # Used to constrain claim journey routing so only slugs | ||
| # that are part of a policy’s slug sequence are routed. | ||
| class RestrictToSequenceSlugs |
Contributor
There was a problem hiding this comment.
I'm a bit iffy about having the class inline in the routing block. Would it be OK to have it up top, so it makes the routes easier to read?
Contributor
Author
There was a problem hiding this comment.
Personally I like having the class near to where it is being used as it places it closer to the context in which it needs to be understood.
Contributor
There was a problem hiding this comment.
No drama. Happy for it to stay where it is in that case 👍
robbiepaul
approved these changes
Nov 7, 2019
robbiepaul
left a comment
robbiepaul
left a comment
Contributor
There was a problem hiding this comment.
Some pretty nifty changes here. I learnt about prepend_view_path today 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should be the last PR laying the groundwork for introducing the Maths & Physics policy to the service. Several changes have been made to make it possible to drop in additional policies:
Policiesso that we have a global list of available policies to work withrouting.rbnow generates routes for multiple policies, based on what is defined inPolicies