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

Keep dynamically added model receivers at the end when unmuting signals #941

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shosca
Copy link

@shosca shosca commented Jun 9, 2022

Receiver order is somewhat important and packages like django-dirtyfields adds receivers dynamically where it can reset the model state. This change restores the original receivers first then appends the dynamically added receivers.

Receiver order is somewhat important and packages like `django-dirtyfields` adds
receivers dynamically where it can reset the model state. This change restores
the original receivers first then appends any new dynamically added receivers.
@kingbuzzman
Copy link
Contributor

kingbuzzman commented Jun 10, 2022

I’m having a hard time understanding your use case, can you write a test that tests this functionality?

edit: from what i can gather about the package, it attaches itself on __init__ on a signal to the main instance and all the relationships of the instance in order to maintain the state. I understand your use case now. Nevertheless a test that proves this works is vital. Also i’m not entirely sure this needs to be in this package, but that’s up to the members to decide really.

@shosca
Copy link
Author

shosca commented Jun 10, 2022

can you write a test that tests this functionality?

extended the existing test case

i’m not entirely sure this needs to be in this package, but that’s up to the members to decide really.

Yup, though I think it belongs here since mute_signals is the one touching the signal receivers and should be able to restore signal receivers as close to the original as possible

@rbarrois
Copy link
Member

Hey! This looks like a "misfeature" indeed, thanks for the improvement suggestion.

I'll have to dig deeper into the suggested code though, it's not very easy to understand at first sight — and, since this is a seldom touched part of the code, I'll need to be extra sure I understand it in order to avoid breaking the feature later ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants