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

chore: refactor actions controller #2703

Merged
merged 48 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
1167828
WIP
Paul-Bob Jan 10, 2024
8c4c54a
wip
Paul-Bob Jan 10, 2024
fc9720b
WIP
Paul-Bob Jan 13, 2024
04e7d7f
wip
Paul-Bob Jan 15, 2024
2f6dbc3
typo
Paul-Bob Jan 15, 2024
9826cdc
Merge branch 'main' into feature/audit_logs
Paul-Bob Jan 17, 2024
0070b19
WIP
Paul-Bob Jan 17, 2024
f64dfb8
WIP
Paul-Bob Jan 19, 2024
8d9248c
wip
Paul-Bob Jan 22, 2024
0771d63
Merge branch 'main' into feature/audit_logs
Paul-Bob Feb 15, 2024
2c9b896
wip
Paul-Bob Feb 16, 2024
361120a
wip
Paul-Bob Feb 16, 2024
0db29ba
Merge branch 'main' into feature/audit_logs
Paul-Bob Apr 2, 2024
4986af8
dont break if db do not exist
Paul-Bob Apr 2, 2024
16c3019
wip
Paul-Bob Apr 3, 2024
f88a17f
Merge branch 'main' into feature/audit_logs
Paul-Bob Apr 4, 2024
496f42d
rename package
Paul-Bob Apr 9, 2024
cdde642
Merge branch 'main' into feature/audit_logs
Paul-Bob Apr 11, 2024
da5f08f
Merge branch 'main' into feature/audit_logs
Paul-Bob May 13, 2024
58210f7
extract audit method and safe_call it
Paul-Bob May 13, 2024
53221df
register all activity
Paul-Bob May 14, 2024
70cd62c
Update app/components/avo/sidebar_component.html.erb
Paul-Bob May 14, 2024
f0ab0a2
Update lib/avo/configuration.rb
Paul-Bob May 14, 2024
f3c5e1c
Update lib/avo/configuration.rb
Paul-Bob May 14, 2024
8840724
Merge branch 'main' into feature/audit_logs
Paul-Bob May 14, 2024
5e614a4
Merge branch 'main' into feature/audit_logs
Paul-Bob Jun 20, 2024
bd07682
rename to avo-audit_logging
Paul-Bob Jun 20, 2024
dc2f288
feature: log attach and detach
Paul-Bob Jun 21, 2024
019ca76
Merge branch 'main' into feature/audit_logs
Paul-Bob Sep 11, 2024
6745d11
Merge branch 'main' into feature/audit_logs
Paul-Bob Sep 11, 2024
cf57fe1
Merge branch 'main' into feature/audit_logs
Paul-Bob Sep 13, 2024
6fc5c46
unbundle audits from enterprise license
Paul-Bob Sep 13, 2024
80b049a
implement gem configuration
Paul-Bob Sep 13, 2024
9595046
rm unused config attribute
Paul-Bob Sep 13, 2024
02d3595
clear template
Paul-Bob Sep 13, 2024
cefc4dd
extract direct call
Paul-Bob Sep 23, 2024
4dca585
remove direct audit call
Paul-Bob Sep 23, 2024
f03c4cc
rm set_paper_trail_whodunnit
Paul-Bob Sep 23, 2024
0d28364
revert safe_call enhancement
Paul-Bob Sep 23, 2024
415c940
Merge branch 'main' into feature/audit_logs
Paul-Bob Sep 23, 2024
d582db0
temporary solution for not defined audit
Paul-Bob Sep 23, 2024
8ed38be
Merge branch 'feature/audit_logs' of github.com:avo-hq/avo into featu…
Paul-Bob Sep 23, 2024
4d8728f
wip
Paul-Bob Sep 23, 2024
bc71d7a
.
Paul-Bob Sep 23, 2024
9db0654
lint
Paul-Bob Sep 23, 2024
e7f14ae
extract audit call
Paul-Bob Sep 24, 2024
6ecaf15
rm activity
Paul-Bob Sep 24, 2024
c1ea7a1
Update lib/tasks/avo_tasks.rake
Paul-Bob Oct 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/components/avo/sidebar_component.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
<% end %>
<% end %>


Paul-Bob marked this conversation as resolved.
Show resolved Hide resolved
<%= render partial: "/avo/partials/sidebar_extra" %>
</div>
</div>
Expand Down
18 changes: 15 additions & 3 deletions app/controllers/avo/actions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,24 @@ def show
def handle
resource_ids = action_params[:fields][:avo_resource_ids].split(",")

query = decrypted_query || (resource_ids.any? ? @resource.find_record(resource_ids, params: params) : [])
fields = action_params[:fields].except(:avo_resource_ids, :avo_selected_query)

audit(
activity_class: @action.class,
payload: {
fields: fields,
resource: resource,
},
action: __method__,
records: query
)

performed_action = @action.handle_action(
fields: action_params[:fields].except(:avo_resource_ids, :avo_selected_query),
fields: fields,
current_user: _current_user,
resource: @resource,
query: decrypted_query ||
(resource_ids.any? ? @resource.find_record(resource_ids, params: params) : [])
query: query
)

@response = performed_action.response
Expand Down
1 change: 1 addition & 0 deletions app/controllers/avo/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class ApplicationController < ::ActionController::Base
before_action :set_view
before_action :set_sidebar_open
before_action :set_stylesheet_assets_path
before_action :set_paper_trail_whodunnit, if: -> { defined? PaperTrail }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we add this ourselves in the audit logging gem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, done


rescue_from Avo::NotAuthorizedError, with: :render_unauthorized
rescue_from ActiveRecord::RecordInvalid, with: :exception_logger
Expand Down
4 changes: 4 additions & 0 deletions app/controllers/avo/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ def create
end

