Share view context with presenters to access helpers #1752
Merged
Conversation
This file contains 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
laurentqro
approved these changes
May 1, 2020
This adds a new method in, create_presenter, which has default test arguments set for presenters. This then updates all of the presenter tests so that they all use this helper. The reason for this change is that I intend to add a third argument soon to presenter classes which would mean an increase in test verbosity. I also think this improves consistency of these tests.
This shortens this variable name and I believe it still signifies the same intent as the previous name. The optional nature of it is also removed since every time this class is initialised in app this attribute is set, I imagine this was set as optional for test verbosity but this is no longer an issue. This also fixes a couple of ivars that weren't necessary due to methods existing.
This changes the way presenters work in this application so that they have the equivalent of a view object available to call helpers on. This removes the need to import specific helpers into individual presenters. The reason for adding this in is to fix a number of subtle issues that can occur when it comes to generating URLs without any request context. This problem affected me when I went to looking at adjusting the asset host in order to make progress towards RFC-115 [1]. I found that we'd experience different behaviours on asset url methods depending on the GOVUK_ASSET_ROOT env var. When this env var is not set, as per [2], any calls for an asset_url would return a path rather than URL like we requested; when a GOVUK_ASSET_ROOT env var is set as a hostname without protocol, as per [3], any calls for asset urls would raise a "undefined method `protocol` for nil:NilClass" exception at [4]. These problems don't affect production since GOVUK_ASSET_ROOT is set as a full URL. Anyway, I came to the conclusion that these presenters are effectively just extensions of the view - evidenced by the high proliferation of view helpers embedded - and I felt that it'd be easier to have a view_context object available to call any view helpers we might want. This helps us out when it comes to rendering asset_urls as this view context object has the request context available and can determine the URL based on the request. Were we to use Rails route helpers for navigating the app we'd also be able to drop the various usages of Plek.current.website_root and instead rely on view context to generate URLs. [1]: https://github.com/alphagov/govuk-rfcs/blob/master/rfc-115-enabling-http2-on-govuk.md [2]: https://github.com/alphagov/govuk-docker/blob/4b40e479799ea304709276efb29c49898a8b8c9f/projects/government-frontend/docker-compose.yml#L48-L57 [3]: https://github.com/alphagov/govuk-docker/blob/4b40e479799ea304709276efb29c49898a8b8c9f/projects/government-frontend/docker-compose.yml#L20-L30 [4]: https://github.com/rails/rails/blob/92d03850f3bb4c44103d0b06b43e14d6e270e646/actionview/lib/action_view/helpers/asset_url_helper.rb#L303
kevindew
force-pushed
the
use-contextual-helpers
branch
from
a75d7c4 to
c57012c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 changes the way presenters work in this application so that they
have the equivalent of a view object available to call helpers on. This
removes the need to import specific helpers into individual presenters.
The reason for adding this in is to fix a number of subtle issues that
can occur when it comes to generating URLs without any request
context. This problem affected me when I went to looking at adjusting
the asset host in order to make progress towards RFC-115 1.
I found that we'd experience different behaviours on asset url methods
depending on the GOVUK_ASSET_ROOT env var. When this env var is not set,
as per 2, any calls for an asset_url would return a path rather than
URL like we requested; when a GOVUK_ASSET_ROOT env var is set as a
hostname without protocol, as per 3, any calls for asset urls would
raise a "undefined method
protocolfor nil:NilClass" exception at 4.These problems don't affect production since GOVUK_ASSET_ROOT is set as
a full URL.
Anyway, I came to the conclusion that these presenters are effectively
just extensions of the view - evidenced by the high proliferation of
view helpers embedded - and I felt that it'd be easier to have a
view_context object available to call any view helpers we might want.
This helps us out when it comes to rendering asset_urls as this view
context object has the request context available and can determine the
URL based on the request.
Were we to use Rails route helpers for navigating the app we'd also be
able to drop the various usages of Plek.current.website_root and instead
rely on view context to generate URLs.