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

Need to call alert_profile_edit_load_edit when cancel is pressed #3235

Merged
merged 1 commit into from Jan 12, 2018

Conversation

h-kataria
Copy link
Contributor

alert_profile_edit_load_edit method loads @edit that's being used in alert_profile_load method. Issue was introduced during cleanup in #2026

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

@dclarizio @martinpovolny please review.

@hstastna
Copy link
Contributor

hstastna commented Jan 12, 2018

@h-kataria I was looking at this BZ just right before the BZ was assigned to you (somebody was faster than me :D ) and I found almost the same fix as you. I have just one question: what if https://github.com/ManageIQ/manageiq-ui-classic/pull/3235/files#diff-dec290862651b2c668afbc0cfd64e57bR9 returns in some special case false? There is nothing which would deal with it, in your fix. Theoretically, it could happen. Wouldn't be better to add the line return unless alert_profile_edit_load_edit to alert_profile_edit method under 'cancel' case (and also to remove alert_profile_load from alert_profile_edit_cancel, as you already did)? The same way as the other cases, to deal with every button the same way :)

@h-kataria
Copy link
Contributor Author

@hstastna https://github.com/h-kataria/manageiq-ui-classic/blob/c6f119b7f9c3333c31c6f20eb78bd479e1cc8f20/app/controllers/miq_policy_controller/alert_profiles.rb#L76 takes care of that, in case load of @edit fails it should redirect back to replace_right_cell and show "Edit aboterd!...." flash message on screen, same way as we do in other explorer controllers.

@hstastna
Copy link
Contributor

hstastna commented Jan 12, 2018

@h-kataria You are right, it should redirect but...

When I made load_edit method return false somehow in alert_profile_edit_load_edit, no redirection, no expected error message occurred, but another error because it jumped to https://github.com/ManageIQ/manageiq-ui-classic/pull/3235/files#diff-dec290862651b2c668afbc0cfd64e57bL14 and @alert_profile was nil (because it did not went to alert_profile_load method) and problem with multiple redirection. I hope I did something wrong :D (or I don't know why I get this behavior) Could you somehow also check this edge situation, to be sure? It is always better to be sure :)

alert_profile_edit_load_edit method loads `@edit` that's being used in alert_profile_load method. Issue was introduced during cleanup in ManageIQ#2026

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533633
@h-kataria
Copy link
Contributor Author

@hstastna good catch, addressed the issue

@miq-bot
Copy link
Member

miq-bot commented Jan 12, 2018

Checked commit h-kataria@0d97247 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

spec/controllers/miq_policy_controller/alert_profiles_spec.rb

  • ❗ - Line 79, Col 92 - Layout/MultilineHashBraceLayout - Closing hash brace must be on the line after the last hash element when opening brace is on a separate line from the first hash element.

@dclarizio
Copy link

Tested successfully in the UI.

@dclarizio dclarizio merged commit 6361567 into ManageIQ:master Jan 12, 2018
simaishi pushed a commit that referenced this pull request Jan 12, 2018
Need to call alert_profile_edit_load_edit when cancel is pressed
(cherry picked from commit 6361567)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534060
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 766616b1399ad4b9ab3917d9c613d118b46d6933
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Fri Jan 12 15:02:39 2018 -0800

    Merge pull request #3235 from h-kataria/alert_profile_editor_fix
    
    Need to call alert_profile_edit_load_edit when cancel is pressed
    (cherry picked from commit 63615671fe880d677caacb17ef54d17f9d68074f)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534060

@dclarizio dclarizio added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 12, 2018
@h-kataria h-kataria deleted the alert_profile_editor_fix branch March 14, 2018 20:18
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

5 participants