Local content item support #4366
Conversation
KludgeKML
force-pushed
the
local-content-item-support
branch
from
f68df37
to
8c2506a
Compare
KludgeKML
force-pushed
the
local-content-item-support
branch
from
8c2506a
to
b1cffa4
Compare
KludgeKML
force-pushed
the
local-content-item-support
branch
from
b1cffa4
to
fdc8e03
Compare
KludgeKML
force-pushed
the
local-content-item-support
branch
from
e1add66
to
e0d8c80
Compare
There was a problem hiding this comment.
I had a bit more time to review, so had a closer look at the ContentItemLoader class and added a few inline suggestions for refactoring it.
There's also a request for some tests around the scaffolding if this is going to become a permanent feature.
There's nothing major though and the overall approach is great 🎉
lib/content_item_loader.rb
Outdated
| end | ||
|
|
||
| def local_file?(base_path) | ||
| File.exist?(local_json_filename(base_path)) || File.exist?(local_yaml_filename(base_path)) |
There was a problem hiding this comment.
It occurs to me that you're doing this check twice. Once here and once in load_local_file.
It probably means that this class could do with a refactor.
Maybe split out the if clause in the load method and start from there?
e.g.
def load(base_path)
cache[base_path] ||= if use_local_file? && yaml_file?(base_path)
load_yaml_file(base_path)
elsif use_local_file? && json_file?(base_path)
load_json_file(base_path)
else
begin
GdsApi.content_store.content_item(base_path)
rescue GdsApi::HTTPErrorResponse, GdsApi::InvalidUrl => e
e
end
end
endThen you wouldn't need to redo the checks in the load_file methods.
It'd also allow you to split up the load_local_file method into the bits that load the file and create the gds-api response
def load_yaml_file(base_path)
file_name = yaml_filename(base_path)
Rails.logger.debug("Loading content item #{base_path} from #{file_name}")
contents = YAML.load(File.read(file_name)).to_json
gds_api_response(contents)
end
def load_json_file(base_path)
file_name = json_filename(base_path)
Rails.logger.debug("Loading content item #{base_path} from #{file_name}")
contents = File.read(file_name)
gds_api_response(contents)
end
def gds_api_response(body)
GdsApi::Response.new(OpenStruct.new(code: 200, body:, headers: { cache_control: "max-age=0, public", expires: "" }))
endI think also that limiting the use of local in the method names will make them a bit shorter and easier to read.
There was a problem hiding this comment.
All of the methods apart from load should be made private as they're not being used or tested elsewhere.
| # SCAFFOLDING: can be removed when basic content items are available | ||
| # from content-store | ||
| def old_scaffolding_content_item | ||
| result = ContentItemLoader.load(request.path) |
There was a problem hiding this comment.
It's great that you've fixed this. I think it would be good to add a test for this scaffolding too.
There was a problem hiding this comment.
The good news is that the existing request tests for landing_page already cover the old scaffolding (tests written... checks blame ... one @leenagupte)
There was a problem hiding this comment.
They can't have been thorough enough because nothing caught the attempted call to ContentItemLoaderGdsApi.content_store.content_item(request.path).to_h which doesn't exist!
KludgeKML
force-pushed
the
local-content-item-support
branch
from
42eb998
to
e9c8461
Compare
KludgeKML
force-pushed
the
local-content-item-support
branch
from
e9c8461
to
de1459c
Compare
KludgeKML
force-pushed
the
local-content-item-support
branch
from
de1459c
to
9d149f2
Compare
| context "with a local JSON file" do | ||
| let!(:item_request) { stub_content_store_has_item("/my-json-item") } | ||
|
|
||
| it "loads from that" do |
There was a problem hiding this comment.
Not a blocker, but this isn't a great test name!
It should be probably be something like `it "loads content from the json file not content store". Or something.
| context "with a local YAML file" do | ||
| let!(:item_request) { stub_content_store_has_item("/my-yaml-item") } | ||
|
|
||
| it "loads from that file" do |
There was a problem hiding this comment.
Ditto here about the test name, but again, not a blocker.
- ContentItemLoader replaces direct calls to GdsApi.content_store.content_item, (of which there are only 4 in the rest of the codebase), and provides two functions. - First, it centralises caching of calls, so that that can be removed from the FormatRoutingConstraint classes and using request.env to save those values can be simplified. - Second, if the ALLOW_LOCAL_CONTENT_ITEM_OVERRIDE env var is set true, it allows loading content items from a file in /lib/data/local-content-items, which gives developers an extra option for local development or preview apps when working with content types that have not crystalised yet or where publishing support is not yet present. It should not be used in production. - The load method is slightly odd in that it will either return an API response or an exception (note: return the exception, not raise it!). This allows us to cache errors in a similar way to the way the routing constraints used to. It might be there are better ways to handle this, but for the moment this is a minimal change to maintain the current behaviour.
- Because the cache is at class level, it's very aggressive and would otherwise interfere with what people would normally expect about tests (ie that in two unrelated tests you could use the same slug to point to different things). So we default here to just clearing the cache before each test. - Do it manually in shared tests, which have their own setup that might conflict with "before" blocks in the system specs that call them.
- Now that ContentItemLoader is handling the caching, we can remove that layer of code / responsibility from the constraints. - ContentItemLoader either returns the adapter response or the error that it caught, so querying for those classes gives us a "was it or wasn't it an error" check. - We also simplify the spec tests slightly to use more idiomatic RSpec.
- Now that ContentItemLoader handles caching we can use that rather than the stuff that the format constraints were putting into the request env. - LandingPageController's scaffolding is still needed for the moment, but we can remove it soon (since part of this project is about replacing that scaffolding with a more generally useful one). - Content items in controllers are now always loaded from `content_item_path`, which defaults to the request path, and is overridden if necessary for items with multiple paths on one content item.
KludgeKML
force-pushed
the
local-content-item-support
branch
from
9d149f2
to
41e22f2
Compare
What
Adds more structured support for loading content items from the local machine. By setting an environment variable and putting a content item in a particular directory, it will be loaded from that file instead of from a call to the content store.
Why
There's currently a restricted version of this code working for Landing Pages (it only loads the blocks from the fixture, but includes fake values for the rest of the content item). This scaffolding has proved useful during Landing Pages development, but it's limited in scope and only works for Landing Pages. By replacing it with a more robust option, we can improve developer experience in the absence of a local version of content store, and make it easier to demo experimental content items on Heroku.
https://trello.com/c/FytCkByy/383-improve-contentitem-offline-loading
How
We add a new singleton class (ContentItemLoader) that is responsible for calls to GdsApi.content_store.get_content - this allows us to have a cache of items (simplifying the current Format Constraint code, which caches responses in the request env field), and to have a single point where those calls can be intercepted and swapped out for local code if the required environment variable is set. Setting ALLOW_LOCAL_CONTENT_ITEM_OVERRIDE to true will cause the loader to look in /config/local-content-items/path/to/the/item for any call to the content store (falling back to an actual call if the file isn't present).
See Also: