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

Reorder condition in get_field_value function #652

Closed
wants to merge 6 commits into from

Conversation

GreatBahram
Copy link
Contributor

Hello there,

Some projects have their own fields that are not connected to any model. With the previous approach, the code would have broken, but with this order, it will consider those too. What do you think?

…n-database fields and this will break their code ;)
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (5289482) to head (96d3906).
Report is 17 commits behind head on master.

Current head 96d3906 differs from pull request most recent head fbb1bc7

Please upload reports for the commit fbb1bc7 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #652      +/-   ##
==========================================
+ Coverage   95.21%   95.29%   +0.07%     
==========================================
  Files          31       31              
  Lines        1025     1042      +17     
==========================================
+ Hits          976      993      +17     
  Misses         49       49              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hramezani
Copy link
Member

Thanks @GreatBahram for this PR.
Could you please:

  • Add a test to show what problem you are going to fix
  • Add an entry to the changelog file.

@GreatBahram
Copy link
Contributor Author

Thanks @GreatBahram for this PR. Could you please:

* Add a test to show what problem you are going to fix

* Add an entry to the changelog file.

Hi there, I thought it would be beneficial to check the rel_class attribute first and then validate the one_to_one and many_to_one attributes.

This change was disrupting our codebase. We had several non-database fields that functioned correctly before updating the auditlog. The workaround for us was to set the many_to_one and one_to_one attributes to make sure we don't end up in this newly introduced condition. I wonder if it makes sense to do it this way. what do you think?

@hramezani
Copy link
Member

GreatBahram

Agree, that make sense to check the rel_class before. we were not aware of your usecase. that's why I asked for a test case. so, next time if somebody change the condition order, the test will fail.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -2164,6 +2164,19 @@ def test_log_entry_created_if_obj_strings_are_same_for_two_objs(self):
self.assertEqual(int(log_create.changes_dict["related"][1]), one_simple.id)
self.assertEqual(int(log_update.changes_dict["related"][1]), two_simple.id)

def test_rel_class_checked_first(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to create a model as you described in the PR description and test with a real model instead of mocking

Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
@GreatBahram
Copy link
Contributor Author

Hi there, while working on the test case and investigating Django non-database fields reached this conclusion we should have set many_to_many, many_to_one and one_to_one to None, so I'll close this issue, thanks for the support 🌻

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.

2 participants