Refactor ContentBlockEdition create to accepted nested document attrs #9271
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
tahb
commented
Jul 16, 2024
lib/engines/content_object_store/app/models/content_object_store/content_block_edition.rb
Outdated
Show resolved
Hide resolved
tahb
force-pushed
the
content-modelling/refactor-content-block-param-handling
branch
2 times, most recently
from
125b375
to
a0d25cf
Compare
tahb
commented
Jul 16, 2024
lib/engines/content_object_store/test/unit/app/models/content_block_edition_test.rb
Show resolved
Hide resolved
|
Looks good - I totally forgot about |
tahb
added a commit
that referenced
this pull request
Jul 18, 2024
Following a discussion[1] we want to clarify what we expect this concern to contain. We want this to be more than identification, we want it to be everything that connects an edition to a document. This does diverge us from the main Whitehall we were referencing which may make this harder to merge back together later. But for now the short term benefit of the team knowing how to use it clearly takes priority. [1] #9271 (review)
tahb
force-pushed
the
content-modelling/refactor-content-block-param-handling
branch
from
a0d25cf
to
d45cf66
Compare
|
Okay folks after looking at that cucumber test I had to work on customising the error messages. That's now done with help from @pezholio ❤️ Whilst I look at this rebase it's ready for another review please. I'm conscious the longer this type of change stays in progress the many more conflicts we'll get. cc @Harriethw |
tahb
force-pushed
the
content-modelling/refactor-content-block-param-handling
branch
from
d45cf66
to
ad57e4f
Compare
tahb
added a commit
that referenced
this pull request
Jul 18, 2024
Following a discussion[1] we want to clarify what we expect this concern to contain. We want this to be more than identification, we want it to be everything that connects an edition to a document. This does diverge us from the main Whitehall we were referencing which may make this harder to merge back together later. But for now the short term benefit of the team knowing how to use it clearly takes priority. [1] #9271 (review)
tahb
force-pushed
the
content-modelling/refactor-content-block-param-handling
branch
3 times, most recently
from
d17d3f1
to
acb4d12
Compare
We’ll use this next with custom error messages.
Following a discussion[1] we want to clarify what we expect this concern to contain. We want this to be more than identification, we want it to be everything that connects an edition to a document. This does diverge us from the main Whitehall we were referencing which may make this harder to merge back together later. But for now the short term benefit of the team knowing how to use it clearly takes priority. [1] #9271 (review)
tahb
force-pushed
the
content-modelling/refactor-content-block-param-handling
branch
2 times, most recently
from
d02b545
to
974078c
Compare
It was becomign cumbersome to remember which attributes were for the document and which were for the edition when it came to creating and updating.
Instead of having to track this we can use the Rails `accepts_nested_attributes_for` to help us take care of this. The main Whitehall repository takes this approach[1].
With it we’re able to send around a single blob of params, pass them to the Edition ORM and it’ll figure out what to do, including validations.
* We have to make one override to the title error message so we see “Title can’t be blank” rather than “Content document title can’t be blank”.
* We also adjust the hidden field for block tag so it puts the params in the same place. Inside `content_block_edition`, rather than `content_block_document_attributes ` at the top level. We’ll now have `content_block_edition: { details: {}, content_block_documentattributes: {}}`.
* In renaming these attributes we also fix the form so the error messages correctly link to the title field on error
[1] https://github.com/alphagov/whitehall/blob/7cbd2b9b6d97df9741befcc4700fb573fb2e1960/app/models/edition.rb#L82]
tahb
force-pushed
the
content-modelling/refactor-content-block-param-handling
branch
from
974078c
to
cc4a997
Compare
Harriethw
approved these changes
Jul 18, 2024
Harriethw
reviewed
Jul 18, 2024
| @@ -1,3 +1,5 @@ | |||
| en: | |||
| activerecord: | |||
| attributes: | |||
| content_object_store/content_block_edition/content_block_document: | |||
There was a problem hiding this comment.
would you be able to ELI5 how this is used? Is there magic going on in the view somewhere?
There was a problem hiding this comment.
Rails magic! Which is why it was so hard to find the right combination.
What I had to go on was this piece of the docs, if you scroll down slightly you get to this:
In the event you need to access nested attributes within a given model, you should nest these under model/attribute at the model level of your translation file:
en:
activerecord:
attributes:
user/role:
admin: "Admin"
contributor: "Contributor"
Which gave me the clue there was some magic / syntax to make it work for nested objects.
The trouble I had was how content_object_store came into the picture, i.e why not
content_object_store/content_block_edition/content_object_store/content_block_document:
or
content_object_store/content_block_edition/content_block_document_attributes:
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.
Changes in this PR
It was becomign cumbersome to remember which attributes were for the document and which were for the edition when it came to creating and updating.
Instead of having to track this we can use the Rails
accepts_nested_attributes_forto help us take care of this. The main Whitehall repository takes this approach[1].With it we’re able to send around a single blob of params, pass them to the Edition ORM and it’ll figure out what to do, including validations.
content_block_edition, rather thancontent_block_document_attributesat the top level. We’ll now havecontent_block_edition: { details: {}, content_block_document_attributes: {}}. This feels more conventional, what do we think?creation.mov