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

Remove catching flash messages in report_data_controller.js #4789

Merged
merged 1 commit into from
Oct 22, 2018

Conversation

rvsia
Copy link
Contributor

@rvsia rvsia commented Oct 17, 2018

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

Steps to Reproduce:

  1. Add infra or cloud provider
  2. Navigate to Compute -> Infrastructure -> Virtual Machines
    or Compute -> Clouds -> Instances
  3. Click on Lifecycle -> Provision VMs
    or Lifecycle -> Provision Instances
  4. Click Cancel

Actual results:

Two success messages appear, one without text

Expected results:

One success message appears

Before / After

screenshot from 2018-10-17 19-46-01

screenshot from 2018-10-17 19-44-30

NOTES

  • This is a just a a suggestion how to solve it. I would appreciate any help!

The oneMessage.msg ( here , introduced by #1245 ) is undefined, so it's causing empty messages. It should be oneMessage.message, but then it's causing double messages.

I don't know if msg is used somewhere else, but I checked issues from #1273 (which should be fixed by this piece of code) after I had removed the function and they are fixed anyway. So I think the function actually doesn't work right now. Maybe there was another change which fixed all these thing. I don't know.

@karelhala @himdel @skateman Pinging you, because you were involved in that PR.

@rvsia
Copy link
Contributor Author

rvsia commented Oct 17, 2018

@miq-bot add_label gaprindashvili/no, hammer/yes, bug, gtls

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

* oneMessage.msg is actually undefined (it should be oneMessage.message), it probably never works
@miq-bot
Copy link
Member

miq-bot commented Oct 18, 2018

Checked commit rvsia@85cc039 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@rvsia rvsia changed the title [WIP] Remove catching flash messages in report_data_controller.js Remove catching flash messages in report_data_controller.js Oct 18, 2018
@miq-bot miq-bot removed the wip label Oct 18, 2018
@hstastna
Copy link

I tried this PR and it fixes also my BZ https://bugzilla.redhat.com/show_bug.cgi?id=1611612. 👍 from my side

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Tested in UI, fixes the issue 👍

@himdel himdel self-assigned this Oct 22, 2018
@himdel himdel added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 22, 2018
@himdel
Copy link
Contributor

himdel commented Oct 22, 2018

Not seeing any indications of .msg being set anywhere 👍

@himdel himdel merged commit 888aa60 into ManageIQ:master Oct 22, 2018
simaishi pushed a commit that referenced this pull request Oct 22, 2018
Remove catching flash messages in report_data_controller.js

(cherry picked from commit 888aa60)

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

Hammer backport details:

$ git log -1
commit 6dccebbd000b045a0b833248caddcc93d6e34b85
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Oct 22 15:35:48 2018 +0200

    Merge pull request #4789 from rvsia/bz1639213
    
    Remove catching flash messages in report_data_controller.js
    
    (cherry picked from commit 888aa60fe36bad22fb8c4cd8331a91395857cc50)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1639213

@rvsia rvsia deleted the bz1639213 branch September 17, 2019 12:01
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.

6 participants