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

Add spec to test embedded documents changes in parent's history #188

Conversation

mpetazzoni
Copy link

Demonstrates #187.

@mongoid-bot
Copy link

mongoid-bot commented Apr 7, 2017

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.
1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#188](https://github.com/mongoid/mongoid-history/pull/188): Add spec to test embedded documents changes in parent's history - [@mpetazzoni](https://github.com/mpetazzoni).

Generated by 🚫 danger

@dblock
Copy link
Collaborator

dblock commented Apr 8, 2017

This one failed because of rubocop, so rubocop -a ; rubocop --auto-gen-config. Make the spec fail in the way it fails in your bug report.

@mpetazzoni mpetazzoni force-pushed the embedded-document-history-in-parent branch from 2a4dfac to 1f34a85 Compare April 9, 2017 17:53
@mpetazzoni
Copy link
Author

Running rubocop -a finds close to 200 offenses over the whole source tree. rubocop --auto-gen-config finds about 675. Both somehow modify a bunch of files in the source tree. Am I missing some configuration?

Anyway, running bundle exec rake (which is what the continuous build is doing) correctly only show only the errors in my file. I've updated the commit.

@coveralls
Copy link

coveralls commented Apr 9, 2017

Coverage Status

Coverage decreased (-0.03%) to 99.72% when pulling 1f34a85 on mpetazzoni:embedded-document-history-in-parent into c641447 on mongoid:master.

@dblock
Copy link
Collaborator

dblock commented Apr 9, 2017

I played with this, and it's definitely a bug. In short, when the parent is saved, changes doesn't include the changes from the child relation during update, however it does include it during insert. I am not sure how to fix this, maybe @jagdeepsingh who wrote embedded support or @jnfeinstein who added some improvements on top could take a look?

@dblock dblock added the bug label Apr 9, 2017
@jagdeepsingh
Copy link
Contributor

@mpetazzoni i appreciate the research done on this bug. I will try to look into the cause.

@mpetazzoni
Copy link
Author

mpetazzoni commented Apr 10, 2017

Thanks @dblock and @jagdeepsingh, appreciated. Unfortunately I don't know enough about Rails and Mongoid to fix this myself; there's way too much magic going on and I couldn't figure out whether I was missing something in the way my models were setup, or if something was missing in mongoid-history's code. Looking forward to hearing back from you on this soon, hopefully with a fix😄 !

@mateuspontes
Copy link

def modified_attributes_for_update
     @modified_attributes_for_update ||= Mongoid::History::Attributes::Update.new(self).attributes
end

always returns nil when you update only the child.

@jagdeepsingh
Copy link
Contributor

Yes, @mateuspontes as i posted here, this is because the changes hash remains empty if we update only the embedded relations AND we are not using any code/gem to track changes in them.

@mateuspontes
Copy link

@jagdeepsingh in create we track changes on embedded relations, can we use the same aproach to catch changes on update?

mateuspontes pushed a commit to mateuspontes/mongoid-history that referenced this pull request Apr 13, 2017
* Fix test to check Parent and Child updates
@mateuspontes
Copy link

I'll create a new PR with a new aproach to solve this without extra gems, this is working with embed_one only for now. It's a WIP

[#<Tracker _id: 58efcf927ce04a71b049aac4, created_at: 2017-04-13 19:20:50 UTC, updated_at: 2017-04-13 19:20:50 UTC, association_chain: [{"name"=>"Parent", "id"=>BSON::ObjectId('58efcf927ce04a71b049aac5')}], modified: {"name"=>"bowser", "child"=>{"_id"=>BSON::ObjectId('58efcf927ce04a71b049aac6'), "name"=>"todd"}}, original: {}, version: 1, action: "create", scope: "parent", modifier_id: nil>, #<Tracker _id: 58efcf927ce04a71b049aac7, created_at: 2017-04-13 19:20:50 UTC, updated_at: 2017-04-13 19:20:50 UTC, association_chain: [{"name"=>"Parent", "id"=>BSON::ObjectId('58efcf927ce04a71b049aac5')}], modified: {"name"=>"brow"}, original: {"name"=>"bowser", "child"=>{}}, version: 2, action: "update", scope: "parent", modifier_id: nil>, #<Tracker _id: 58efcf927ce04a71b049aac8, created_at: 2017-04-13 19:20:50 UTC, updated_at: 2017-04-13 19:20:50 UTC, association_chain: [{"name"=>"Parent", "id"=>BSON::ObjectId('58efcf927ce04a71b049aac5')}], modified: {"child"=>{"name"=>"mario"}}, original: {"child"=>{"name"=>"todd"}}, version: 3, action: "update", scope: "parent", modifier_id: nil>]

mateuspontes pushed a commit to mateuspontes/mongoid-history that referenced this pull request Apr 13, 2017
* Fix test to check Parent and Child updates

Fix: remove focus: true and debug info with puts

Fix tests: add method to track changes on update with changes

* Add method to track changes with changes empty
mateuspontes pushed a commit to mateuspontes/mongoid-history that referenced this pull request Apr 13, 2017
* Fix test to check Parent and Child updates

Fix: remove focus: true and debug info with puts

Fix tests: add method to track changes on update with changes

* Add method to track changes with changes empty
mateuspontes pushed a commit to mateuspontes/mongoid-history that referenced this pull request Apr 13, 2017
* Fix test to check Parent and Child updates

Fix: remove focus: true and debug info with puts

Fix tests: add method to track changes on update with changes

* Add method to track changes with changes empty
mateuspontes pushed a commit to mateuspontes/mongoid-history that referenced this pull request Apr 13, 2017
* Fix test to check Parent and Child updates

Fix: remove focus: true and debug info with puts

Fix tests: add method to track changes on update with changes

* Add method to track changes with changes empty
mateuspontes pushed a commit to mateuspontes/mongoid-history that referenced this pull request Apr 13, 2017
* Fix test to check Parent and Child updates

Fix: remove focus: true and debug info with puts

Fix tests: add method to track changes on update with changes

* Add method to track changes with changes empty
@mpetazzoni mpetazzoni closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants