Remove PfG scaffolding and simplify content items. #4656
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
Closed
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
52841cd to
53ab22b
Compare
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
4506ff2 to
2b057f2
Compare
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
2b057f2 to
bf07822
Compare
leenagupte
reviewed
Feb 24, 2025
spec/models/landing_page_spec.rb
Outdated
| ) | ||
| GdsApi::Response.new(http_response) | ||
| end | ||
| subject(:landing_page) { ContentItemFactory.build(landing_page_example) } |
There was a problem hiding this comment.
Why is ContentItemFactory.build being called here rather than described_class.new?
There was a problem hiding this comment.
Oh yeah, this is just a failure to follow all the way down (I was fixated on removing the helper, forgot this example could go even further). Thanks! fixed now.
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
bf07822 to
a357f2d
Compare
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
a357f2d to
8572c67
Compare
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
8572c67 to
02c9a5f
Compare
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
f05def0 to
e4b584e
Compare
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
e4b584e to
7c44657
Compare
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
7c44657 to
f1ff9c1
Compare
- add explicit before_action to set the content item once. Before this, the memoization on the last line would prevent building the content item once, but not querying the ContentItemLoader for it (this is largely fine because ContentItemLoader would cache calls to ContentStore, but we didn't need to invoke that code so many times) - add attr_reader so that calls to content_item still work. - for the moment, skip this in the landing_page_controller because of the scaffolding (which we'll be removing in a later commit).
- This meta-data about a content_item doesn't really need to be contained in it. It's only used in the content_items controller, so just save it there from the same response, and remove the delegation from the content_item. - rename the before_action, since it's now getting two things.
- We now expect landing pages to be built entirely from content items, just like all other content items, so remove the param that allows us to pass another hash here. At this point, tests for landing page will be failing if they depend on the old scaffolding.
- attachments code was only added for the Chart block in landing pages, which did not in fact make it into production. We can put it back later, but for the moment we're not using it so we can cut out the tests, plus one used and one unused factory.
- Pass only the hash from the Api response to the content_item factory. Now only hashes should be being passed to content_item, so we lose the content_item_hash attribute and just use content_item_response.
- we pull in the improved example from alphagov/publishing-api#3153, - this allows us to get rid of the setup hash, and stop stubbing the additional content path, since we want to get rid of that. - we can remove the one block factory, since we're always loading data from the content store now, so that test and the previous are merged into one.
- We now no longer want anything to render when a content item doesn't exist, so we remove that test and leave it to standard behaviour. - We've removed the attachment support from ContentItem for the time being, so remove that mock data.
- Using the same example as above, we can remove the mocked content item and the additional content path. - rename content_item to landing_page_example to match previous tests.
- landing page content items are now fully available from content store (in practise) and publishing-api schemas (during tests), so this scaffolding is now no longer necessary. - we no longer need to skip the before action.
KludgeKML
force-pushed
the
remove-pfg-scaffolding-2
branch
from
f1ff9c1 to
167df9b
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.
What
Remove PfG scaffolding code
Why
PfG is now live, so we don't need the scaffolding that was necessary to test it before go-live. The ability to partially load landing page items from the local path has been made obsolete by #4366
Relies on / Cannot be merged before: alphagov/publishing-api#3153