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

let caller method render flash message for sublist screens. #4368

Merged
merged 1 commit into from Sep 28, 2015

Conversation

Projects
None yet
6 participants
@h-kataria
Contributor

h-kataria commented Sep 15, 2015

  • Fixed respective 'button' methods to render flash message partial when task(clone,migrate,publish) are not supported for selected items when user trees to perform these tasks from list of VMs thru relationships. render_flash_not_applicable_to_model will only render flash message when performing tasks directly from VM explorers.
  • Added spec test to verify fix.

https://bugzilla.redhat.com/show_bug.cgi?id=1263326

@martinpovolny @dclarizio please review and suggest if there is another way this issue can be fixed. To recreate the issue Go to Infrastructure Providers, Go to SCVMM Provider summary screen, from there Click on VMs in the relationships box, Select one or more VMs from the list and click on Migrate selected Items button under Lifecycle button. You should see double render error in the log, you should be able to see this behavior for any of the CIs that have link to view VMs in the relationship box on their respective summary screens. Selected VM has to be the one that do not support Migrate task, which is why i used VMs under a SCVMM provider. Let me know if you have questions or need help recreating this issue.

@h-kataria

This comment has been minimized.

Show comment
Hide comment
@h-kataria

h-kataria Sep 17, 2015

Contributor

@martinpovolny please review.

Contributor

h-kataria commented Sep 17, 2015

@martinpovolny please review.

@martinpovolny

View changes

Show outdated Hide outdated app/controllers/cim_instance_controller.rb
@martinpovolny

View changes

Show outdated Hide outdated spec/controllers/ems_common_controller_spec.rb
@martinpovolny

This comment has been minimized.

Show comment
Hide comment
@martinpovolny

martinpovolny Sep 18, 2015

Contributor

@h-kataria : tested, looks good 👍 Please, see the 2 minor comments in-line.

Contributor

martinpovolny commented Sep 18, 2015

@h-kataria : tested, looks good 👍 Please, see the 2 minor comments in-line.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Sep 18, 2015

Contributor

I don't like this; if "just silently ignore a second attempt to render" was the reasonable thing to do, we'd be doing it upstream.

If a controller is so confused that it's trying to render twice, that is a bug in the action's control flow, which should be fixed.

Where is the first render occurring here, and why are we not returning at that point?

Contributor

matthewd commented Sep 18, 2015

I don't like this; if "just silently ignore a second attempt to render" was the reasonable thing to do, we'd be doing it upstream.

If a controller is so confused that it's trying to render twice, that is a bug in the action's control flow, which should be fixed.

Where is the first render occurring here, and why are we not returning at that point?

@h-kataria

This comment has been minimized.

Show comment
Hide comment
@h-kataria

h-kataria Sep 18, 2015

Contributor

@matthewd i didn't feel comfortable adding this fix to begin with, i have another approach that i am going to try to implement, hopefully that will be a better one.

Contributor

h-kataria commented Sep 18, 2015

@matthewd i didn't feel comfortable adding this fix to begin with, i have another approach that i am going to try to implement, hopefully that will be a better one.

@h-kataria h-kataria changed the title from Fix to not try to render, to prevent double render error. to let caller method render flash message for sublist screens. Sep 18, 2015

@h-kataria

This comment has been minimized.

Show comment
Hide comment
@h-kataria

h-kataria Sep 18, 2015

Contributor

@matthewd @martinpovolny @dclarizio tried a different approach please review.

Contributor

h-kataria commented Sep 18, 2015

@matthewd @martinpovolny @dclarizio tried a different approach please review.

@martinpovolny

This comment has been minimized.

Show comment
Hide comment
@martinpovolny

martinpovolny Sep 19, 2015

Contributor

@matthewd : You are right. The approach is wrong. But most of the controller code has this issue. So to address the cause we would have to do a major refactoring. Of course, we want to do that and we try to do that as part of our work.

But here we need to make a small enough fix to just fix the bug. Refactoring should be done in a separate PR.

Contributor

martinpovolny commented Sep 19, 2015

@matthewd : You are right. The approach is wrong. But most of the controller code has this issue. So to address the cause we would have to do a major refactoring. Of course, we want to do that and we try to do that as part of our work.

But here we need to make a small enough fix to just fix the bug. Refactoring should be done in a separate PR.

@chessbyte

This comment has been minimized.

Show comment
Hide comment
@chessbyte

chessbyte Sep 22, 2015

Member

@matthewd @martinpovolny @dclarizio Please review with rework from @h-kataria

Member

chessbyte commented Sep 22, 2015

@matthewd @martinpovolny @dclarizio Please review with rework from @h-kataria

@martinpovolny

This comment has been minimized.

Show comment
Hide comment
@martinpovolny

martinpovolny Sep 23, 2015

Contributor

@h-kataria : style issues:

  1. you have if/unless modifiers with the same condition for 2 consequent statements ---> normal if/else/end would look better for sure
  2. several of the modified code blocks look exactly the same, can we extract a method, please?
Contributor

martinpovolny commented Sep 23, 2015

@h-kataria : style issues:

  1. you have if/unless modifiers with the same condition for 2 consequent statements ---> normal if/else/end would look better for sure
  2. several of the modified code blocks look exactly the same, can we extract a method, please?
let caller method render flash message for sublist screens.
- Fixed respective 'button' methods to render flash message partial when task(clone,migrate,publish) are not supported for selected items when user trees to perform these tasks from list of VMs thru relationships. render_flash_not_applicable_to_model will only render flash message when performing tasks directly from VM explorers.
- Added spec test to verify fix.
- Moved common code in controllers into a single method.

https://bugzilla.redhat.com/show_bug.cgi?id=1263326
@h-kataria

This comment has been minimized.

Show comment
Hide comment
@h-kataria

h-kataria Sep 23, 2015

Contributor

@martinpovolny please re-review, addressed your comments, the only controller that is not using the new common method is host controller as it passes in additional parameters on a redirect.

Contributor

h-kataria commented Sep 23, 2015

@martinpovolny please re-review, addressed your comments, the only controller that is not using the new common method is host controller as it passes in additional parameters on a redirect.

@miq-bot

This comment has been minimized.

Show comment
Hide comment
@miq-bot

miq-bot Sep 23, 2015

Member

Checked commit h-kataria@a757660 with ruby 1.9.3, rubocop 0.34.2, and haml-lint 0.13.0
14 files checked, 5 offenses detected

app/controllers/application_controller.rb

Member

miq-bot commented Sep 23, 2015

Checked commit h-kataria@a757660 with ruby 1.9.3, rubocop 0.34.2, and haml-lint 0.13.0
14 files checked, 5 offenses detected

app/controllers/application_controller.rb

dclarizio added a commit that referenced this pull request Sep 28, 2015

Merge pull request #4368 from h-kataria/fixed_prov_related_buttons_fo…
…r_sub_list_views

let caller method render flash message for sublist screens.

@dclarizio dclarizio merged commit 5cc63e5 into ManageIQ:master Sep 28, 2015

1 of 3 checks passed

Hakiri Error scanning for security vulnerabilities.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage increased (+0.06%) to 46.836%
Details

@dclarizio dclarizio deleted the h-kataria:fixed_prov_related_buttons_for_sub_list_views branch Sep 28, 2015

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