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.

Sign up for GitHub

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

Jump to bottom

feature: audit logs #2703

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

feature: audit logs #2703

wants to merge 35 commits into from

Conversation

Paul-Bob
Copy link
Contributor

@Paul-Bob Paul-Bob commented Apr 18, 2024 •
edited
Loading

Description

Fixes #1328

Audit logging calls.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@Paul-Bob Paul-Bob marked this pull request as draft April 18, 2024 13:42
@Paul-Bob Paul-Bob self-assigned this Apr 18, 2024
@codeclimate CodeClimate
Copy link

codeclimate bot commented Apr 18, 2024 •
edited
Loading

Code Climate has analyzed commit 02d3595 and detected 0 issues on this pull request.

View more on Code Climate.

github-actions[bot]
github-actions bot reviewed Apr 18, 2024
View reviewed changes
app/helpers/avo/application_helper.rb Outdated Show resolved Hide resolved
lib/avo/configuration.rb Outdated Show resolved Hide resolved
lib/avo/configuration.rb Outdated Show resolved Hide resolved
yorsant
yorsant reviewed Apr 26, 2024
View reviewed changes
lib/avo/configuration.rb Outdated
@@ -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.

@github-actions GitHub Actions
Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

@github-actions github-actions bot added the Stale label May 12, 2024
@Paul-Bob Paul-Bob removed the Stale label May 13, 2024
Paul-Bob
Paul-Bob commented May 14, 2024
View reviewed changes
lib/avo/configuration.rb Outdated Show resolved Hide resolved
Paul-Bob
Paul-Bob commented May 14, 2024
View reviewed changes
lib/avo/configuration.rb Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

@github-actions github-actions bot added the Stale label May 30, 2024
@adrianthedev adrianthedev added Stale exempt Task Something to get done Enterprise and removed Stale labels May 30, 2024
@Paul-Bob Paul-Bob marked this pull request as ready for review September 11, 2024 11:28
adrianthedev
adrianthedev requested changes Sep 12, 2024
View reviewed changes
app/controllers/avo/application_controller.rb
@@ -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?

lib/generators/avo/templates/initializer/avo.tt Outdated
@@ -100,6 +100,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!

lib/avo/configuration.rb Outdated
true
end

def audit?
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

lib/avo/configuration.rb Outdated
@@ -172,7 +174,9 @@ def license=(value)
def license
gems = Gem::Specification.map {|gem| gem.name}

@license ||= if gems.include?("avo-advanced")
@license ||= if gems.include?("avo-audit_logging")
"enterprise"
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.

app/controllers/avo/base_controller.rb
@@ -570,10 +582,6 @@ def set_pagy_locale
@pagy_locale = locale.to_s || Avo.configuration.default_locale || "en"
end

def safe_call(method)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need this? And shouldn't it take more args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you missed this change https://github.com/avo-hq/avo/pull/2703/files#diff-27ec89c6677b158fe3a800b268c9c196b5409f439315240504626f4c8bf81bc7R140

The method was extracted to be accessible in more places

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xeron xeron xeron left review comments

@yorsant yorsant yorsant left review comments

@github-actions GitHub Actions github-actions left review comments

@adrianthedev adrianthedev adrianthedev requested changes

Assignees

@Paul-Bob Paul-Bob

Labels
Enterprise Feature Stale exempt Task Something to get done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Audit logging
4 participants
@Paul-Bob @xeron @adrianthedev @yorsant