Setting PhysicalSwitch as its base model

[EsdrasVP]

This PR is able to:

• Set PhysicalSwitch as its base model to avoid problems when Physical Infra Provider calls it

The problems:

When trying to list the Physical Switches from a Physical Infra Provider (/ems_physical_infra/:id?display=physical_switches)

An error is returned

This happens because the application controller from the UI calls paged_view_search, which is a method from Search class inside ManageIQ core that, internally, uses the base model for the class passed. The result is that, instead of "PhysicalSwitch", its parent "Switch" is called, and the Physical Infra Provider deals with PhysicalSwitch, not Switch.

And while accessing a Physical Switch in its summary page (/physical_switch/:id), the QuadIcon doesn't show all quadrants, instead, it is a single icon

This also happens because, in quadicon helper, a call is made for the class's base model. Currently this is solved using "demodulize" in the class, but this could affect other classes, and could induce an error to happen.

[agrare]

Not sure exactly what the issue is but I don't think this is the right approach

[EsdrasVP]

@agrare Just changed the approach to a more used in ManageIQ. This solves the problem and reduces the error probability.

[agrare]

@EsdrasVP can you post the exact issue that you're seeing with screenshots or error backtraces? I'm still not sure what you're trying to fix.

[EsdrasVP]

@agrare I put some screenshots to explain better the problem.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[miq-bot]

Checked commit EsdrasVP@b9181ec 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. 👍

[agrare]

@himdel can you help out with how this is usually handled at the UI level? I don't think hacking the base_model is correct but I'm not sure what is typically done in the UI to handle associations that aren't on the base class (which we do a lot of btw).

[saulotoledo]

Hi @agrare and @himdel, @skateman just opened a PR at ManageIQ/manageiq-ui-classic#3980 to fix an issue caused by the demodulize changes. I think that can help in the current discussion. I've added some comments for it.

[skateman]

@agrare I have a feeling that this is against the logic of the base_model method...

[agrare]

I'm going to send this one over to @Fryguy

[EsdrasVP]

@Fryguy @agrare @skateman @saulotoledo I think we're trying to acomplish things that don't fit together. The way I see, we have this options here:

• We keep the relationship between Switch and PhysicalSwitch and redefine PhysicalSwitch base model (The current solution)
• We keep the relationship between Switch and PhyiscalSwitch and refactor the pieces of code where base model is called to avoid this kind of problem (this is a major change)
• We remove the relationship between Switch and PhysicalSwitch and keep them sharing the same table (this is even worst than the current solution)
• We remove the relationship between Switch and PhysicalSwitch and create an physical interface for all physical infra, making PhysicalSwitch inherit from it (this is another major change)

What do you think of these? Is there something that I'm letting unnoticed?

[skateman]

Hm, we're trying to solve a purely UI issue in the model...this is wrong. Let's take this discussion back to the UI, we have decorators there for this purrpose (pun intended). Anyway we would need to rename the quadicon settings keys so the case statement can be deleted. I will come up with something, just need some time for that. Until then here's quick ui-only fix: ManageIQ/manageiq-ui-classic#3980

[EsdrasVP]

@skateman This would solve quadicon, however the physical switch listing would continue to fail, because even though that's a problem happening in the UI, the method used by it is from ManageIQ core (more precisely in manageiq/app/models/miq_report/search.rb).

[skateman]

@EsdrasVP ok, what's the problem there and how it can be solved without changing the base_model?

[EsdrasVP]

@skateman The method itself is not the problem. The problem is that the UI application controller calls get_view > paged_view_search > get_parent_targets. Since we are trying to obtain Physical Switches from a Physical Infra Provider, this method causes the listing to fail because it replaces PhysicalSwitch by its base model Switch. So an error occur because Physical Infra Provider doesn't have switches, it has physical switches.

[agrare]

@EsdrasVP is the problem really just the association name? Could we change or alias https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/physical_infra_manager.rb#L8 ?

[EsdrasVP]

@agrare It's not only the association name. We could make it work if we change Physical Infra Manager to contain Switches and set its class_name as "PhysicalSwitch". But in order for that to work we have to replace :physical_switches by :switches in any call, and this affects model, parser and the save on inventory. And the bigger problem would be save the inventory, because after parsing all switch data a call will be made for a method in save_inventory_physical_infra.rb that binds its name to the passed model (in this case "switches"). The thing is that there is already a method to save inventory for switches in save_inventory_infra.rb. Doing this, we force the Ruby to break because it doesn't distinguish two methods with the same name by arguments number. So VMware, for instance, would break in this scenario. I think this solution would cause an even bigger mess.

[EsdrasVP]

I'm closing this PR since this was solved in ManageIQ/manageiq-ui-classic#3954.