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

fix: use sender for m2m signal dispatch connection #686

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

cdubz
Copy link
Contributor

@cdubz cdubz commented Nov 10, 2024

This fix adds support for a use case where a single m2m through model is used on multiple models. When the receiver is used for the dispatch uid in this use case it cause duplicated logs because the through model signal connection happens multiple times.

By changing the m2m signal connection to use the sender for the dispatch uid this duplication is prevented because the signal connection only happens once for the through model.

Fixes #685

I wasn't able to run tests as I don't have a postgres database connection handy and I couldn't get the tests to work with SQLite. Test still need to be run and probably a test should be added for this use case.

This fix adds support for a use case where a single m2m through model is
used on multiple models. When the reciever is used for the dispatch uid
in this use case it cause duplicated logs because the through model
singal connection happens multiple times.

By changing the m2m signal connection to use the sender for the dispatch
uid this duplication is prevented because the signal connection only
happens once for the through model.

Refs: #685
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.49%. Comparing base (5289482) to head (b8a36e3).
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #686      +/-   ##
==========================================
+ Coverage   95.21%   95.49%   +0.27%     
==========================================
  Files          31       32       +1     
  Lines        1025     1087      +62     
==========================================
+ Hits          976     1038      +62     
  Misses         49       49              

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

@hramezani
Copy link
Member

Thanks @cdubz for reporting the issue and for the fix PR.

You can easily run a Postgres in your machine by docker run --name some-postgres -e POSTGRES_PASSWORD=postgres -p 5432:5432 -d postgres and then you can add a test.

Please remember to add a changelog entry as well

@cdubz
Copy link
Contributor Author

cdubz commented Nov 11, 2024

Oh yeah! I will add a test and changelog entry. Thanks!

@cdubz
Copy link
Contributor Author

cdubz commented Nov 11, 2024

Ok, I think this is ready for review again -- not used to this set of formatting tools 😁

I confirmed the test I added reproduces the issue on master (and fails).

CHANGELOG.md Outdated Show resolved Hide resolved
@hramezani hramezani merged commit 5621777 into jazzband:master Nov 12, 2024
7 checks passed
@cdubz cdubz deleted the 685-duplicate-logs branch November 12, 2024 15:35
@hramezani
Copy link
Member

Thanks @cdubz

@cdubz
Copy link
Contributor Author

cdubz commented Nov 12, 2024

Thanks for the quick review!

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.

Logs duplicated when used in conjunction with django-taggit and a custom tag model shared between models
2 participants