-
Notifications
You must be signed in to change notification settings - Fork 991
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
Basic usage report implementation with reports #10306
base: develop
Are you sure you want to change the base?
Conversation
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.
Would you mind splitting off the jail inheritance change to its own commit (or even own PR)? It took me a moment to understand what was going on.
@@ -56,7 +56,7 @@ class Domain < ApplicationRecord | |||
prop_group :basic_model_props, ApplicationRecord, meta: { example: 'example.com' } | |||
property :fullname, String, desc: 'User name for this domain, e.g. "Primary domain for our company"' | |||
end | |||
class Jail < Safemode::Jail | |||
class Jail < Jail | |||
allow :id, :name, :fullname |
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.
If you're inheriting, I think this is sufficient
allow :id, :name, :fullname | |
allow :fullname |
I think I will split this PR into multiple ones that can be merged regardless to this effort. For example this one should be split and applied to all the models, and not just for the two that I have done it for. I think we need to decide which improvements we want as the base implementation, and at least theoretically, we can split the improvements into separate PRs once we want to get that info in the report. |
This is a base implementation of usage report as a report template.
It shows what changes are required to helpers framework to become suitable for aggregation reports.