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

Add m2m with no through_defaults fix #170

Merged

Conversation

grahamulator
Copy link
Contributor

This change addresses issue #168, (mentioned also in this comment).

In summary, attempting to add an m2m relationship when through_defaults are not passed as an argument causes the following error:
TypeError: 'NoneType' object does not support item assignment

This change adds a line to django_multitenant/mixins.py to set through_defaults to an empty dict if no value is passed in, which prevents an assignment to None error.

@grahamulator
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Twingate"

@grahamulator
Copy link
Contributor Author

Hi @gurkanindibay. Following up on our conversation here. I modified the Transaction model to better illustrate the scenario I described. The test I added is a duplicate of the one above it, but with no through_defaults passed in the add operation.

To reproduce the error scenario, simply comment out or remove the line I added to the mixins.py file and run the test.

Please feel free to modify this PR if you don't want to make changes to the models/migrations themselves, or let me know if I can change/improve in the PR. Thanks again!!

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #170 (d35a9b4) into main (68270b1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #170   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           43        44    +1     
  Lines         1180      1193   +13     
=========================================
+ Hits          1180      1193   +13     
Impacted Files Coverage Δ
...nt/tests/migrations/0031_alter_transaction_date.py 100.00% <100.00%> (ø)
django_multitenant/tests/models.py 100.00% <100.00%> (ø)
django_multitenant/tests/test_models.py 100.00% <100.00%> (ø)

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

@gurkanindibay gurkanindibay self-requested a review April 6, 2023 00:21
Copy link
Contributor

@gurkanindibay gurkanindibay left a comment

Choose a reason for hiding this comment

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

Thanks @grahamulator for your amazing efforts to fix the issue.
All works well

I hope you contribute more and make django-multitenant great together :)

@gurkanindibay gurkanindibay merged commit ab0acaf into citusdata:main Apr 6, 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.

2 participants