-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 Changeset.changed?/2
for assocs
#4544
base: master
Are you sure you want to change the base?
Conversation
Setting assoc to nil actually errors as it is not handled in changed?/2, while settin existing assoc to belongs_to assoc returns changed?/2 as false.
It now returns true whenever `fetch_change? != :error`. Previously, it returned false when assoc was set to existing struct (as `changeset.changes == %{}`) and errored when set to nil (due to `nil.changes != %{}` check in `relation_changed?`. Fixes elixir-ecto#4543.
e74f9ee
to
2fce190
Compare
We should fix it to handle |
Hi @josevalim thank you for looking into this. Note that handling |
Yes, we need to check more cases. And potentialy make it recursive. But simply checking if the field is set is not enough. |
Ping! :) |
Sorry for the delay. I've been tied up with other priorities — I will try to move this forward till the end of this month... |
@josevalim I am having some difficulty coming up with test cases that would fail with
already removes profile assoc changeset (as it would contain We could probably manipulate changeset struct directly, but we should probably test for scenarios that can results from using Changeset functions? |
It now returns true whenever
fetch_change? != :error
. Previously, it returnedfalse when assoc was set to existing struct (as
changeset.changes == %{}
) anderrored when set to nil (due to
nil.changes != %{}
check inrelation_changed?
.Fixes #4543.