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 new option to save embedded relations changes in parent #150

Merged
merged 12 commits into from
Jun 25, 2016

Conversation

jagdeepsingh
Copy link
Contributor

@jagdeepsingh jagdeepsingh commented Jun 13, 2016

This add a new option embeds_many to the history_trackable_options to save history of embedded objects in parent record itself.
e.g. if there are User and Article models as following:

class User
  include Mongoid::Document
  field :foo
  embeds_many :articles
  track_history embeds_many: [:articles]
end
class Article
  include Mongoid::Document
  field :bar
  embedded_in :user
end

This will save the article.attributes in addition to user's trackable fields to user's history record whenever an user/article is created/updated/destroyed.

@coveralls
Copy link

coveralls commented Jun 14, 2016

Coverage Status

Coverage increased (+0.2%) to 99.415% when pulling bb879e6 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@jagdeepsingh jagdeepsingh force-pushed the store-embedded-relations-in-parent branch from 1e5eeef to f81ce5f Compare June 14, 2016 07:45
@coveralls
Copy link

coveralls commented Jun 14, 2016

Coverage Status

Coverage increased (+0.2%) to 99.415% when pulling f81ce5f on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 14, 2016

Coverage Status

Coverage increased (+0.2%) to 99.415% when pulling f81ce5f on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 14, 2016

Coverage Status

Coverage increased (+0.2%) to 99.416% when pulling 0fe0d74 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@jagdeepsingh
Copy link
Contributor Author

Build failing with following error:

rbx-2.2.10 is not installed - installing.
Searching for binary rubies, this might take some time.
Requested binary installation but no rubies are available to download, consider skipping --binary flag.
Gemset '' does not exist, 'rvm rbx-2.2.10 do rvm gemset create ' first, or append '--create'.

@dblock
Copy link
Collaborator

dblock commented Jun 14, 2016

This is great, but I think embedded objects are no different from other fields in some way, so I think the DSL should just be on: [ :articles ] and the code should be smart enough to know that those are embedded objects, relationships, etc. What do you think?

@jagdeepsingh
Copy link
Contributor Author

@dblock on: [ :articles ] seems as good as embeds_many: [ :articles ]. I will see if this can solve my requirement and do the change if possible.

@coveralls
Copy link

coveralls commented Jun 14, 2016

Coverage Status

Coverage increased (+0.05%) to 99.296% when pulling d5c4456 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@jagdeepsingh
Copy link
Contributor Author

@dblock Now if a model has track_history on: :all, should it also track all embedded relations in addition to all the fields?

@jagdeepsingh
Copy link
Contributor Author

If not, will the user have to specify all fields and embedded associations here?

@jagdeepsingh
Copy link
Contributor Author

Also, if we track all fields and embedded relations in :all, then it will affect users who are already using it. They will be tracking the embedded objects in their applications, unintentionally.

@dblock
Copy link
Collaborator

dblock commented Jun 15, 2016

I think we shouldn't touch :all for backward compatibility purposes. We can add :all_fields and :all_relations or similar in a separate PR if we want to.

@jagdeepsingh
Copy link
Contributor Author

jagdeepsingh commented Jun 15, 2016

We can't touch on: :all, to support existing users. So, how would you like me to modify the DSL in this PR?. May be i can modify it to use a new option associations: :all (Other values can be associations: :embeds_many, associations: :has_many, associations: :belongs_to, etc and of course associations:[:articles, ...]). Wdyt?

@dblock
Copy link
Collaborator

dblock commented Jun 15, 2016

First, thanks for hanging on here.

What I don't like about a DSL that you propose is that it's not naming "things", but "kinds of things". We can't touch on: :all, but we can do on: [:all, :comments], right? It's not ideal because :all should, as you point out, be everything, but i think it's better than associations: :comments and much better than associations: :has_many.

@dblock
Copy link
Collaborator

dblock commented Jun 15, 2016

We should alias :all to :fields and in the future add :associations.

@jagdeepsingh
Copy link
Contributor Author

Ok. For now, i will make it on: [:all, :comments, ...]. And make :all an alias to :fields.

@johnnyshields
Copy link
Member

@dblock the term :relations is better than :associations (in Mongoid parlance)

@dblock
Copy link
Collaborator

dblock commented Jun 16, 2016

Thanks @jagdeepsingh, let us know when it's updated.

embeds_many :comments

track_history :on => [:title, :body],
:embeds_many => [:comments], # track comments as embedded attributes, default is []
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still wrong.

@dblock
Copy link
Collaborator

dblock commented Jun 21, 2016

LMK when this is ready to re-review, see comment above, update CHANGELOG, fix the build, etc.

@jagdeepsingh jagdeepsingh force-pushed the store-embedded-relations-in-parent branch from a332122 to 774dd0d Compare June 22, 2016 06:45
@coveralls
Copy link

coveralls commented Jun 22, 2016

Coverage Status

Coverage decreased (-21.5%) to 77.792% when pulling 52da937 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@jagdeepsingh
Copy link
Contributor Author

This is still a WIP. I will let you know when to re-review it.

- Fix polymorphic tracking and dynamic fields tracking, Fix rspecs
@jagdeepsingh jagdeepsingh force-pushed the store-embedded-relations-in-parent branch from 52da937 to 511542d Compare June 23, 2016 06:29
@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage increased (+0.07%) to 99.318% when pulling 511542d on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage increased (+0.2%) to 99.436% when pulling d7bd271 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage increased (+0.3%) to 99.517% when pulling 113d825 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

- jruby-19mode

allow_failures:
- rvm: rbx-2
Copy link
Collaborator

@dblock dblock Jun 23, 2016

Choose a reason for hiding this comment

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

You do need to re-add rbx-2 above though ;)

@dblock
Copy link
Collaborator

dblock commented Jun 23, 2016

Thanks. I made a comment on class naming, I think we should rename it.

There's still an entry in CHANGELOG needed.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage decreased (-0.01%) to 99.239% when pulling ab6dc25 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage decreased (-0.008%) to 99.242% when pulling 50de994 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@jagdeepsingh
Copy link
Contributor Author

@dblock Please check now.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage decreased (-0.03%) to 99.222% when pulling e7c57bc on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage decreased (-0.03%) to 99.222% when pulling 211ff84 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage decreased (-0.03%) to 99.223% when pulling 12c7d74 on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 99.292% when pulling e4d15ea on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage decreased (-0.08%) to 99.172% when pulling e4d15ea on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.59% when pulling dc2bf2e on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage increased (+0.2%) to 99.435% when pulling dc2bf2e on JagdeepSingh:store-embedded-relations-in-parent into 6948018 on aq1018:master.

@jagdeepsingh
Copy link
Contributor Author

@dblock This is ready to be reviewed now.

@dblock
Copy link
Collaborator

dblock commented Jun 25, 2016

Excellent work, thank you.

@dblock dblock merged commit 47dcfb8 into mongoid:master Jun 25, 2016
@jagdeepsingh jagdeepsingh deleted the store-embedded-relations-in-parent branch June 27, 2016 07:41
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.

None yet

4 participants