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

added fix for OneToOneRel error happening in case of Polymorphic model #429

Merged
merged 12 commits into from
Sep 21, 2022

Conversation

dev27git
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #429 (4c498c8) into master (fb90112) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   92.24%   92.28%   +0.04%     
==========================================
  Files          24       24              
  Lines         709      726      +17     
==========================================
+ Hits          654      670      +16     
- Misses         55       56       +1     
Impacted Files Coverage Δ
auditlog/diff.py 94.11% <100.00%> (ø)
auditlog/mixins.py 86.95% <0.00%> (+1.62%) ⬆️

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

@hramezani
Copy link
Member

Thanks @rahulpr27git for this patch 👍
Please add a regression test and also add a changelog entry in CHANGELOG.md

tox.ini Outdated
@@ -21,6 +21,7 @@ deps =
codecov
freezegun
psycopg2-binary==2.8.6
django-polymorphic
Copy link
Member

Choose a reason for hiding this comment

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

Do we need django-polymorphic for testing? isn't it possible to add test without this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. it's happening with a Polymorphic model so need this dependency else i wont be able to reproduce.

Copy link
Member

Choose a reason for hiding this comment

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

I would take a look. it may be possible to reproduce the problem without Polymorphic. Otherwise, we can accept it.

Copy link
Contributor Author

@dev27git dev27git Sep 20, 2022

Choose a reason for hiding this comment

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

Ok. thanks.
this is currently became a major blocker for my project.
Hopefully we can resolve this one as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

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

btw, test still failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hramezani updated the test

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @rahulpr27git.

Seems hard to reproduce the problem without Polymorphic. I would suggest just keeping the change and reverting all the changes related to the test. we will accept this change without tests. Because the fix itself is clear and I don't want to include another project just for one single test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hramezani please check again.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @rahulpr27git

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hramezani hramezani merged commit a56d0e6 into jazzband:master Sep 21, 2022
dbramwell pushed a commit to sponge-learning/django-auditlog that referenced this pull request Jun 23, 2023
dbramwell pushed a commit to sponge-learning/django-auditlog that referenced this pull request Jun 26, 2023
jazzband#429) (munged a bit to work with our old version of auditlog)
dbramwell pushed a commit to sponge-learning/django-auditlog that referenced this pull request Jun 26, 2023
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