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

Remove replacement of whole right cell and instead render flash message div so flash message isn't doubled #3180

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Remove replacement of whole right cell and instead render flash message div so flash message isn't doubled #3180

merged 1 commit into from
Mar 1, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 5, 2018 •
edited
Loading

With the right cell being replaced, the control policy description validation appears twice. This removes the replacement of the right cell per h-kataria's suggestion at the review that's at the bottom of this PR.

Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1531469

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 5, 2018

@eclarizio can you look at this please sometime?

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 5, 2018

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Jan 5, 2018
@d-m-u d-m-u changed the title Removes doubled flash error message call from policy creation [WIP] Removes doubled flash error message call from policy creation Jan 5, 2018
@miq-bot miq-bot added the wip label Jan 5, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 5, 2018

@miq-bot assign @dclarizio

@dclarizio dclarizio assigned h-kataria and unassigned dclarizio Jan 5, 2018
@d-m-u d-m-u changed the title [WIP] Removes doubled flash error message call from policy creation Removes doubled flash error message call from policy creation Jan 5, 2018
@miq-bot miq-bot removed the wip label Jan 5, 2018
@h-kataria
Copy link
Contributor

@d-m-u changes here need to be tested/verified closely since this change is removing rendering of flash message partial from expression editor that is being used on many screens, don't want to end up breaking something else.

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 7, 2018

@h-kataria from what I saw, all these errors are added to the flash message here as well

@gmcculloug
Copy link
Member

@h-kataria Do you have any suggestions here on how to best "tested/verified closely"? Need advice on how to move forward with this fix.

@h-kataria
Copy link
Contributor

@d-m-u sorry for the delay in looking into this, in my opinion you can fix this issue by replacing https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/miq_policy_controller/conditions.rb#L89 with javascript_flash in case of flash errors we do not need to replace the whole right cell instead we should only render flash message div to display appropriate error messages. Let me know if you have questions.

@d-m-u d-m-u changed the title Removes doubled flash error message call from policy creation Remove replacement of whole right cell and instead render flash message div so flash message isn't doubled Mar 1, 2018
h-kataria
h-kataria reviewed Mar 1, 2018
View reviewed changes
app/views/layouts/_exp_editor.html.haml Outdated
@@ -7,7 +7,6 @@
= javascript_tag("ManageIQ.expEditor.second.title = '#{@edit[@expkey][:val2][:title]}';")

#exp_editor_div
= render :partial => 'layouts/flash_msg', :locals => {:flash_div_id => 'exp_editor_flash'}
Copy link
Contributor

@h-kataria h-kataria Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@d-m-u we need to keep rendering of flash message partial here, to show flash messages specific to expression editor.
screenshot from 2018-03-01 12-01-10

@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2018

Checked commit d-m-u@f4e6f09 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@h-kataria h-kataria added this to the Sprint 81 Ending Mar 12, 2018 milestone Mar 1, 2018
@h-kataria h-kataria merged commit d64dcc1 into ManageIQ:master Mar 1, 2018
simaishi pushed a commit that referenced this pull request Mar 8, 2018
@h-kataria @simaishi
Merge pull request #3180 from d-m-u/bz_1531469
d744e82 
Remove replacement of whole right cell and instead render flash message div so flash message isn't doubled
(cherry picked from commit d64dcc1)

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

simaishi commented Mar 8, 2018

Gaprindashvili backport details:

$ git log -1
commit d744e82194c60cb3f4977c73bc52782f46157346
Author: Harpreet Kataria <[email protected]>
Date:   Thu Mar 1 13:57:29 2018 -0500

    Merge pull request #3180 from d-m-u/bz_1531469
    
    Remove replacement of whole right cell and instead render flash message div so flash message isn't doubled
    (cherry picked from commit d64dcc1c6a6076437a4b76e0c0059325df638de7)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553340

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

@h-kataria h-kataria h-kataria left review comments

Assignees

@h-kataria h-kataria

Labels
bug control gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@d-m-u @h-kataria @gmcculloug @miq-bot @simaishi @dclarizio