Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize fields that are relations to another model correctly #4

Merged
merged 7 commits into from
Apr 14, 2023

Conversation

akikoskinen
Copy link
Contributor

@akikoskinen akikoskinen commented Apr 12, 2023

While making the library more configurable, I encountered an issue that the serialization didn't work for fields that are relations to another model. The Profile model's user field, as a one-to-one relation to the User model, is an example of such a field. The user field was previously serialized only with an "accessor" that extracted only a single field from the User model. Without an accessor serialization of the user field didn't work.

Before fixing the above mentioned issue, I did some refactoring on the serialization code.

@akikoskinen akikoskinen changed the title Fix related field serializeSerialize fields that are relations to another model correctly Serialize fields that are relations to another model correctly Apr 12, 2023
The snapshot tests require fixed data, which was previously provided by
two tests. The fixed data can be provided by the test data factory too.
This is a more appropriate name for this value, which describes a field
in model.
The inner function can get everything it needs from the outer context.
The previous code serialized every field twice. First to check if it
should get included into the end result (if it's not `None`), and if
yes, then another time for the actual return value.
Removes one identical getattr call.
If a model that is serialized has a field that is a relation to another
model, that related model's (or manager's) `serialize` method needs to
be called.

Tested this by serializing also the User model that Profile model has a
one-to-one relation to. The "user" field is now serialized twice, once
via its `serialize` method and once with the special "accessor".
@codecov-commenter
Copy link

Codecov Report

Merging #4 (1ece52b) into main (6d26b60) will increase coverage by 0.39%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
+ Coverage   91.83%   92.23%   +0.39%     
==========================================
  Files           4        4              
  Lines          98      103       +5     
  Branches       13       14       +1     
==========================================
+ Hits           90       95       +5     
  Misses          5        5              
  Partials        3        3              
Impacted Files Coverage Δ
helsinki_gdpr/models.py 92.85% <91.66%> (+1.55%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@akikoskinen akikoskinen merged commit 1ece52b into main Apr 14, 2023
@akikoskinen akikoskinen deleted the fix-related-field-serialize branch April 14, 2023 05:06
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.

None yet

3 participants