Fixes #4151 - Reports STI #2530

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@shlomizadok
Member

shlomizadok commented Jul 9, 2015

No description provided.

@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot

theforeman-bot Jul 9, 2015

Member

There were the following issues with the commit message:

  • 537f01e must be in the format Fixes/refs #redmine_number - brief description.

Guidelines are available on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

Member

theforeman-bot commented Jul 9, 2015

There were the following issues with the commit message:

  • 537f01e must be in the format Fixes/refs #redmine_number - brief description.

Guidelines are available on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

app/models/puppet_report.rb
@@ -0,0 +1,20 @@
+class PuppetReport < Report
+ def self.model_name
+ superclass.model_name

This comment has been minimized.

@ohadlevy

ohadlevy Jul 9, 2015

Member

why is that needed? IMHO you can use include Foreman::STI to get rid of most related issues?

@ohadlevy

ohadlevy Jul 9, 2015

Member

why is that needed? IMHO you can use include Foreman::STI to get rid of most related issues?

This comment has been minimized.

@ohadlevy

ohadlevy Jul 9, 2015

Member

ah, you use it already, is it needed?

@ohadlevy

ohadlevy Jul 9, 2015

Member

ah, you use it already, is it needed?

This comment has been minimized.

@ohadlevy

ohadlevy Jul 9, 2015

Member

actually i take it back, please ignore :)

@ohadlevy

ohadlevy Jul 9, 2015

Member

actually i take it back, please ignore :)

app/models/report.rb
def self.report_status
"status"
end
end
+
+require_dependency 'puppet_report'

This comment has been minimized.

@ohadlevy

ohadlevy Jul 9, 2015

Member

maybe add it to config.autoload_paths under config/application.rb?

@ohadlevy

ohadlevy Jul 9, 2015

Member

maybe add it to config.autoload_paths under config/application.rb?

@@ -0,0 +1,5 @@
+class AddTypeToReports < ActiveRecord::Migration
+ def change
+ add_column :reports, :type, :string, :default => 'PuppetReport'

This comment has been minimized.

@ohadlevy

ohadlevy Jul 9, 2015

Member

two issues:

  1. what about salt/chef reports? they would also be migrated to puppet report?
  2. type should be null => false
@ohadlevy

ohadlevy Jul 9, 2015

Member

two issues:

  1. what about salt/chef reports? they would also be migrated to puppet report?
  2. type should be null => false

This comment has been minimized.

This comment has been minimized.

@stbenjam

stbenjam Jul 9, 2015

Member

I would prefer if there was a generic ConfigReport that has the general idea behind a config management system: system status, resources, results, etc, and allow config management platforms to extend it if necessary. Does that make sense?

That would let puppet/salt/chef customize it, while still providing the base data to be the source of data for the host status, dashboard, etc.

@stbenjam

stbenjam Jul 9, 2015

Member

I would prefer if there was a generic ConfigReport that has the general idea behind a config management system: system status, resources, results, etc, and allow config management platforms to extend it if necessary. Does that make sense?

That would let puppet/salt/chef customize it, while still providing the base data to be the source of data for the host status, dashboard, etc.

@domcleal domcleal changed the title from [WIP][Don't Merge] Reports STI to [WIP][Don't Merge] fixes #4151 - Reports STI Jul 9, 2015

@shlomizadok shlomizadok changed the title from [WIP][Don't Merge] fixes #4151 - Reports STI to Fixes #4151 - Reports STI Jul 12, 2015

app/models/report.rb
def self.report_status
"status"
end
end
+
+require_dependency 'config_report'

This comment has been minimized.

@ohadlevy

ohadlevy Jul 12, 2015

Member

I don't think this should be needed. autoloader should handle it afaik

@ohadlevy

ohadlevy Jul 12, 2015

Member

I don't think this should be needed. autoloader should handle it afaik

This comment has been minimized.

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

It is required for dev env, if you call Report.all in dev env before ConfigReport was loaded the STI does not work correctly.

@ares

ares Jul 14, 2015

Member

It is required for dev env, if you call Report.all in dev env before ConfigReport was loaded the STI does not work correctly.

@@ -0,0 +1,5 @@
+class AddTypeToReports < ActiveRecord::Migration
+ def change
+ add_column :reports, :type, :string, :default => 'ConfigReport'

This comment has been minimized.

@ohadlevy

ohadlevy Jul 12, 2015

Member

null => false?

@ohadlevy

ohadlevy Jul 12, 2015

Member

null => false?

This comment has been minimized.

@shlomizadok

shlomizadok Jul 12, 2015

Member

is null => false needed if :default => 'ConfigReport' ?

@shlomizadok

shlomizadok Jul 12, 2015

Member

is null => false needed if :default => 'ConfigReport' ?

app/services/report_importer.rb
+ # to be defined in children
+ def report_name_class
+ # to raise NotImplementedError ?
+ Foreman::Deprecation.deprecation_warning(1.12, "This shit is deprecated, wtf are you doing?")

This comment has been minimized.

@domcleal

domcleal Jul 13, 2015

Contributor

Not at all appropriate, please remove it and fix the question above.

@domcleal

domcleal Jul 13, 2015

Contributor

Not at all appropriate, please remove it and fix the question above.

app/models/config_report.rb
+ ConfigReportImporter.import(report, proxy_id)
+ end
+
+ def summaryStatus

This comment has been minimized.

@domcleal

domcleal Jul 13, 2015

Contributor

I'd suggest changing this to underscores from camel case if you're moving it.

@domcleal

domcleal Jul 13, 2015

Contributor

I'd suggest changing this to underscores from camel case if you're moving it.

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Jul 13, 2015

Contributor

Please address any questions/TODOs in the code, there are a few.

Contributor

domcleal commented Jul 13, 2015

Please address any questions/TODOs in the code, there are a few.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Jul 13, 2015

Member

Addressed the questions and changed (😰 hope all is well)

Member

shlomizadok commented Jul 13, 2015

Addressed the questions and changed (😰 hope all is well)

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Jul 13, 2015

Member

Added @ohadlevy comments too

Member

shlomizadok commented Jul 13, 2015

Added @ohadlevy comments too

app/views/api/v1/reports/show.json.rabl
@@ -13,5 +13,5 @@ child :logs do
end
node :summary do |report|
- report.summaryStatus
+ report.summary_status

This comment has been minimized.

@tstrachota

tstrachota Jul 14, 2015

Member

Wrong indentation, there's tab instead of 2 spaces.

@tstrachota

tstrachota Jul 14, 2015

Member

Wrong indentation, there's tab instead of 2 spaces.

app/models/report.rb
@@ -82,7 +83,9 @@ def runtime
end
def self.import(report, proxy_id = nil)
- ReportImporter.import(report, proxy_id)
+ # to raise NotImplementedError ?
+ Foreman::Deprecation.deprecation_warning(1.12, "Report model has turned to be STI, please use child classes")

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

I think the version should be string

@ares

ares Jul 14, 2015

Member

I think the version should be string

This comment has been minimized.

@shlomizadok

shlomizadok Jul 14, 2015

Member

Works as an integer too :)

@shlomizadok

shlomizadok Jul 14, 2015

Member

Works as an integer too :)

+ def self.report_status
+ "status"
+ end
+end

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

missing new line at the end of file

@ares

ares Jul 14, 2015

Member

missing new line at the end of file

app/services/config_report_importer.rb
@@ -0,0 +1,9 @@
+class ConfigReportImporter < ReportImporter
+ def self.authorized_smart_proxy_features
+ ['Puppet', 'Chef Proxy']

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

I think don't need 'Chef Proxy', since it's called 'Chef' and it's defined by chef plugins. It should be dropped from seeds as well (saying it's out of scope for this PR is OK)

EDIT: saying that I think we need API to add more features here, e.g. Chef or Salt

@ares

ares Jul 14, 2015

Member

I think don't need 'Chef Proxy', since it's called 'Chef' and it's defined by chef plugins. It should be dropped from seeds as well (saying it's out of scope for this PR is OK)

EDIT: saying that I think we need API to add more features here, e.g. Chef or Salt

This comment has been minimized.

@shlomizadok

shlomizadok Jul 14, 2015

Member

Original importer had Puppet and Chef feature.
Other importers (features) can inherit from ReportImporter and add their own feature?

@shlomizadok

shlomizadok Jul 14, 2015

Member

Original importer had Puppet and Chef feature.
Other importers (features) can inherit from ReportImporter and add their own feature?

app/services/report_importer.rb
+
+ def self.authorized_smart_proxy_features
+ Rails.logger.debug("Importer #{self} does not implement authorized_smart_proxy_features.")
+ Foreman::Deprecation.deprecation_warning(1.12, "ReportImporter is a superclass, please use child classes. Maybe try: ConfigReportImporter?")

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

1.12 should be string I think

@ares

ares Jul 14, 2015

Member

1.12 should be string I think

This comment has been minimized.

@shlomizadok

shlomizadok Jul 14, 2015

Member

ditto, integer seems ok

@shlomizadok

shlomizadok Jul 14, 2015

Member

ditto, integer seems ok

This comment has been minimized.

@domcleal

domcleal Jul 14, 2015

Contributor

That's a float, not an int, so extra reason to avoid it. The intention was for it to be a string, but the class doesn't enforce it, thought it probably would.

@domcleal

domcleal Jul 14, 2015

Contributor

That's a float, not an int, so extra reason to avoid it. The intention was for it to be a string, but the class doesn't enforce it, thought it probably would.

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

yup, float was my concern

@ares

ares Jul 14, 2015

Member

yup, float was my concern

app/services/report_importer.rb
- ['Puppet', 'Chef Proxy']
+ # to be defined in children
+ def report_name_class
+ Foreman::Deprecation.deprecation_warning(1.12, "ReportImport is a supperclass, please use child classes to define 'report_class_name'")

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

1.12 should be string I think, also I'm wondering if any plugin uses this class directly, maybe we don't need to keep backwards compatibility here

@ares

ares Jul 14, 2015

Member

1.12 should be string I think, also I'm wondering if any plugin uses this class directly, maybe we don't need to keep backwards compatibility here

@@ -132,13 +135,6 @@ def no_report
false
end
- def summaryStatus

This comment has been minimized.

@ares

ares Jul 14, 2015

Member

just thank you for renaming!

@ares

ares Jul 14, 2015

Member

just thank you for renaming!

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Jul 14, 2015

Member

Stringified deprecation warning version number. Changed "Chef Proxy" to "Chef" + migration to existing installations. (Please note there was a duplication with https://github.com/theforeman/foreman_chef/blob/master/db/seeds.rb#L1)

Member

shlomizadok commented Jul 14, 2015

Stringified deprecation warning version number. Changed "Chef Proxy" to "Chef" + migration to existing installations. (Please note there was a duplication with https://github.com/theforeman/foreman_chef/blob/master/db/seeds.rb#L1)

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Jul 14, 2015

Member

re[test] due to un-related Katello failure

Member

shlomizadok commented Jul 14, 2015

re[test] due to un-related Katello failure

app/services/config_report_importer.rb
+ def report_name_class
+ ConfigReport
+ end
+end

This comment has been minimized.

@ares

ares Jul 15, 2015

Member

missing new line at the end of file

@ares

ares Jul 15, 2015

Member

missing new line at the end of file

This comment has been minimized.

@shlomizadok

shlomizadok Jul 15, 2015

Member

Fixed. Thanks :)

2015-07-15 14:31 GMT+03:00 Marek Hulán notifications@github.com:

In app/services/config_report_importer.rb
#2530 (comment):

@@ -0,0 +1,9 @@
+class ConfigReportImporter < ReportImporter

  • def self.authorized_smart_proxy_features
  • ['Puppet', 'Chef']
  • end
  • def report_name_class
  • ConfigReport
  • end
    +end

missing new line at the end of file


Reply to this email directly or view it on GitHub
https://github.com/theforeman/foreman/pull/2530/files#r34669168.

@shlomizadok

shlomizadok Jul 15, 2015

Member

Fixed. Thanks :)

2015-07-15 14:31 GMT+03:00 Marek Hulán notifications@github.com:

In app/services/config_report_importer.rb
#2530 (comment):

@@ -0,0 +1,9 @@
+class ConfigReportImporter < ReportImporter

  • def self.authorized_smart_proxy_features
  • ['Puppet', 'Chef']
  • end
  • def report_name_class
  • ConfigReport
  • end
    +end

missing new line at the end of file


Reply to this email directly or view it on GitHub
https://github.com/theforeman/foreman/pull/2530/files#r34669168.

app/services/report_importer.rb
+
+ def self.authorized_smart_proxy_features
+ Rails.logger.debug("Importer #{self} does not implement authorized_smart_proxy_features.")
+ Foreman::Deprecation.deprecation_warning('1.12', "ReportImporter is a superclass, please use child classes. Maybe try: ConfigReportImporter?")

This comment has been minimized.

@domcleal

domcleal Jul 16, 2015

Contributor

I don't understand the deprecation warnings here - they're going to fire all the time with the ReportsController and I don't see what replaces them. The controller is always going to call ReportImporter.authorized_smart_proxy_features, not some Report or ReportImporter subclass.

They're also for a new method, which doesn't make a lot of sense.

@domcleal

domcleal Jul 16, 2015

Contributor

I don't understand the deprecation warnings here - they're going to fire all the time with the ReportsController and I don't see what replaces them. The controller is always going to call ReportImporter.authorized_smart_proxy_features, not some Report or ReportImporter subclass.

They're also for a new method, which doesn't make a lot of sense.

This comment has been minimized.

@domcleal

domcleal Jul 16, 2015

Contributor

Hm, possibly ignore this, I think the issue might be in the controller.

@domcleal

domcleal Jul 16, 2015

Contributor

Hm, possibly ignore this, I think the issue might be in the controller.

@@ -7,7 +7,7 @@ class ReportsController < V2::BaseController
before_filter :find_resource, :only => %w{show update destroy}
before_filter :setup_search_options, :only => [:index, :last]
- add_smart_proxy_filters :create, :features => ReportImporter.report_features
+ add_smart_proxy_filters :create, :features => ReportImporter.authorized_smart_proxy_features

This comment has been minimized.

@domcleal

domcleal Jul 16, 2015

Contributor

Oh, regarding my comment about deprecation of authorized_smart_proxy_features, did you mean to actually keep ReportImporter.report_features here which calls that method on each of the known report importers?

@domcleal

domcleal Jul 16, 2015

Contributor

Oh, regarding my comment about deprecation of authorized_smart_proxy_features, did you mean to actually keep ReportImporter.report_features here which calls that method on each of the known report importers?

This comment has been minimized.

@domcleal

domcleal Jul 20, 2015

Contributor

Still an issue, or use ConfigReportImporter specifically?

@domcleal

domcleal Jul 20, 2015

Contributor

Still an issue, or use ConfigReportImporter specifically?

+class RenameChefProxy < ActiveRecord::Migration
+ def up
+ chef_feature = Feature.find_by_name("Chef Proxy")
+ chef_feature.update_attribute(:name, "Chef") if chef_feature

This comment has been minimized.

@domcleal

domcleal Jul 16, 2015

Contributor

If foreman_chef's loaded, this might lead to a duplicate - should we delete Chef Proxy if it's there, leaving the plugin to define Chef?

@domcleal

domcleal Jul 16, 2015

Contributor

If foreman_chef's loaded, this might lead to a duplicate - should we delete Chef Proxy if it's there, leaving the plugin to define Chef?

+ end
+
+ def down
+ end

This comment has been minimized.

@domcleal

domcleal Jul 16, 2015

Contributor

todo?

@domcleal

domcleal Jul 16, 2015

Contributor

todo?

db/seeds.d/11-smart_proxy_features.rb
@@ -1,5 +1,5 @@
# Proxy features
-[ "TFTP", "DNS", "DHCP", "Puppet", "Puppet CA", "BMC", "Chef Proxy", "Realm", "Facts" ].each do |input|
+[ "TFTP", "DNS", "DHCP", "Puppet", "Puppet CA", "BMC", "Chef", "Realm", "Facts" ].each do |input|

This comment has been minimized.

@domcleal

domcleal Jul 16, 2015

Contributor

Time to remove this altogether, see above?

@domcleal

domcleal Jul 16, 2015

Contributor

Time to remove this altogether, see above?

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Aug 24, 2015

Contributor

In foreman, as well as in ruby -Itest test/unit/report_test.rb they work flawlessly. It fails for me only under rake:test with NoMethodError: undefined method `persisted?' for nil:NilClass.

Report tests are failing: http://ci.theforeman.org/job/test_develop_pr_core/5973/,

The test that's failing is a host unit test (of report imports), not a report unit test. I put an inline comment where I think it's probably failing, due to a short-circuit in the report importer code.

Contributor

domcleal commented Aug 24, 2015

In foreman, as well as in ruby -Itest test/unit/report_test.rb they work flawlessly. It fails for me only under rake:test with NoMethodError: undefined method `persisted?' for nil:NilClass.

Report tests are failing: http://ci.theforeman.org/job/test_develop_pr_core/5973/,

The test that's failing is a host unit test (of report imports), not a report unit test. I put an inline comment where I think it's probably failing, due to a short-circuit in the report importer code.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Aug 24, 2015

Member

@domcleal Thank you so much!!! I was pulling my hear around this issue.
Short-circuit fixed

Member

shlomizadok commented Aug 24, 2015

@domcleal Thank you so much!!! I was pulling my hear around this issue.
Short-circuit fixed

app/models/report.rb
- count = Report.where(cond).delete_all
- logger.info Time.now.to_s + ": Expired #{count} Reports"
+ count = self.where(cond).delete_all
+ logger.info Time.now.to_s + ": Expired #{count} #{self.to_s.underscore.humanize.pluralize}"

This comment has been minimized.

@domcleal

domcleal Aug 28, 2015

Contributor

self is superfluous on these two lines I think

@domcleal

domcleal Aug 28, 2015

Contributor

self is superfluous on these two lines I think

app/services/config_report_importer.rb
@@ -0,0 +1,23 @@
+class ConfigReportImporter < ReportImporter
+ attr_reader :report

This comment has been minimized.

@domcleal

domcleal Aug 28, 2015

Contributor

superfluous if it's in the parent?

@domcleal

domcleal Aug 28, 2015

Contributor

superfluous if it's in the parent?

- map.permission :upload_reports, {:"api/v2/reports" => [:create] }
+ permission_set.security_block :config_reports do |map|
+ map.permission :view_config_reports, {:"api/v1/reports" => [:index, :show, :last],
+ :"api/v2/reports" => [:index, :show, :last],

This comment has been minimized.

@domcleal

domcleal Aug 28, 2015

Contributor

reindent these lines to match the style, please

@domcleal

domcleal Aug 28, 2015

Contributor

reindent these lines to match the style, please

@@ -0,0 +1,13 @@
+class ChangeReportPermissions < ActiveRecord::Migration
+ PERMISSIONS = %w(view_reports destroy_reports upload_reports)
+ def up

This comment has been minimized.

@domcleal

domcleal Aug 28, 2015

Contributor

missing down?

@domcleal

domcleal Aug 28, 2015

Contributor

missing down?

db/seeds.d/03-permissions.rb
- ['Report', 'view_reports'],
- ['Report', 'destroy_reports'],
- ['Report', 'upload_reports'],
+ ['ConfigReport', 'view_config_reports'],

This comment has been minimized.

@domcleal

domcleal Aug 28, 2015

Contributor

This file's alphabetical, please fix the position

@domcleal

domcleal Aug 28, 2015

Contributor

This file's alphabetical, please fix the position

This comment has been minimized.

@shlomizadok

shlomizadok Aug 28, 2015

Member

The whole file's abc ordering seems off. Fixing globally

@shlomizadok

shlomizadok Aug 28, 2015

Member

The whole file's abc ordering seems off. Fixing globally

@@ -10,7 +10,7 @@ class ReportImporterTest < ActiveSupport::TestCase
end
test 'it should import reports with no metrics' do
- r = ReportImporter.import(read_json_fixture('report-empty.json'))
+ r = ConfigReportImporter.import(read_json_fixture('report-empty.json'))

This comment has been minimized.

@domcleal

domcleal Aug 28, 2015

Contributor

Wrong class?

@domcleal

domcleal Aug 28, 2015

Contributor

Wrong class?

This comment has been minimized.

@shlomizadok

shlomizadok Aug 28, 2015

Member

No, ReportImporter will raise NotImplementedError

@shlomizadok

shlomizadok Aug 28, 2015

Member

No, ReportImporter will raise NotImplementedError

+ @host = FactoryGirl.create(:host, :owner => @owner)
+ end
+
+ test 'when owner is subscribed to notification, a mail should be sent on error' do

This comment has been minimized.

@domcleal

domcleal Aug 28, 2015

Contributor

Shouldn't these be in the base class test, since that's where notifications are?

@domcleal

domcleal Aug 28, 2015

Contributor

Shouldn't these be in the base class test, since that's where notifications are?

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Aug 28, 2015

Contributor

There's still quite a bit of support for metrics (applied, changed, failed_restarts etc) in Report that looks rather config management specific. How will these be used in other subclasses? Should they remain here or move - or would subclasses just use the fields that are most applicable to them?

Contributor

domcleal commented Aug 28, 2015

There's still quite a bit of support for metrics (applied, changed, failed_restarts etc) in Report that looks rather config management specific. How will these be used in other subclasses? Should they remain here or move - or would subclasses just use the fields that are most applicable to them?

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Aug 28, 2015

Member

@domcleal - I was expecting to something like:
merged as 23e5g7h thanks @shlomizadok
However you've got valid comments / questions. Will attend to them later today / Sunday
Thanks!

Member

shlomizadok commented Aug 28, 2015

@domcleal - I was expecting to something like:
merged as 23e5g7h thanks @shlomizadok
However you've got valid comments / questions. Will attend to them later today / Sunday
Thanks!

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Aug 28, 2015

Member

regarding #2530 (comment) - I am not sure.
I'd say the latter - subclasses use the field that are most applicable to them. What says you?

Member

shlomizadok commented Aug 28, 2015

regarding #2530 (comment) - I am not sure.
I'd say the latter - subclasses use the field that are most applicable to them. What says you?

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Aug 28, 2015

Member

@domcleal - fixed the comments above, but #2530 (comment) - please instruct me how to proceed there. Thanks

Member

shlomizadok commented Aug 28, 2015

@domcleal - fixed the comments above, but #2530 (comment) - please instruct me how to proceed there. Thanks

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Aug 31, 2015

Member

@domcleal - came to think of it and I'd like to keep the metrics in the current place so other subclasses may use them. If we will see that this is not the case and only config_report uses them we'll refactor on a separate pull request.

Member

shlomizadok commented Aug 31, 2015

@domcleal - came to think of it and I'd like to keep the metrics in the current place so other subclasses may use them. If we will see that this is not the case and only config_report uses them we'll refactor on a separate pull request.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 2, 2015

Member

Manually added #2644. Refactored to reports STI. @elobato, mind taking a look at the changes?

Member

shlomizadok commented Sep 2, 2015

Manually added #2644. Refactored to reports STI. @elobato, mind taking a look at the changes?

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Sep 2, 2015

Contributor

It needs rebasing to include that change, not added as a further commit.

Contributor

domcleal commented Sep 2, 2015

It needs rebasing to include that change, not added as a further commit.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 2, 2015

Member

@domcleal - rebased. thanks

Member

shlomizadok commented Sep 2, 2015

@domcleal - rebased. thanks

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 2, 2015

Member

tests 📗 & rubocop 👮 are 💚

Member

shlomizadok commented Sep 2, 2015

tests 📗 & rubocop 👮 are 💚

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 3, 2015

Member

Rebased

Member

shlomizadok commented Sep 3, 2015

Rebased

@shlomizadok shlomizadok closed this Sep 3, 2015

@shlomizadok shlomizadok deleted the shlomizadok:fix_4151 branch Sep 3, 2015

@ares

This comment has been minimized.

Show comment
Hide comment
@ares

ares Sep 9, 2015

Member

@shlomizadok is this meant to be closed?

Member

ares commented Sep 9, 2015

@shlomizadok is this meant to be closed?

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
Member

shlomizadok commented Sep 9, 2015

@ares yes

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