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

BZ 1490103 - Fixed issue for VM custom button actions #1022

Merged
merged 3 commits into from
Sep 28, 2017
Merged

BZ 1490103 - Fixed issue for VM custom button actions #1022

merged 3 commits into from
Sep 28, 2017

Conversation

chalettu
Copy link
Contributor

@chriskacerguis
Copy link
Contributor

@chalettu can you please address that NSP fail?

Copy link
Contributor

@chriskacerguis chriskacerguis left a comment

Choose a reason for hiding this comment

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

NSP failed...need to update lib

@chalettu
Copy link
Contributor Author

I checked the library that is the culprit. It is a mime package. This vulnerability was released today and our dependent packages that depend on that package have not been updated yet. I recommend we ignore it for now.

@chriskacerguis
Copy link
Contributor

Thanks for the info.

@AllenBW I'm good with the merge (with this being so new). Your vote?

@@ -122,4 +122,80 @@ describe('services.custom_button_details', function() {
});
});
});

describe('Custom button actions for a VM', () => {
var collectionsApiSpy;
Copy link
Member

Choose a reason for hiding this comment

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

prefer, const or let to var. consider stacking all the ones without assignments on the same line

@@ -51,6 +73,7 @@ function StateController ($state, $stateParams, dialog, service, CollectionsApi,
vm.dialogs = dialog.content
vm.service = service
vm.serviceId = $stateParams.serviceId
vm.vmId = ($stateParams.vmId ? $stateParams.vmId : null)
Copy link
Member

Choose a reason for hiding this comment

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

$stateParams.vmId || null ternary might be a bit of overkill

@@ -25,6 +25,28 @@ function getStates () {
dialog: resolveDialog,
service: resolveService
}
},
Copy link
Member

Choose a reason for hiding this comment

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

these two states appear very similar, instead of duplicating the code with a mild name change, why not make the vmId an optional param on the existing state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about it. It is really just a judgement call. Initially I wanted to make it this way so we could refresh the page successfully but it looks like we are passing button data and that isn't reproducible so we can't refresh either way. I am kinda fine either way.

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Any chance we can get visual evidence of a fix here? Screenshot, gif, something else surprising or inventive 🤔

@chalettu
Copy link
Contributor Author

vm-actions
@AllenBW , here is a ss of the fix.

@miq-bot
Copy link
Member

miq-bot commented Sep 27, 2017

Checked commits https://github.com/chalettu/manageiq-ui-service/compare/1510ae133abf4a8ce869fff596b1c735ee15cec1~...0ffc636186ab37d43453e269797c6e46407c9272 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

EFFING SWEEEEEET YO 🍭 👍

@chriskacerguis chriskacerguis added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 28, 2017
@chriskacerguis chriskacerguis merged commit 2c2f8f5 into ManageIQ:master Sep 28, 2017
@chalettu chalettu deleted the vm-custom-button-actions branch January 10, 2018 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants