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
[REFACTOR] fixes #3205 Extracts report importing logic #929
Conversation
Looks good, I like that it aligns with the facts importer. Could you also move the import tests from the report unit test into one named after this new service class (a la test/unit/facts_importer_test.rb)? |
@ohadlevy a test failed: Failed assertion, no message given. (test_0001_it should true on error? if there were errors) /var/lib/workspace/workspace/test_develop_pull_request/database/mysql/puppet/3.0/ruby/1.8.7/test/unit/report_test.rb:11 |
@domcleal fixed and rebased |
I realize we now have multiple methods in test that parse json fixtures, should probably fix it in another commit |
@ohadlevy not fixed :) |
rebased again, thanks @domcleal |
when Hash | ||
ReportStatusCalculator.new(:counters => st).calculate | ||
else | ||
raise Foreman::Exception(_('Unsupported report status format')) |
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.
This should be N_()
, as we don't want to translate immediately (in order to calculate a static error ID)
@domcleal fixed :) |
[test] |
end | ||
|
||
def initialize(raw) | ||
raise ::Foreman::Exception.new(_('Invalid report')) unless raw.is_a?(Hash) |
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.
N_()
Could you rebase please? :) |
rebased |
This patch extracts all report importing logic into seperate importer and status classes. long term this could allow: 1. aliases methods for async operations 2. different types of reports imports (based on reporting type).
This patch extracts all report importing logic into seperate importer class.
long term this could allow: