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

Adding PhysicalStorages list and summary pages #4167

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Adding PhysicalStorages list and summary pages #4167

merged 1 commit into from
Jul 24, 2018

Conversation

EsdrasVP
Copy link
Member

@EsdrasVP EsdrasVP commented Jun 19, 2018 •
edited
Loading

This PR is able to:

  • Add PhysicalStorages to dashboard view
  • Add PhysicalStorages list page
  • Add PhysicalStorages summary page

Depends on:

Storage Menu:
image

Storage On Physical Infra Provider Dashboard:
image

Storage On Physical Infra Provider Summary Page:
image

Storage List Page:
image

Storage Summary Page:
image

Storage Inside Rack Reference
image

@EsdrasVP EsdrasVP requested a review from martinpovolny as a code owner June 19, 2018 18:31
@miq-bot miq-bot added the wip label Jun 19, 2018
@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch 3 times, most recently from 3c33789 to 2897396 Compare June 21, 2018 19:29
skateman
skateman reviewed Jun 22, 2018
View reviewed changes
app/decorators/physical_storage_decorator.rb Outdated

private

def health_state_img
Copy link
Member

Choose a reason for hiding this comment

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

Unused method and also it's no longer needed, as it has been replaced by a method in QuadiconHelper.

Copy link
Member Author

Choose a reason for hiding this comment

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

@skateman Just fixed it in my new push, can you take a look to see if it's correct now?

@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch from 2897396 to a8b2553 Compare June 22, 2018 20:22
@EsdrasVP EsdrasVP changed the title [WIP] Adding PhysicalStorages list and summary pages Adding PhysicalStorages list and summary pages Jun 22, 2018
@miq-bot miq-bot removed the wip label Jun 22, 2018
@EsdrasVP EsdrasVP changed the title Adding PhysicalStorages list and summary pages [WIP] Adding PhysicalStorages list and summary pages Jun 22, 2018
@miq-bot miq-bot added the wip label Jun 22, 2018
@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch from a8b2553 to 660673f Compare June 25, 2018 17:37
@EsdrasVP EsdrasVP changed the title [WIP] Adding PhysicalStorages list and summary pages Adding PhysicalStorages list and summary pages Jun 25, 2018
@miq-bot miq-bot removed the wip label Jun 25, 2018
martinpovolny
martinpovolny reviewed Jul 2, 2018
View reviewed changes
app/views/physical_storage/show_list.html.haml
%physical-switch-toolbar#physical_storage_show_list_form

:javascript
miq_bootstrap('#physical_storage_show_list_form');
Copy link
Member

Choose a reason for hiding this comment

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

pls, add the missing EOL (and update you editor setting to do that for you automatically)

also above

Copy link
Member Author

Choose a reason for hiding this comment

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

@martinpovolny Thanks for the reminder, just added here.

@martinpovolny
Copy link
Member

Code looks good. I'd like to test this in the UI before merging (and I did not do that yet).

@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch from 660673f to d0eb9e8 Compare July 2, 2018 17:24
martinpovolny
martinpovolny reviewed Jul 3, 2018
View reviewed changes
config/routes.rb Outdated
button
listnav_search_selected
show_list
create
Copy link
Member

@martinpovolny martinpovolny Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What about all these create and update routes? do you need these?

Also, if the controller + action name match the feature name, then this would work: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/application_controller.rb#L1172

The idea is that people forget to add the assert_rbac(...) to their action so there's a second checking mechanism in place for all the actions.

Of course you might need actions that don't check RBAC (but those should not be POST for sure) and sure you might want to pass throught the for all mechanism and just use your assert_rbac call in your action.

physical_storage_ + create looks good. It might be a feature name that you might have. But physical_storage_ + create_del looks weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about all these create and update routes? do you need these?

@martinpovolny Currently no, I thought that some of these could be used in the future, but I'm going to remove them and leave only the necessary for now. Also, and correct me if I'm wrong, I just noticed that some routes in compare_get might not be supported in this case, so to be safe I'll remove it too. If they'll be needed in the future, we can simply add it here.

Copy link
Member

Choose a reason for hiding this comment

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

Unless you are adding the compare capability for the new entity you don't need the compare_get set of routes.
Please, do not add routes that you do not need ;-)

@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch 2 times, most recently from 12150b7 to b4cdee2 Compare July 4, 2018 19:21
@miq-bot
Copy link
Member

miq-bot commented Jul 7, 2018

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

@rodneyhbrown7
Copy link

@miq-bot add_label ux/review

@EsdrasVP EsdrasVP closed this Jul 18, 2018
@EsdrasVP EsdrasVP reopened this Jul 18, 2018
@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch from b4cdee2 to f7eb122 Compare July 18, 2018 16:15
@EsdrasVP EsdrasVP closed this Jul 19, 2018
@EsdrasVP EsdrasVP reopened this Jul 19, 2018
@EsdrasVP
Copy link
Member Author

@martinpovolny Could you take a look at it? All dependencies were merged and I rebased the code.

@martinpovolny
Copy link
Member

@EsdrasVP : I have a very last request before I merge this PR: Please add a spec for the textual summary. Here's a simple example: #4258

@martinpovolny
Copy link
Member

Here's a better example: https://github.com/ManageIQ/manageiq-ui-classic/pull/4065/files#diff-dc95b07f3cc46bacf801f38cdbf17cd7R1

@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch from f7eb122 to dbcd85c Compare July 23, 2018 14:44
@EsdrasVP
Copy link
Member Author

@martinpovolny I checked the example that you sent and I adapted it to the PhysicalStorage textual summary. Can you take a look at it to see if it's ok?

@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch 2 times, most recently from e4094f6 to be30d28 Compare July 23, 2018 17:28
@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2018

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

@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch 2 times, most recently from d94e138 to 5e4a8b7 Compare July 24, 2018 14:39
@martinpovolny
Copy link
Member

Looks good to me. Would be nice if you could fix the style issues in the spec file. Also waiting for travis.

Otherwise I think we can merge this. Thx!

@EsdrasVP EsdrasVP force-pushed the physical_storages_page branch from 5e4a8b7 to 60d7ad7 Compare July 24, 2018 15:25
@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2018

Some comments on commit EsdrasVP@60d7ad7

spec/controllers/physical_storage_controller_spec.rb

  • ⚠️ - 28 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2018

Checked commit EsdrasVP@60d7ad7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
25 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@EsdrasVP
Copy link
Member Author

Looks good to me. Would be nice if you could fix the style issues in the spec file. Also waiting for travis.

Otherwise I think we can merge this. Thx!

@martinpovolny I fixed the style issues, and also travis tests passed. Now I think it is good to merge. Thank you!

@martinpovolny martinpovolny merged commit 8df22c0 into ManageIQ:master Jul 24, 2018
@martinpovolny martinpovolny added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 24, 2018
@martinpovolny martinpovolny self-assigned this Jul 24, 2018
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

@skateman skateman skateman left review comments

Assignees

@martinpovolny martinpovolny

Labels
compute/physical infrastructure enhancement pending core ux/review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EsdrasVP @martinpovolny @miq-bot @rodneyhbrown7 @skateman