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 :ignore_log_data option to #has_logidze #98

Merged
merged 2 commits into from
Nov 28, 2018

Conversation

DmitryTsepelev
Copy link
Collaborator

@DmitryTsepelev DmitryTsepelev commented Nov 21, 2018

Fixes #97

@DmitryTsepelev DmitryTsepelev force-pushed the ignore-columns branch 9 times, most recently from e043e67 to 81e21b6 Compare November 22, 2018 13:23
end
end

def log_data
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to override this? If the column is ignored (in Rails 5), would Rails raise this exception for us?

I’d like to use as many Rails built-in functionality as possible

end

def log_data!
log_data_attribute || self.class.with_log_data.find(id).log_data
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn’t call DB twice if we call log_data! twice.

Also, can we use pluck(:log_data) to avoid additional AR instance initialization?

module IgnoreLogData
extend ActiveSupport::Concern

module BackportIgnoredColumns # :nodoc:
Copy link
Owner

Choose a reason for hiding this comment

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

Let’s move this patch into a separate file and add columns information manipulation (like in the original patch for Rails 5 or in ignorable).

@DmitryTsepelev DmitryTsepelev force-pushed the ignore-columns branch 7 times, most recently from 864b9ef to 59e9f1f Compare November 27, 2018 19:20
end

def log_data!
@log_data ||= self.class.where(id: id).pluck(:log_data).first
Copy link
Owner

Choose a reason for hiding this comment

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

Should we store the value in the attributes["log_data"]? As I understand, the following code reloads log_data (even if it present):

user = User.with_log_data.find(id)
user.log_data!

TBH, the API looks like a hack.

Maybe, it's better to add a method to reload log_data explicitly instead? Smth like:

def reload_log_data
  attributes["log_data"] = self.class.where(id: id).pluck(:log_data).first
end

This method could be a part of Logidze::Model.

Btw, we should not rely on id but on model's primary key instead:

self.class.where(self.class.primary_key => send(self.class.primary_key)).pluck(:log_data).first

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like id takes care about resolving primary key

Copy link
Owner

Choose a reason for hiding this comment

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

It takes care of the value; does it take care of the key? (i.e. where(id: ...) still uses id)

Let's rename this method to reload_log_data, move to Logidze::Model and add a readme section. This method could also be useful to fetch the actual log_data after creating or updating a record.

@DmitryTsepelev DmitryTsepelev force-pushed the ignore-columns branch 2 times, most recently from ef63cdc to f2711a9 Compare November 28, 2018 10:52
@palkan palkan merged commit cf8c840 into palkan:master Nov 28, 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.

2 participants