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

Fixed #17688 -- Added actions to m2m_changed for deleted objects. #5505

Closed
wants to merge 1 commit into from
Closed

Fixed #17688 -- Added actions to m2m_changed for deleted objects. #5505

wants to merge 1 commit into from

Conversation

jorgecarleitao
Copy link
Contributor

attr_name = m2m_relation.name
sender = m2m_relation.remote_field.through
else:
assert(isinstance(m2m_relation, ManyToManyRel))
Copy link
Member

Choose a reason for hiding this comment

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

What about using an elif isinstance(m2m_relation, ManyToManyRel) clause instead and issuing a continue in an else branch? This looks more safe in regard to third party fields that might exposed them as many_to_many = True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing this PR. I agree, with this and all your other suggestions. They were incorporated in the new version of the PR. Also, the failing tests now should pass.

sender = m2m_relation.remote_field.through
elif isinstance(m2m_relation, ManyToManyRel):
attr_name = m2m_relation.get_accessor_name()
sender = m2m_relation.through
Copy link
Member

Choose a reason for hiding this comment

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

I can't come up with a way to differentiate both instance either. Ideally they would have a similar API.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it can be cleaned up once https://code.djangoproject.com/ticket/24317 is resolved?

@timgraham
Copy link
Member

Needs a mention in the 1.10 release notes.

@timgraham
Copy link
Member

Closing due to inactivity. Please send a new PR if you return to it.

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