Merged
Conversation
SamJamCul
force-pushed
the
forms-pages-via-repository
branch
2 times, most recently
from
8703e14 to
6611b22
Compare
lfdebrux
approved these changes
Jan 23, 2025
Contributor
lfdebrux
left a comment
lfdebrux
left a comment
There was a problem hiding this comment.
This works for me locally, thanks for putting it together.
Adds a pages method to the FormRepository service. This allows accessing a form's pages indirectly through the repository, rather than directly through the model - which will make the API-admin switching work easier
Replaces any calls to the Form model for pages with a FormRepository.pages method call instead.
SamJamCul
force-pushed
the
forms-pages-via-repository
branch
from
6611b22 to
00d02d4
Compare
|
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
2 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.
What problem does this pull request solve?
Trello card: https://trello.com/c/XJVoEeTw/2075-add-form-pages-to-form-repository
Adds a FormRepository
pagesmethod, which requests a form's pages via the form model. This is then used throughout the app to replace any requests for form pages that we're currently making.Things to consider when reviewing
It was a little fiddly to find all the places we're getting form pages, and the way activeresource works makes testing that we're actually stubbing out a request is also quite fiddly.
There may be some tests where technically we could have removed the stubbing, and they still would have worked, as the way we factory build forms means they come with pages already, so a call for
.pageswouldn't result in an API request. I've tried to still replace any possible circumstances where we could have made an API request with the FormRepository version.I'm also not sure if there'll be a performance hit with some of this, mainly around the conditions stuff. There are a few places where we're looking at a form's pages, and if each of those hits the API, we might (big might) see some slowdown. I've tried testing locally which was fine, but I'm very tempted to test on dev to see if it's fine.