Add column report_base_model to ChargebackRate table

[hstastna]

Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1581817

What:
We need to store the type of the Rate which is related to type of Report: Chargeback for VMs/Images/Projects.

Why:
We need it for adding new drop down to editing screen for Rate to be able to choose the type of the rate and then to display only appropriate columns of the rate, based on the chosen type of the rate (VMs, Images, Projects).

---

Additional note:
This PR was, let's say, 'reverted' by #270

[miq-bot]

Checked commit hstastna@57100e3 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

[lpichler]

@gtanzillo are you ok with this ? if naming is correct ? Thanks!

[lpichler]

👍

[hstastna]

@lpichler @gtanzillo Any update about this PR? Thanks! ;)

[gtanzillo]

👍 @Fryguy you ok with merging this change?

[Fryguy]

I'm not sure about this one...something feels off about the design...can we discuss?

[hstastna]

So let's discuss :) .. We need this PR to be merged to implement this: ManageIQ/manageiq-ui-classic#4001 (screenshots included)

[lpichler]

@Fryguy yes are right, Let me bring some facts:
current diagram:

These days as you know we have 3 types of chargeback reports: for VMs/Images/Projects.
Each report is reporting different columns(in DB ChargeableField) but some of them are common.

This information is now done at application level by this hash. in ChargebackVm, ChargebackContainerImage, ChargebackContainerProject.

It looks that we don't need to extra store this type in ChargebackRate table. We rather want to know which column is related to which types.

ChargebackRate are always related to particular report type. but if we go deeper the chargeback field are making the relations to chargeback report type.

We can think about these three options:

1) Store report type in ChargebackRate
This will simplify work in UI (ManageIQ/manageiq-ui-classic#4001).
It could be also used in backend when chargeback rate are picked up.

2) Store report type in ChargebackReportType
Because basically it is about chargeable fields and their relations to report type.
But one chargeback field can be related to more chargeback type.
So I guess this option will require another table to represent chargeback report type.
This option looks like cleanest DB solution.

3) Leave as it is and use application code
This will bring lot of code and complex methods.

Note (or option 4): Of course, there are more ways to reach this with IFs in UI. But this has primary reason: Don`t mess up the (UI //cc @martinpovolny ) code if there is a chance to make it clean with database changes.

@hstastna if I forgot something or if you see different way let us know.

@Fryguy which option are you prefer ? I am starting to prefer option 2.

thanks

[carbonin]

+1 for option 2.

Based on what I've read, the issue you're trying to solve is that different models have different fields associated with them and we should only display the fields for the selected model when building a report, right? (If this is wrong then just let me know and disregard my opinion here 😆 )

A lookup table for model names could be a good solution for this, but I wonder if there is any other place that we store what metrics apply to what model? Do we have the concept of "metric types" that we can link here instead of using a metric string? That would probably be a more useful solution generally, but definitely not suggesting we build that for this PR if it doesn't exist currently.

[hstastna]

@lpichler @Fryguy @carbonin @gtanzillo Any update about this? Does it mean that we have chosen option 2? Thanks! :)

[carbonin]

@hstastna I think we're waiting on @Fryguy 's input here.

[Fryguy]

I am generally not a fan of storing model names in the database unless they are part of a polymorphic association (and thus Rails takes care of it), and that's part of the reason for my initial concern. That being said, in this particular case, there are only 3 specific models and they are all subclasses of Chargeback, so the scope of what can be stored here is limited and can probably even be mitigated with a model validation to ensure only those types are stored there. Additionally, those 3 models are not actual models, but ActsAsArModel types, so they couldn't be stored via polymorphic relations anyway. Given all that I'm good with option 2.

[carbonin]

@Fryguy Just to be clear, I don't think this PR actually implements Libor's option 2.

[lpichler]

@Fryguy yes, I agree with you, but as @carbonin said this is not option 2, this is option 1, should we create revert of this PR or create migration which is removing the column report_base_model back ?

[hstastna]

Ahh, maybe I had better to move this PR to WIP. Because originally, option 1 was implemented and then we started discussion about another options and I was waiting for others to choose the best option, before making any change in this PR. But now, maybe option 1 is not the worst (it was approved) :D

[Fryguy]

Gah I totally misread the PR. I saw it adding the column to chargeable_rates, but when I read option 2 I saw it say chargeable_fields and was like "yup, that's what the PR is doing...merge".

@lpichler @hstastna You'll have to create a new PR that deletes the one column and adds another...sorry about that.

EDIT: Fixed the author...I knew I shouldn't have been doing any actual work the day after vacation 😆

[hstastna]

Just a note that this PR was 'reverted'.