-
Notifications
You must be signed in to change notification settings - Fork 652
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
Migrate ::Logs model to AR #15896
Migrate ::Logs model to AR #15896
Conversation
(@fixed_entries_half + ordered_circular_entries_half).join('') | ||
end | ||
|
||
def append(content, truncate = true, timestamp = Time.now.utc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubocop is complaning about this method signature that is being called hundreds of times along the project. Changing it is kind of risky, so I'm going to ignore it (for now)
add_to_entries(format(ENTRY_FORMAT, timestamp, content)) | ||
end | ||
|
||
def append_and_store(content, truncate = true, timestamp = Time.now.utc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above with the append
method
|
||
SAVE_BLOCK = 100 # Save the changes to DB every X lines | ||
|
||
def initialize(log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the parent constructor is breaking tests. Ignoring this linter error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how much behavior is really being reused from the ::Logger
class and that if we could remove it 🤔 . In any case, it's out the scope of this ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor (optional) comments. Really nice work, one less model to go 👍 🙂
@@ -145,7 +145,9 @@ def validate_export_data | |||
end | |||
|
|||
def set_defaults | |||
self.log = Carto::Log.create(type: 'user_migration_export') unless log | |||
self.log = Carto::Log.new_user_migration_export unless log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as curiosity, another way to achieve this is:
self.log ||= Carto::Log.new_user_migration_export
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, thanks!
@@ -554,7 +553,7 @@ def log | |||
|
|||
log_attributes.merge(user_id: user.id) if user | |||
|
|||
@log = CartoDB::Log.where(log_attributes).first | |||
@log = Carto::Log.find_by(log_attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
@@ -60,7 +60,7 @@ def teardown_mock_plpython_function(user) | |||
export = create(:user_migration_export, user_id: carto_user.id, export_data: false) | |||
export.run_export | |||
|
|||
puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE | |||
puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these puts
some kind of debugging info for when the test fails? Do you think it would be sensible to remove them?
@@ -1,72 +1,264 @@ | |||
class Carto::Log < ActiveRecord::Base | |||
module Carto | |||
class Log < ActiveRecord::Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the namespace 🙏
app/models/carto/log.rb
Outdated
CartoDB.notify_error( | ||
'Error appending log, likely an encoding issue', | ||
{ error_info: "id: #{id}. #{inspect} --------- #{e.backtrace.join}" } | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we replace this kind of reports per the new "syntax"? Something like:
log_error(message: 'Error appending log, likely an encoding issue', exception: e, log_id: id)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in fact we commented this improvements a few days ago, but I totally forgot :(
Let me fix it now
app/models/carto/log.rb
Outdated
|
||
list = @fixed_entries_half | ||
circular_half = ordered_circular_entries_half | ||
list += [HALF_OF_LOG_MARK] unless circular_half.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid a double negation you could do:
list += [HALF_OF_LOG_MARK] if circular_half.any?
|
||
SAVE_BLOCK = 100 # Save the changes to DB every X lines | ||
|
||
def initialize(log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how much behavior is really being reused from the ::Logger
class and that if we could remove it 🤔 . In any case, it's out the scope of this ticket.
I had to ignore some Linter errors due to the complexity of fixing them