# record gets instantiated and filled in the fill_record method
audit(activity_class: @resource.class, payload: params, action: __method__)
saved = save_record
@resource.hydrate(record: @record, view: :new, user: _current_user)

Expand All @@ -185,6 +186,8 @@ def edit
end

def update
audit(activity_class: @resource.class, payload: params, action: __method__, records: @record)

# record gets instantiated and filled in the fill_record method
saved = save_record
@resource = @resource.hydrate(record: @record, view: :edit, user: _current_user)
Expand All @@ -200,6 +203,7 @@ def update
end

def destroy
audit(activity_class: @resource.class, payload: params, action: __method__, records: @record)
if destroy_model
destroy_success_action
else
Expand Down
17 changes: 17 additions & 0 deletions app/helpers/avo/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,23 @@ def frame_id(resource)
["frame", resource.model_name.singular, resource.record.id].compact.join("-")
end

def audit(activity_class:, payload:, action:, records: [])
return unless Avo.configuration.audit? && activity_class.has_avo_activity

Avo::Current.activity = Avo::Activity.create!(
activity_class: activity_class,
action: action,
author_id: _current_user.id,
author_type: _current_user.class,
payload: payload.to_json
)


Paul-Bob marked this conversation as resolved.
Show resolved Hide resolved
Array(records).each do |record|
Avo::Current.activity.avo_activity_pivots.create!(record: record)
end
end

def chart_color(index)
Avo.configuration.branding.chart_colors[index % Avo.configuration.branding.chart_colors.length]
end
Expand Down
22 changes: 21 additions & 1 deletion lib/avo/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class Configuration
attr_writer :root_path
attr_writer :cache_store
attr_writer :logger
attr_writer :audit
attr_writer :turbo
attr_accessor :timezone
attr_accessor :per_page
Expand Down Expand Up @@ -101,6 +102,7 @@ def initialize
@mount_avo_engines = true
@cache_store = computed_cache_store
@logger = default_logger
@audit = false
@turbo = default_turbo
@default_url_options = []
end
Expand Down Expand Up @@ -160,7 +162,9 @@ def license=(value)
def license
gems = Gem::Specification.map {|gem| gem.name}

@license ||= if gems.include?("avo-advanced")
@license ||= if gems.include?("avo-audits")
"enterprise"
Copy link

Choose a reason for hiding this comment

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

Why? 😢

Copy link

Choose a reason for hiding this comment

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

Can you consider moving the audit feature to the advanced plan?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @yorsant.
Apologies for the late reply.

The audit logging feature is, generally speaking, an enterprise feature.

As much as we want to give features away or make them cheaper, we can't do that. It takes a lot of time and effort to build Avo.
We love the self-serve product (Pro, Advanced) but the reality is that we can't survive and improve our business without having some big customers. We've heard from customers that they saved tens and even hundreds of thousands of dollars by using Avo and that they would like to pay more.
If we don't have any Enterprise features, we can't sell Enterprise subscriptions.

This all comes back to us being able to build a robust business and being able to ship more value through our products.

Thanks for asking the question.

Copy link

Choose a reason for hiding this comment

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

FWIW you can build audit log yourself using papertrail and avo resources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This feature will not be bundled in a gem, so let's not force the license to enterprise.

elsif gems.include?("avo-advanced")
"advanced"
elsif gems.include?("avo-pro")
"pro"
Expand Down Expand Up @@ -219,6 +223,22 @@ def default_logger
}
end

def database_exists?
ActiveRecord::Base.connection
rescue ActiveRecord::NoDatabaseError
false
else
true
end


Paul-Bob marked this conversation as resolved.
Show resolved Hide resolved
Paul-Bob marked this conversation as resolved.
Show resolved Hide resolved
def audit?
Paul-Bob marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see where we use this method, probably in the gem itself?
If so, how about moving it there so there's little noise about it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm moving the audit configuration to the gem and will revert this change, thanks for pointing it

Avo.plugin_manager.installed?("avo-audits") &&
@audit &&
database_exists? &&
ActiveRecord::Base.connection.table_exists?(:avo_activities)
end

def turbo
Avo::ExecutionContext.new(target: @turbo).handle
end
Expand Down
1 change: 1 addition & 0 deletions lib/avo/current.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class Avo::Current < ActiveSupport::CurrentAttributes
attribute :tool_manager
attribute :plugin_manager
attribute :locale
attribute :activity

# The tenant attributes are here so the user can add them on their own will
attribute :tenant_id
Expand Down
3 changes: 3 additions & 0 deletions lib/generators/avo/templates/initializer/avo.tt
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ Avo.configure do |config|
# file_logger
# }

## == Audit ==
# config.audit = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's think about the configuration of this gem a bit more. true/false is a bit terse and we'll for sure have more configs on that gem.
How about moving this config as config.enabled = true|false on that gem's configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about moving this config as config.enabled = true|false on that gem's configuration?

Well, that's a really good idea to have a configuration file for each gem!


## == Customization ==
# config.app_name = 'Avocadelicious'
# config.timezone = 'UTC'
Expand Down
2 changes: 1 addition & 1 deletion lib/tasks/avo_tasks.rake
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ task "avo:sym_link" do
`touch #{packages_path}/.keep`
end

["avo-advanced", "avo-pro", "avo-dynamic_filters", "avo-dashboards", "avo-menu"].each do |gem|
["avo-advanced", "avo-pro", "avo-dynamic_filters", "avo-dashboards", "avo-menu", "avo-audits"].each do |gem|
path = `bundle show #{gem} 2> /dev/null`.chomp

# If path is emty we check if package is defined outside of root (on release process it is)
Expand Down
Loading