Skip to content

Fix embed serializer to put only changed attrs to item_changes - #238

Open
sergei-krylov wants to merge 2 commits into
izelnakri:mainfrom
sergei-krylov:fix-embed-serialize
Open

Fix embed serializer to put only changed attrs to item_changes#238
sergei-krylov wants to merge 2 commits into
izelnakri:mainfrom
sergei-krylov:fix-embed-serialize

Conversation

@sergei-krylov

Copy link
Copy Markdown

Similar to how changes are tracked in version item_changes for top-level entity, the same should apply to embeds.

Issue with the current behavior is that when updating it puts all the attributes available in embedded schema, having new values set only to changed attributes, and others are set to nil. Therefore, from a single version record it is impossible to determine whether the value was changed and set to nil or not.

Fixed behavior keeps only attributes tracked in changeset as changes, the rest untouched attributes are not put to serialized embed.

Similar to how changes are tracked in version `item_changes` for top-level entity, the same should apply to embeds.

Issue with the current behavior is that when updating it puts all the attributes available in embedded schema, having new values set only to changed attributes, and others are set to `nil`.
Therefore, from a single version record it is impossible to determine whether the value was changed and set to `nil` or not.

Fixed behavior keeps only attributes tracked in changeset as changes, the rest untouched attributes are not put to serialized embed.
@izelnakri

izelnakri commented Feb 21, 2025

Copy link
Copy Markdown
Owner

Hi @sergei-krylov , please add a test case showing the issue and then I can review the fix easily and we can keep these edge cases in our test suite.

…d explicit test asserts for embed schema changes
@sergei-krylov

Copy link
Copy Markdown
Author

Hi @izelnakri , there is already an existing test checking that when updating embeds it creates a version with correct attributes:
https://github.com/izelnakri/paper_trail/pull/238/files#diff-6fc8a204da55df7353b15220afa17117be281e40849223a82a892acd9f248f10R452

Compared to the previous behavior where it always returned all attributes of the embed, now it only returns those that were changed.
I also added one more language attribute to Person model for more attribute variety.

I just rechecked the test and it was a bit incomplete, I added a strict asserts to check for only changed attributes. Now it will fail if there are more (or less) attributes in item_changes -> embed, so it ensures there are only attributes that have changed.

Let me know if you want me to put a whole separate test case or something. For me this updated one is good enough.
Thank you.

@goodniceweb

Copy link
Copy Markdown

Hi @izelnakri

Hope you're doing well.

I was wondering if you might have an estimated timeline for when this PR might be reviewed or merged?

Please know there's absolutely no pressure - I'm simply trying to plan accordingly. If there's anything we can do to help move the PR forward, such as making adjustments or providing additional information, we'd be more than happy to do so.

Thank you for your time and consideration.

Best regards,
Alexey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants