Reorder condition in get_field_value function

jazzband / django-auditlog

A Django app that keeps a log of changes made to an object.

MIT License

1.15k stars 414 forks source link

Reorder condition in get_field_value function #652

Closed GreatBahram closed 5 months ago

GreatBahram commented 5 months ago

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?

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hramezani commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

GreatBahram commented 5 months ago

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 :sunflower: