Fix for service catalog service dialog refresh function behaving differently #814
Conversation
https://bugzilla.redhat.com/show_bug.cgi?id=1439761 This change no longer uses the jQuery '$' to trigger and listen to messages but uses an angular service
|
Fixing test failures... |
|
@eclarizio prepending unused params with _ will keep travis from whining about em as being unused |
|
@AllenBW I think there was just a problem with the rebase, because I was still working on the same branch that the old PR was made from, but that got reverted. So some of the changes I needed just didn't make it into the final push and I didn't notice after fixing the conflicts git raised. |
There was a problem hiding this comment.
Only a few minor quibily things, love that code coverage increase!
Upon further reflection I realize most of my asks won't survive a backport to fine or euwe. Gonna make the horrible request prs directly to those branches rather than sacrifice the code base of master moving forward.
client/app/core/core.module.js
Outdated
| @@ -23,6 +23,7 @@ import { BaseModalController } from './modal/base-modal-controller.js'; | |||
| import { BaseModalFactory } from './modal/base-modal.factory.js'; | |||
| import { ChargebackFactory } from './chargeback.service.js'; | |||
| import { CollectionsApiFactory } from './collections-api.factory.js'; | |||
| import { AutoRefreshFactory } from '../services/auto-refresh.service.js'; | |||
There was a problem hiding this comment.
If the factory is to be advertised on the core module it should be located in the core module folder
| if (dialogField.trigger_auto_refresh === true || dialogField.triggerOverride === true) { | ||
| parent.postMessage({refreshableFieldIndex: dialogField.refreshableFieldIndex}, '*'); | ||
| var triggerOptions = {}; |
There was a problem hiding this comment.
Prefer let or const for declaring variables
| }; | ||
|
|
||
| function triggerAutoRefresh(data) { | ||
| angular.forEach(callbacks, (function(callback) { |
There was a problem hiding this comment.
consider using arrow function and .forEach markup
would result in the following rewrite:
callbacks.forEach((callback) => { callback(data) });
|
Checked commits eclarizio/manageiq-ui-service@2993a33~...3d1e769 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@eclarizio looking at what @AllenBW suggested (and said) this will probably need a separate PR for Fine and Euwe. Other than that, LGTM 👍 |
|
@AllenBW This can be backported to Fine branch without conflict. @eclarizio suggested that I check with you to make sure this can go in as is, in case something needs to be done differently in Fine branch. Please let me know. Thanks! |
|
@simaishi well hot darn! yeah ok i see now, all good to go to fine, but not euwe, app structure is fundamentally different :-/ |
Fix for service catalog service dialog refresh function behaving differently (cherry picked from commit 8c8cd0b) https://bugzilla.redhat.com/show_bug.cgi?id=1461183
|
@AllenBW thanks! Added |
|
Fine backport details: |
This is basically a re-do of #793, because that PR somehow had dropped the test coverage and highlighted some problems with the way the specs were written.
As such, this PR moves the triggering and listening to a specific AutoRefresh angular service that doesn't use jQuery's
$(document).on,.off, or.triggermethods.https://bugzilla.redhat.com/show_bug.cgi?id=1439761
@miq-bot add_label euwe/yes, fine/yes, bug
@miq-bot assign @chriskacerguis
@chalettu Can you review and let me know if this is more along the lines of what you were thinking for the auto refresh communications?