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

WIP: track changes on embedded_one #191

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

Conversation

mateuspontes
Copy link

  • embeds_one with changes in Parent
  • embeds_one with empty changes in Parent
  • embeds_many with empty changes on Parent

@mateuspontes mateuspontes force-pushed the master branch 4 times, most recently from 454e075 to 448700d Compare April 13, 2017 21:53
* 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
@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage increased (+0.004%) to 99.752% when pulling f741e5c on mateuspontes:master into 439c0e1 on mongoid:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.752% when pulling f741e5c on mateuspontes:master into 439c0e1 on mongoid:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.752% when pulling f741e5c on mateuspontes:master into 439c0e1 on mongoid:master.

@@ -13,6 +13,7 @@ def attributes
@attributes[k] = format_field(k, v)
end
end
insert_embeds_one_changes_on_child if trackable_class.tracked_embeds_one.present? && changes.empty?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems strange that you'd have the changes.empty? check here. Maybe write a spec that changes fields, then embeds, then both. Then we're keeping things in @attributes, so I imagine this really should be something like

@attributes ||= begin
   changes_in_this_instance_or_something_like_that  + changes_in_embeds + ...
end

@@ -27,6 +28,22 @@ def insert_embeds_one_changes(relation, value)
@attributes[relation][1] = value[1][paranoia_field].present? ? {} : format_embeds_one_relation(relation, value[1])
end

def insert_embeds_one_changes_on_child
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be renamed and refactored to return changes on embeds. The code for non-embeds should be changed to return changes on non-embeds, finally attributes should collect all of the things.

@dblock
Copy link
Collaborator

dblock commented Apr 15, 2017

Great progress. If you solve 2/3 of the cases that's already good enough to merge, so feel free to do that if the last one turns out to be too hard.

@dblock
Copy link
Collaborator

dblock commented Apr 15, 2017

Also, there's nothing wrong with taking a dependency on something like https://github.com/bopm/mongoid_relations_dirty_tracking, in fact I would prefer that since it (theoretically) specializes in one thing better.

@johnnyshields
Copy link
Member

I'm actually doing this now using https://github.com/bopm/mongoid_relations_dirty_tracking

@mateuspontes
Copy link
Author

mateuspontes commented Apr 17, 2017

@johnnyshields so I can pause this POC and wait your PR.

@mateuspontes
Copy link
Author

@johnnyshields pls tell me if I can help you with code or something like that.

@mpetazzoni
Copy link

Hi folks,

Thanks for the work on this. What's the current status? What combination of extensions/code do I need to make #187 work?

@mateuspontes
Copy link
Author

@mpetazzoni I'm facing the same problem. I'm using bopm/mongoid_relations_dirty_tracking to track modifications but it post all embeds_many to history_track and after that my embeds_many data are duplicated (see this commit). Without this gem I cann't track changes in embed.

I was trying to create a POC to track changes in embeds_one, but I think the best option is to use MRDT. Waiting @johnnyshields update this issue

@johnnyshields
Copy link
Member

johnnyshields commented Apr 27, 2017

@mateuspontes no PR needed, this works for me now:

class Customer
  include Mongoid::History::Trackable
  include Mongoid::RelationsDirtyTracking

  embeds_many :phones

  relations_dirty_tracking only: [:phones]

  track_history on: [{ phones: %i(tag number extension) }]
end

@johnnyshields
Copy link
Member

@mateuspontes sorry I didn't read this carefully enough, if this PR is specifically for the embeds_one case then please go ahead, my apologies I didn't realize there was an issue there.

@mateuspontes
Copy link
Author

@johnnyshields thx for your help, your code works for me but I end up with my embeds duplicated, I'm using mongo 3.4

Something like that:

{
  "customer": {
    "name": "Doe",
    "phones": [
      {
        "comercial": "11111"
      }
    ]
  }
}

After update @customer:

{
  "customer": {
    "name": "Doe",
    "phones": [
      {
        "comercial": "11111"
      },
      {
        "comercial": "11111"
      }
    ]
  }
}

@johnnyshields
Copy link
Member

@mateuspontes that sounds like an issue unrelated to this gem. When you are updating make sure you are using the _id field on the embedded objects.

@mateuspontes
Copy link
Author

mateuspontes commented Apr 28, 2017

@johnnyshields I'm using rails 4.2.7 and mongoid 5.2, the issue is related to bopm/mongoid_relations_dirty_tracking, without mongoid_relations_dirty_tracking I cann't store relations changes.

IMHO mongoid-history should track embeds changes as it can track has_many, actually it doesn't track embeds, the code isn't work and it is confirmed as a bug.

@mateuspontes
Copy link
Author

mateuspontes commented Apr 28, 2017

After update parent (with mongoid_relations_dirty) action.update(action_params).
Without mongoid_relations_dirty I cann't see embeds diff, mongoid-history doesn't track it. That was the motivation to PR.

> db.actions.findOne({"_id": ObjectId("5903a7f07ce04a617e3c3bf1")})
{
	"_id" : ObjectId("5903a7f07ce04a617e3c3bf1"),
	"name" : "Parent Action",
	"operations" : [
		{
			"_id" : ObjectId("5903ae5c7ce04a63e0d8149f"),
			"name" : "Duplicated child"
		},
		{
			"_id" : ObjectId("5903ae5c7ce04a63e0d8149f"),
			"name" : "Duplicated child"
		}
	],
	"version" : 2
}

@dblock
Copy link
Collaborator

dblock commented May 1, 2017

Still looking forward for a mergeable PR here!

Startouf added a commit to Startouf/mongoid-history that referenced this pull request Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants