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

Generate ChargeBack report by Tenant #7433

Merged
merged 2 commits into from Mar 29, 2016

Conversation

aljesusg
Copy link
Member

After have assignment to tenant #7202 we need make a chargeback report by tenant.

Description

This pull request provides a chargeback report by tenant

Now we can show costs by Tenant

captura de pantalla 2016-03-21 a las 16 26 03

And select the tenant that we wanna get the report

captura de pantalla 2016-03-21 a las 16 26 15

### Working with

@lpichler

More Information

Taiga:
https://tree.taiga.io/project/sergioocon-charging-in-manageiq/task/40

@aljesusg
Copy link
Member Author

@miq-bot add-label wip
@miq-bot add-label chargeback
@miq-bot add-label reporting

@chessbyte chessbyte removed the wip label Mar 21, 2016
@aljesusg aljesusg changed the title Generate ChargeBack report by Tenant [WIP] Generate ChargeBack report by Tenant Mar 21, 2016
@aljesusg
Copy link
Member Author

@miq-bot add-label wip

@miq-bot miq-bot added the wip label Mar 21, 2016
@aljesusg aljesusg changed the title [WIP] Generate ChargeBack report by Tenant Generate ChargeBack report by Tenant Mar 21, 2016
@aljesusg
Copy link
Member Author

@miq-bot remove-label wip

@miq-bot miq-bot removed the wip label Mar 21, 2016
@@ -373,6 +373,7 @@ def check_tabs
elsif @edit[:new][:model] == "Chargeback"
unless @edit[:new][:cb_show_typ] &&
((@edit[:new][:cb_show_typ] == "owner" && @edit[:new][:cb_owner_id]) ||
(@edit[:new][:cb_show_typ] == "tenant" && @edit[:new][:cb_tenant_id]) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

@aljesusg can you move this huge condition (and if it is possible change it somehow for better readibility) to a method valid_chargeback_fields
then we can get here

elsif @edit[:new][:model] == "Chargeback" && !valid_chargeback_fields
     add_flash..
     ....

what do you think ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok @lpichler It's fine. I am working on it. I have 2 proposal that you can check here https://tree.taiga.io/project/sergioocon-charging-in-manageiq/task/40 but Rubocop throw a complexity errors :(

  # Check if chargeback field is valid
  VALID_SHOW_TYP_FIELDS = %w(owner tenant tag).freeze
  def valid_chargeback_fields
    if @edit[:new][:cb_show_typ] && VALID_SHOW_TYP_FIELDS.include?(@edit[:new][:cb_show_typ])
      case @edit[:new][:cb_show_typ]
      when "owner"
        return true if @edit[:new][:cb_owner_id]
      when "tenant"
        return true if @edit[:new][:cb_tenant_id]
      when "tag"
        return true if @edit[:new][:cb_tag_cat] && @edit[:new][:cb_tag_value]
      end
    end
    false
  end
  # Check if chargeback field is valid
  VALID_SHOW_TYP_FIELDS = %w(owner tenant tag).freeze
  def valid_chargeback_fields
    if @edit[:new][:cb_show_typ] && VALID_SHOW_TYP_FIELDS.include?(@edit[:new][:cb_show_typ])
      case @edit[:new][:cb_show_typ]
      when "owner" then return true if @edit[:new][:cb_owner_id]
      when "tenant" then return true if @edit[:new][:cb_tenant_id]
      when "tag" then return true if @edit[:new][:cb_tag_cat] && @edit[:new][:cb_tag_value]
      end
    end
    false
  end

Copy link
Contributor

Choose a reason for hiding this comment

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

@aljesusg yes, much better - I am for 1.,and I am suggesting also something like this:

 # Check if chargeback field is valid
  def valid_chargeback_fields
    is_valid = false
    if %w(owner tenant tag).include?(@edit[:new][:cb_show_typ]) 
      is_valid = case @edit[:new][:cb_show_typ]
                       when "owner"  then @edit[:new][:cb_owner_id]
                       when "tenant" then @edit[:new][:cb_tenant_id]
                       when "tag"    then @edit[:new][:cb_tag_cat] && @edit[:new][:cb_tag_value]
    end
    is_valid
  end

inspired by https://github.com/bbatsov/ruby-style-guide and find on this page When assigning the result of a conditional expression to a variable

maybe this will rubocop accept :)

@lpichler
Copy link
Contributor

@aljesusg I was able to save report when "Show costs by tenant" was selected(= 'Tenant') but "Tenant" select box was unselected (= 'choose tenant')
so can you validate also this case?
it seems that change need to be done in this method:
https://github.com/aljesusg/manageiq/blob/generate_report_on_tenant/app/controllers/report_controller/reports.rb#L223

@lpichler
Copy link
Contributor

@aljesusg

I was looking into db how it is stored: from MiqReport#db_options:

---
:rpt_type: chargeback
:options:
  :interval: daily
  :interval_size: 1
  :end_interval_offset: 1
  :tenant: '10000000000002'

so there will be :tenant_id instead of :tenant and can be tenant_id stored as integer ? so maybe it will be better to have:

---
:rpt_type: chargeback
:options:
  :interval: daily
  :interval_size: 1
  :end_interval_offset: 1
  :tenant_id: 10000000000002

Do you agree ?

@lpichler
Copy link
Contributor

@aljesusg otherwise looks good to me !

It will be also nice to write spec for generating report to ensure if it considers only VMs which belongs to any tenant or basically test method build_results_for_report_chargeback

@gtanzillo can you review it also, please ?

@gtanzillo
Copy link
Member

@aljesusg Looks good if you make the changes @lpichler suggested, especially adding a test for the report.

@aljesusg aljesusg force-pushed the generate_report_on_tenant branch 2 times, most recently from d99f1ae to 8ba6ca3 Compare March 23, 2016 09:19
Update editor.rb

Improve function valid chargeback fields
@miq-bot
Copy link
Member

miq-bot commented Mar 24, 2016

Checked commits aljesusg/manageiq@38a1214~...53650dd with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
5 files checked, 0 offenses detected
Everything looks good. 🍪

@aljesusg
Copy link
Member Author

Done !!!
Thanks @lpichler & @gtanzillo

@chessbyte chessbyte merged commit 76bc59a into ManageIQ:master Mar 29, 2016
@chessbyte chessbyte added this to the Sprint 38 Ending Mar 28, 2016 milestone Mar 29, 2016
@aljesusg aljesusg deleted the generate_report_on_tenant branch March 29, 2016 06:19
@aljesusg
Copy link
Member Author

Thanks @lpichler , @gtanzillo & @chessbyte

@aljesusg aljesusg mentioned this pull request Mar 30, 2016
@chargio chargio mentioned this pull request Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants