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 support for displaying network adapter details #1654

Merged
merged 4 commits into from Aug 30, 2017

Conversation

skovic
Copy link

@skovic skovic commented Jul 5, 2017

Updated the physical server summary page to display details for network adapters.

image

@skovic
Copy link
Author

skovic commented Jul 5, 2017

@miq-bot add_label wip

@miq-bot miq-bot changed the title Add support for displaying network adapter details [WIP] Add support for displaying network adapter details Jul 5, 2017
@miq-bot miq-bot added the wip label Jul 5, 2017
@skovic
Copy link
Author

skovic commented Jul 11, 2017

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Add support for displaying network adapter details Add support for displaying network adapter details Jul 11, 2017
@miq-bot miq-bot removed the wip label Jul 11, 2017
@mzazrivec
Copy link
Contributor

I'm getting the following error when trying to render physical server summary:

Error caught: [ActionView::Template::Error] undefined method `child_devices' for #<GuestDevice:0x007faf13990b78>
~/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activemodel-5.0.4/lib/active_model/attribute_methods.rb:433:in `method_missing'
manageiq-ui-classic/app/helpers/physical_server_helper/textual_summary.rb:170:in `block in textual_network_adapter'

@mzazrivec
Copy link
Contributor

@skovic Or is there perhaps a backend / provider PR that this change depends on?

@skovic
Copy link
Author

skovic commented Jul 12, 2017

@mzazrivec Yes, there are backend and provider changes that this PR depends on. The changes are in manageiq-providers-lenovo PR 59 and manageiq PR 15371.

@mzazrivec
Copy link
Contributor

@skovic I'm adding pending core label to this PR. Once the backend PRs are resolved / merged,
please remove the label for the review to commence. Thanks.

@miq-bot
Copy link
Member

miq-bot commented Jul 13, 2017

Checked commits skovic/manageiq-ui-classic@d8a6242~...6a71859 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 2 offenses detected

app/views/physical_server/_textual_network_adapter_table.html.haml

  • ⚠️ - Line 17 - Block has too many lines. [30/25]
  • ⚠️ - Line 18 - Block has too many lines. [28/25]

@martinpovolny
Copy link

@skovic : this looks all good and mergeable 👍 However there's one thing that would help us in the future.

The textual summaries are to be converted to Angular in the future and at that point they are going to be rendered on the client side and the data will be passed in as JSON.

To make that simpler I tried to limit the number of different Textual* classes. Yes, I created the TextualCustom to support custom formatting but we will have to deal with that when we do the Angular conversion.

Could you, please, take a look if any of the app/views/shared/summary/_textual_* could be used to achieve the formatting that you need?

If you figure that none of the existing templates provides the formatting that you need, then just ping me here and we merge this as it is. But, please, take a look.

@AparnaKarve
Copy link
Contributor

@skovic Can you checkout TextualMultilabel (template: 'shared/summary/textual_multilabel')?
That could work for this case.

@skovic
Copy link
Author

skovic commented Aug 3, 2017

@AparnaKarve I'm not sure that TextualMultilabel will be able to give me the same layout as shown in the screenshot (in regard to the ports heading and related subheadings). Also FYI, I think that the TextualMultilabel template may need to be updated. I tried using it but it keeps saying undefined variable or method 'values'. I notice that the other templates are using 'items' instead. Also, there is a typo ".pull-rigt" instead of ".pull-right" in the file. Thanks

@AparnaKarve
Copy link
Contributor

AparnaKarve commented Aug 3, 2017

@skovic I recently used TextualMultilabel in https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/generic_object_definition_helper/textual_summary.rb, and did not have any issues for my use case. The typo that you indicated, needs to be fixed, however.

From the screenshot above, perhaps the Firmware table could have used TextualMultilabel.
But as far as the Network adapters table goes, I agree that it looks a tad different where the Ports subtable is rendered.
I guess TextualCustom seems OK for your use case because of the way the Ports table is embedded in the Network adapters table.

@martinpovolny IMO, GTG.

@chargio
Copy link

chargio commented Aug 23, 2017

@martinpovolny Hi, it seems that these changes has been approved long ago.
It is possible to merge them?

@skovic, I understand that we will have to wait until ManageIQ/manageiq#15371 is merged (the other one has already been merged).
Could you update this when that PR is merged?

@martinpovolny
Copy link

martinpovolny commented Aug 30, 2017

The problem is: Any new formatting added here will need to be supported when we convert this to a an angular component and when we feed in the data through some API.

I have spent significant time unifying the layouts and methods of the textual summaries so that we can do the next steps -- components and API. I don't want that effort to be wasted.

Therefor I need to be really sure that there's no other way than adding a new layout.

@martinpovolny martinpovolny merged commit 52bd888 into ManageIQ:master Aug 30, 2017
@martinpovolny martinpovolny added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 30, 2017
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

6 participants