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

Add uniqueness of service for generation chargeback report for SSUI #17082

Merged
merged 1 commit into from
Mar 5, 2018

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Mar 5, 2018

Chargeback report in summary dashboard screen is calculated
from data from this API request:

/api/services?expand=resources&filter[]=service_id%3Dnil&attributes=chargeback_report

and this request is returing data from each service let's say in this structure:
(Assume that we 3 different service with same name but with different ID)

service1:
  chargeback_data_for_service_my_service

service2:
  chargeback_data_for_service_my_service

service3:
  chargeback_data_for_service_my_service

but for each service has been used
same name of report because services could be
named by same name.

So when there is generated result for service3 and
this result is empty then in final API request
will have all services empty result too because
report results from service1 and service2 have been
overwritten by empty result from service 3 -
thanks to the distinguishing by just name.

So I am adding Service#id for distiguishing and
ensuring uniqueness to fetch correct chargeback
report result for each service.

Links

@miq-bot assign @gtanzillo

@yrudman can you review it please ?

@miq-bot add_label chargeback, bug, gaprindashvili/yes

Chargeback report in summary dashboard screen is calculated
from data from this API request:
/api/services?expand=resources&filter[]=service_id%3Dnil&attributes=chargeback_report

and this request is returing data from each service

service1:
  chargeback_data_for_service_my_service

service2:
  chargeback_data_for_service_my_service

service3:
  chargeback_data_for_service_my_service

but for each service has been used
same name of report because Services could be
named by same name.

So when there is generated result for service3 and
this result is empty then in final API request
will have all services empty result as well because
report results from service1 and 2 have been
overwriten by empty result from service 3 -
thanks to the distiguishing by just name.

So I am adding Service#id for distiguishing and
ensuring uniqueness.
@lpichler lpichler changed the title Add uniqueness for generation chargeback report for SSUI Add uniqueness of service for generation chargeback report for SSUI Mar 5, 2018
@miq-bot
Copy link
Member

miq-bot commented Mar 5, 2018

Checked commit lpichler@dfed1c2 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@yrudman
Copy link
Contributor

yrudman commented Mar 5, 2018

It looks like very corner case to have several services with the same name, but in case it would happen - we will be prepared.
The only small drawback to this fix is having internal servcie_id added to the title here https://github.com/lpichler/manageiq/blob/dfed1c2f1a81d057fe4e018a58d36dc4a1143c12/app/models/service.rb#L390 even if we do not have services with the same name.
Does it make sense to keep the same title of report (without added service id), what do you think ?

@lpichler
Copy link
Contributor Author

lpichler commented Mar 5, 2018

Does it make sense to keep the same title of report (without added service id), what do you think ?

I think that it is ok to have id in report name. Or maybe I would like to have only ID here because it is unique and unchangeable identifier.

  1. Are you ok with adding just Service#id ?
  2. Now As I am looking on it looks like that MiqReportResult#name is using MiqReport#title because my code is not changing MiqReportResult#name (I think) - this I need to check.

@yrudman
Copy link
Contributor

yrudman commented Mar 5, 2018

@lpichler you are correct, my previous suggestion is wrong, reports's name is using title:

all looks good to me
👍

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gtanzillo gtanzillo added this to the Sprint 81 Ending Mar 12, 2018 milestone Mar 5, 2018
@gtanzillo gtanzillo merged commit c2f76b6 into ManageIQ:master Mar 5, 2018
@lpichler lpichler deleted the fix_service_uniqueness branch March 7, 2018 17:07
simaishi pushed a commit that referenced this pull request Mar 7, 2018
Add uniqueness of service for generation chargeback report for SSUI
(cherry picked from commit c2f76b6)

https://bugzilla.redhat.com/show_bug.cgi?id=1552817
@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit a75d746056af0084ca84fd55209d5119f1e29f46
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Mar 5 15:51:11 2018 -0500

    Merge pull request #17082 from lpichler/fix_service_uniqueness
    
    Add uniqueness of service for generation chargeback report for SSUI
    (cherry picked from commit c2f76b64e346123c343e1c58fbcaa3d97260ce44)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552817

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.

5 participants