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

Do not create CRUDEvent on update if none of the fields has changed. #106

Conversation

guy881
Copy link
Contributor

@guy881 guy881 commented Oct 10, 2019

Hello! Great project, thanks a lot!
I updated the code to not create CRUDEvent if none of the fields has changed.

@guy881 guy881 changed the title Do not create CRUDEvent on update if none of the fields were updated. Do not create CRUDEvent on update if none of the fields has changed. Oct 10, 2019
Copy link
Collaborator

@jheld jheld left a comment

Choose a reason for hiding this comment

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

couple of minor things but otherwise this looks good. Will plan to merge shortly.

@jheld jheld merged commit 0e720f3 into soynatan:master Feb 2, 2020
@jheld jheld mentioned this pull request Feb 11, 2020
@jheld
Copy link
Collaborator

jheld commented Feb 22, 2020

This appears to be causing a bug in the signal handler (the receiver gets cleared out prematurely).

Will be looking to back out the change for this release and consider putting it back in with it fixed.

@jheld
Copy link
Collaborator

jheld commented Feb 26, 2020

my project uses factories, so it's possible there is something happening there.
the factory does create a crud event, does another thing in the post init, but then when we go to make a real next save, there is no crud event changed. so despite the tests passing, still makes me wonder.
removal of just that delta if makes it pass.

@guy881
Copy link
Contributor Author

guy881 commented Feb 26, 2020

@jheld if you could write the test for that issue, then I could try to help resolve it. :)

@jheld
Copy link
Collaborator

jheld commented Feb 27, 2020

My next thought is we can allow people to use this feature but give them the option to do it, and by default do the old behavior. So if anyone wants to use it/try it out, then they can do so and until we can fully fix it, the rest of the default users (or those who explicitly don't want a behavior change), can go on like normal.

I think I'll do that within this or next week.

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