- Fixed modifyRows when using enableRowHashing option

[idangozlan]

• When using enableRowHashing (which is set right now to true as default), modifyRows match the old and the new row but not using the new entity at all.

[mportuga]

@idangozlan Please include an issue number.

[mportuga]

@idangozlan Can you share a plunker that reproduces the issue and include an issue number?

[mportuga]

Closing due to lack of a response.

[idangozlan]

Hi, sorry for the delay, here is the reproduce:

http://plnkr.co/edit/0y6nKRnhJ6T8BHWfiZEf?p=preview

Test it before and after my changes to see the fix.

[mportuga]

@idangozlan Can you add some unit test to check ensure that this issue does not show up again? I was able to reproduce it in your plunker.

[idangozlan]

You should be able to reproduce it on my plunker (this is the idea, isnt
that?)

I will be able but later

On Tuesday, November 8, 2016, Marcelo Sauerbrunn Portugal <
notifications@github.com> wrote:

@idangozlan https://github.com/idangozlan Can you add some unit test to
check ensure that this issue does not show up again? I was able to
reproduce it in your plunker.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4818 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5hbSj1zHXmOegaEpkAS3ZUfhDinaCeks5q8Ji-gaJpZM4GwUge
.

Best Regards,
Idan Gozlan, +972542251188.

P Please consider the environment before printing this email.

[mportuga]

Sorry, yes, that is the idea. I am happy that I was able to reproduce it, so I will approve as long we cover this via unit tests to prevent it from happening again.

[mportuga]

@idangozlan Are you going to add tests?

[idangozlan]

Hi, I don't have free time to do so right now, sorry.
If you wish, you can add tests or I will try to find a time for that in the
future.

On Wed, Nov 16, 2016 at 8:31 PM, Marcelo Sauerbrunn Portugal <
notifications@github.com> wrote:

@idangozlan https://github.com/idangozlan Are you going to add tests?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4818 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5hbaT-fNBUBaXOzsVTz5p_PAS3SQvdks5q-0vngaJpZM4GwUge
.

Best Regards,
Idan Gozlan, +972542251188.

P Please consider the environment before printing this email.

[mportuga]

Closing this PR, as I have opened a new PR with these same changes plus unit tests.
#5917

[idangozlan]

So after some discussion, I guess you can use my pull request?

[mportuga]

Yeah, it got merged with some unit tests added and minor changes. Thank you for your contribution!

[idangozlan]

Great. My pleasure