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

Call changeAuthTab from more miq_tab_header()s #706

Merged
merged 1 commit into from
Mar 17, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Mar 16, 2017

https://bugzilla.redhat.com/show_bug.cgi?id=1434064 (master)
https://bugzilla.redhat.com/show_bug.cgi?id=1434096 (euwe)

miq_tab_header() once did this automatically.
That was disabled by 2b1da75 in #68 and explicit ng-click
was added to some tab headers but not all. This adds some more.

Should help some tabs in providers add/edit (e.g. containers Hawkular tab) and something dealing with hosts(?), not sure where that is but I see there is changeAuthTab function in host_form_controller.js so added it.

Alas, something still doesn't work and Hawkular tab [Verify] button is broken, there is probably something else going on...
@himdel @AparnaKarve Please review/test.

miq_tab_header() once did this automatically, that was disabled by
2b1da75 and explicit ng-click
was added to some tab headers but not all, this adds some more.
@cben cben mentioned this pull request Mar 16, 2017
@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2017

Checked commit cben@9137125 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 8 offenses detected

app/views/layouts/angular/_multi_auth_credentials.html.haml

  • ⚠️ - Line 10 - Redundant curly braces around a hash parameter.
  • ⚠️ - Line 14 - Redundant curly braces around a hash parameter.
  • ⚠️ - Line 17 - Redundant curly braces around a hash parameter.
  • ⚠️ - Line 20 - Redundant curly braces around a hash parameter.
  • ⚠️ - Line 24 - Redundant curly braces around a hash parameter.
  • ⚠️ - Line 28 - Redundant curly braces around a hash parameter.
  • ⚠️ - Line 30 - Redundant curly braces around a hash parameter.
  • ⚠️ - Line 32 - Redundant curly braces around a hash parameter.

@AparnaKarve
Copy link
Contributor

@himdel It looks like #68 broke some angular functionality for us in forms that have tabs.

changeAuthTab does not trigger an apply/digest cycle anymore while changing tabs because the 'ng-click' => "changeAuthTab changes were applied in the wrong haml : app/views/layouts/_multi_auth_credentials.html.haml

Unfortunately, the PR was backported to darga and euwe, which means we are going to need a BZ here to undo the bug in #68 for darga and euwe.

@AparnaKarve
Copy link
Contributor

@cben The changes look good.
But you'll also need changes to undo what was added in app/views/layouts/_multi_auth_credentials.html.haml in #68
Thanks.

@himdel
Copy link
Contributor

himdel commented Mar 16, 2017

@cben I'm confused.. the change I'm seeing in this PR is exactly the same I did in #68 , isn't it? https://github.com/ManageIQ/manageiq-ui-classic/pull/68/files#diff-bb8a7a7ffa5ad1f77d61eb03b111e361R9

Possibly more to the point, I'm seeing that file with all those ng-click on current master. What am I missing?

@himdel
Copy link
Contributor

himdel commented Mar 16, 2017

@AparnaKarve please don't undo anything, the change is exactly equivalent to before... But do tell me what's actually broken :).

EDIT: (Except in a completely different file O:-))

@himdel
Copy link
Contributor

himdel commented Mar 16, 2017

Aaah.. there's two different files called _multi_auth_credentials, and I've applied the change to the non-angular version instead of the angular version?

Ok, sorry about that, in that case, forget everything I said :D

@himdel
Copy link
Contributor

himdel commented Mar 16, 2017

Also @cben feel free to remove the other multi_auth_credentials. Seems like only the angular version is ever used...

@himdel
Copy link
Contributor

himdel commented Mar 16, 2017

But yes, this will have to be euwe/yes, darga/yes, because of #68.

@himdel himdel self-assigned this Mar 16, 2017
@himdel himdel closed this Mar 16, 2017
@himdel himdel reopened this Mar 16, 2017
@AparnaKarve
Copy link
Contributor

Seems like only the angular version is ever used

True.
Besides ng-click is only an angular thing, and not applicable to the non-angular version of the haml.

@himdel himdel added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 17, 2017
@himdel himdel merged commit 48e62ed into ManageIQ:master Mar 17, 2017
@himdel
Copy link
Contributor

himdel commented Mar 17, 2017

Merged, removing the old one in #725

@himdel
Copy link
Contributor

himdel commented Mar 17, 2017

@cben I guess you'll need to find/create a BZ for this

@cben
Copy link
Contributor Author

cben commented Mar 20, 2017

@simaishi BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1434064
Tested darga & euwe, similar bug there — [Validate] buttons on all Endpoints reflect fields of main Endpoint (precise steps in BZ). Unlike master which also needed #713, this PR is enough for darga & euwe.
Hope you can backport this for today's 5.7 build...

cben added a commit to cben/manageiq that referenced this pull request Mar 20, 2017
Call changeAuthTab from more miq_tab_header()s
miq_tab_header() once did this automatically, that was disabled by
ManageIQ/manageiq-ui-classic@2b1da75
[backported to darga as commit e66e26a in ManageIQ#13471]
and explicit ng-click was added to some tab headers but not all,
this adds some more.

https://bugzilla.redhat.com/show_bug.cgi?id=1434064 (master)
@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit a8a6fc41def7299bbe5dd3a3e90f6e13a811d70f
Author: Martin Hradil <[email protected]>
Date:   Fri Mar 17 13:13:17 2017 +0000

    Merge pull request #706 from cben/changeAuthTab
    
    Call changeAuthTab from more miq_tab_header()s
    (cherry picked from commit 48e62ed81ef51e156e2d883430d4f4c271d9138b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1434096

@simaishi simaishi removed the euwe/yes label Mar 20, 2017
@simaishi
Copy link
Contributor

Backported to Darga via ManageIQ/manageiq#14410

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.

5 participants