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.

Sign up for GitHub

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

Jump to bottom

Display assets detail #1224

Merged
merged 3 commits into from May 16, 2017
Merged

Display assets detail #1224

merged 3 commits into from May 16, 2017

Conversation

MaysaMacedo
Copy link
Member

@MaysaMacedo MaysaMacedo commented May 2, 2017 •
edited

This PR is able to:

  • Show Assets detail informations.

image

Depends on:

  1. [WIP] Physical Server Pages #1169
  2. Physical Server textual summary #1168
  3. #43

martinpovolny
martinpovolny reviewed May 3, 2017
View reviewed changes
app/views/physical_server/_main.html.haml Outdated
= render :partial => "layouts/flash_msg"
.row
.col-md-12.col-lg-6
= render :partial => "shared/summary/textual", :locals => {:title => _("Properties"), :items => textual_group_properties}
Copy link

Choose a reason for hiding this comment

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

This is not how it is done any more. Please, see #274 that includes many examples on how to define the groups.

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, Just fixed it.

@dclarizio dclarizio requested a review from AparnaKarve May 3, 2017 21:58
@dclarizio dclarizio self-assigned this May 3, 2017
@miq-bot
Copy link
Member

miq-bot commented May 3, 2017

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

@miq-bot
Copy link
Member

miq-bot commented May 5, 2017

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

martinpovolny
martinpovolny reviewed May 5, 2017
View reviewed changes
app/views/physical_server/show.html.haml Outdated
@@ -2,5 +2,7 @@
- arr = %w(physical_server physical_servers)
- if arr.include?(@display) && @showtype != "compare"
= render(:partial => "layouts/gtl", :locals => {:action_url => "show/#{@ph_server.id}"})
- elsif @showtype == 'main'
= render :partial => "layouts/textual_groups_generic"
- else
= render(:partial => @showtype)
Copy link

Choose a reason for hiding this comment

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

please, don't do this:
render(:partial => @showtype)

I guess you don't need the else branch at all. And if you do, then list all the @showtype values explicitly. It's hard to figure the possible values for @showtype later on and it is a dangerous practice. If user data would reach @showtype then this can lead to a security issue.

I bet you can just remove it for you case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just removed it.

AparnaKarve
AparnaKarve reviewed May 8, 2017
View reviewed changes
app/controllers/physical_server_controller.rb Outdated
@@ -33,7 +33,7 @@ def show_list
end

def textual_group_list
[]
[%i(properties relationships assets)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot test assets in the UI since it has too many dependencies that are not merged in yet.
This depends on ManageIQ/manageiq-providers-lenovo#43 which depends on ManageIQ/manageiq#14827.

I recommend taking assets out for now until the above PRs are merged... and make this PR about only properties and relationships

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, looks like textual_group_properties and textual_group_relationships are already merged here 90e3a5a
Ideally, that commit should have had [%i(properties relationships)]

Could you make a separate PR that would just have [%i(properties relationships)] in textual_group_list as a follow-up to #1295.

/cc @gabrielsvinha

Copy link
Contributor

Choose a reason for hiding this comment

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

#1272 can address the above. No need for a separate PR to add [%i(properties relationships)].

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we also have #1294 that can address the above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the branch that originated this Pull Request is behind master (that already have these changes)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should come as a follow up to #1294

@miq-bot
Copy link
Member

miq-bot commented May 9, 2017

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

@MaysaMacedo MaysaMacedo force-pushed the display_assets_detail branch 2 times, most recently from 02d035c to afe78f4 Compare May 9, 2017 20:55
@miq-bot
Copy link
Member

miq-bot commented May 9, 2017

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

@miq-bot
Copy link
Member

miq-bot commented May 9, 2017

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

@miq-bot
Copy link
Member

miq-bot commented May 10, 2017

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

@miq-bot
Copy link
Member

miq-bot commented May 16, 2017

Checked commits MaysaMacedo/manageiq-ui-classic@36f2517~...ca99634 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@martinpovolny martinpovolny added this to the Sprint 61 Ending May 22, 2017 milestone May 16, 2017
@martinpovolny martinpovolny merged commit b49a4a3 into ManageIQ:master May 16, 2017
@AparnaKarve
Copy link
Contributor

@martinpovolny Didn't this PR have a spec failure?

@AparnaKarve AparnaKarve mentioned this pull request May 16, 2017
Merged
@martinpovolny
Copy link

@AparnaKarve : Was I trigger happy? Sorry for that!

@AparnaKarve
Copy link
Contributor

@martinpovolny No worries.
I will try and fix the spec today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpovolny martinpovolny martinpovolny left review comments

@AparnaKarve AparnaKarve AparnaKarve left review comments

@gabrielsvinha gabrielsvinha gabrielsvinha left review comments

Assignees

@dclarizio dclarizio

Labels
compute/physical infrastructure enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MaysaMacedo @miq-bot @AparnaKarve @martinpovolny @gabrielsvinha @dclarizio