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

Fix all users of CredentialsController to provide vm-scope #726

Merged
merged 1 commit into from Mar 20, 2017
Merged

Fix all users of CredentialsController to provide vm-scope #726

merged 1 commit into from Mar 20, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Mar 17, 2017

Go to Compute > Clouds > Providers, create a new provider.
Pick the GCE type.

Before

angular.self-a4ec94b….js?body=1:10708 TypeError: Cannot read property 'formId' of undefined
    at new <anonymous> (credentials_controller.self-ec7bece….js?bod…:25)
    at $controllerInit (angular.self-a4ec94b….js?body=1:10593)
    at nodeLinkFn (angular.self-a4ec94b….js?body=1:9470)
    at compositeLinkFn (angular.self-a4ec94b….js?body=1:8811)
    at compositeLinkFn (angular.self-a4ec94b….js?body=1:8814)
    at publicLinkFn (angular.self-a4ec94b….js?body=1:8691)
    at ngIfWatchAction (angular.self-a4ec94b….js?body=1:27108)
    at Scope.$digest (angular.self-a4ec94b….js?body=1:17837)
    at ChildScope.$apply (angular.self-a4ec94b….js?body=1:18103)
    at HTMLSelectElement.<anonymous> (angular.self-a4ec94b….js?body=1:31995)

(And the Validate button is broken)

Now .. it works.


Introduced in #425, CredentialsController needs a vm-scope attribute to be able to determine the correct scope.

This change was not done in app/views/layouts/angular/_auth_service_account_angular.html.haml which caused adding a GCE provider to be broken.

Fixing.

Cc @AparnaKarve , @ZitaNemeckova

@ZitaNemeckova
Copy link
Contributor

Before:
screen shot 2017-03-17 at 2 42 33 pm

After:
screen shot 2017-03-17 at 2 44 28 pm

Looks good :)

@@ -4,7 +4,7 @@
- vm_scope ||= false
- main_scope = vm_scope ? "$parent.vm" : "$parent"

%div{"ng-controller" => "CredentialsController"}
%div{"ng-controller" => "CredentialsController", "vm-scope" => main_scope}
Copy link
Contributor

Choose a reason for hiding this comment

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

Easy enough fix...I like that :)

@@ -4,7 +4,7 @@ ManageIQ.angular.app.controller('CredentialsController', ['$scope', '$attrs', fu
vm.vmScope = function() {
return $scope.$eval($attrs.vmScope);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you get rid of this file altogether from this PR? since it's not really adding anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, there was a trailing whitespace.. I guess we don't need to remove it.. will do :)

@AparnaKarve
Copy link
Contributor

@ZitaNemeckova Why is this commit 754d023 linked to this PR?

Do we still need those changes?

@himdel
Copy link
Contributor Author

himdel commented Mar 17, 2017

@AparnaKarve because this fixes the issue @cben was seeing :).

Introduced in #425, `CredentialsController` needs a `vm-scope` attribute to be able to determine the correct scope.

This change was not done in app/views/layouts/angular/_auth_service_account_angular.html.haml which caused adding a GCE provider to be broken.

Fixing.
@miq-bot
Copy link
Member

miq-bot commented Mar 17, 2017

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/ea7987da7e7b9bfcbff4d339920069039f4b0e67 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

@mzazrivec mzazrivec self-assigned this Mar 20, 2017
@mzazrivec mzazrivec added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 20, 2017
@mzazrivec mzazrivec merged commit 6b79e9b into ManageIQ:master Mar 20, 2017
@himdel himdel deleted the credentials-vmscope branch March 24, 2017 19:43
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