Fix network router buttons for AWS #2234

Ladas · 2017-09-26T09:47:39Z

Depends on:

Enhance NetworkRouter model for Amazon manageiq#16030

Fix network router buttons for AWS and add a bit more info about AWs routers to textual pages.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1539746
https://bugzilla.redhat.com/show_bug.cgi?id=1539736
https://bugzilla.redhat.com/show_bug.cgi?id=1539727
https://bugzilla.redhat.com/show_bug.cgi?id=1539742

Partially implements:
https://bugzilla.redhat.com/show_bug.cgi?id=1488135

Before:
we were not collecting AWS routers

After:

Ladas · 2017-09-26T09:51:37Z

cc @bronaghs routers RFE

@romanblanco can you check if the supports checks are correct? Not sure why they were missing. Also I am a bit confused by list actions, seems like we can't do support checks there? I see that in other models all list actions are commented out, I used the same comment as @ZitaNemeckova with ManageIQ/manageiq#12551 . Not sure if that is a part of a bigger plan to do support checks in list actions.

durandom · 2017-09-26T12:02:54Z

app/helpers/application_helper/toolbar/network_routers_center.rb

-            :enabled   => false,
-            :onwhen    => '1+'),
+          # TODO: Uncomment until cross controllers show_list issue fully in place/also supports? must be in place
+          # https://github.com/ManageIQ/manageiq/pull/12551


this PR has been long merged.
Is there any javascript logic, that enables/disables those buttons?

Yes it was, but the code is commented like this in most of the table toolbars. I don't think we can uncomment it until we solve the supports? there, since now I am getting exceptions in unsupported providers, because of missing methods. (AFAIK there is no JS logic for table level buttons&support mixin)

ok, I dont really understand whats needed from the support mixin here. Maybe @ZitaNemeckova and @romanblanco could chime in, if this is a shortcoming in the supports_feature_mixin

So since the actual supports connected to selected table rows is quite hard to do. We should really have assert similar to assert_privileges("network_router_add_interface")

So e.g. you could select row and click on update, but each controller action would have assert_support(record, "update") and it would redirect you back with flash message "Update on is not supported" (maybe list what records do support it, not sure)

Or we should keep the table actions for individual records hidden, until all providers support them.

That is a simple step before we can actually not show the button.

Ladas · 2017-09-27T12:44:37Z

@miq-bot assign @martinpovolny

Ladas · 2017-09-27T12:44:52Z

@miq-bot add_label enhancement

ohadlevy

thanks, couple of comments + can you please add tests?

ohadlevy · 2017-09-27T14:42:27Z

app/helpers/application_helper/toolbar/network_routers_center.rb

-            :confirm   => N_('Warning: The selected Routers and ALL of their components will be removed!'),
-            :enabled   => false,
-            :onwhen    => '1+'),
+          # TODO: Uncomment when support? for list actions works on some degree


why commit this?

it's like that in other files. Once we deal with supports? these actions are working

although in other files it references non related PR, from what the UI guys told me

I'm not sure I follow, what do you mean by deal with support? bottom line - I can't see how adding in commented out code is acceptable?

@Ladas: I know that you are referring to a situation where someone (bet it was not me) commented out some code.
As I see it, I'd rather delete the code.

On "Once we deal with supports? these actions are working" I don't think so or I don't see how. Please, explain.

I have explained a couple of times why we cannot have complex logic around enabling/disabling, showing/hiding toolbar buttons in list views so I am not going to repeat it here again.

I think that the general approach should be:

having only the basic actions in the list view toolbars and

having the full range of functions in the toolbars on the details screen.

Ah, so the actions are working now, since OpenStack was the only provider with Routers. I am adding AWS routers, so the actions will fail there.

Right, as I mentioned, it can be as as simple as assert_support line in each action, to move the check.

Ok, I will delete the commented code. My first try is always consistence. ;-) (so I research first how we handle list actions elsewhere and voila those are commented out :-D)

ohadlevy · 2017-09-27T14:42:54Z

app/helpers/network_router_helper/textual_summary.rb

+    return nil if @record.main_route_table.nil?
+
+    if @record.main_route_table
+      message = "(Subnets not explicitely assigned to a route table are assigned to this main route table, for this VPC)"


gah, thanks

Ladas · 2017-09-27T15:19:56Z

@ohadlevy I've added specs a long long time ago https://github.com/Ladas/manageiq-ui-classic/blob/679cb360fe7e7b8f180cef1a8e27532ac18b23f3/spec/controllers/network_router_controller_spec.rb#L2 leading to https://github.com/Ladas/manageiq-ui-classic/blob/09b1867835301a066fb884d09bcda3a9072110cb/spec/shared/controllers/shared_examples_for_network_router_controller.rb#L3-L2

ohadlevy · 2017-09-28T07:11:12Z

@Ladas regarding tests, I can't understand how old tests check your new functionality? clearly new tests needs to be written?

ohadlevy · 2017-09-28T07:46:09Z

@Ladas also note the test failures.

Ladas · 2017-10-03T07:57:57Z

@ohadlevy the spec failure is a badly written spec it { is_expected.to eq([@vm1, @vm2]) } it needs to do match_array, cause the order is not guaranteed, so it can randomly fail now.

The current specs are a basic integration specs using the AWS Router model, we do not really test more details of how we render the detail page anywhere.

martinpovolny · 2017-10-03T10:13:52Z

The failing specs are due to a master breakage caused here: ManageIQ/manageiq#15964

martinpovolny · 2017-10-03T10:16:46Z

The current specs are a basic integration specs using the AWS Router model, we do not really test more details of how we render the detail page anywhere.

@Ladas : does that mean that this is a wip, because it lacks the tests? Or is it an excuse for not writing the tests (and later complaining when someone breaks this) ;-) ?

Ladas · 2017-10-03T10:54:10Z

@martinpovolny @ohadlevy no what I mean, I added specs for NetworkRouter for every provider while ago. Now we are actually collecting those for AWS, but we were able to display the AWS routers before.

The specs are https://github.com/Ladas/manageiq-ui-classic/blob/09b1867835301a066fb884d09bcda3a9072110cb/spec/shared/controllers/shared_examples_for_network_router_controller.rb#L3-L2

So, the same basic specs we use for most of the controllers. So if you want more detailed/other specs, let me know what is currently considered as good specs in UI classic. (before we had almost no specs, I added at least these basic specs, what is the next level?)

miq-bot · 2017-10-13T14:17:49Z

This pull request is not mergeable. Please rebase and repush.

martinpovolny · 2017-10-17T08:07:24Z

@Ladas : I am currently deep in the GTLs figuring out how to test it the best way. So let's skip that for this PR.

BUT: you are adding new stuff to: app/helpers/network_router_helper/textual_summary.rb. Is there a spec that runs that code and at least makes sure it renders? That's all I want in this PR.

Thx!

Ladas · 2017-10-17T08:42:14Z

@martinpovolny yes, that is in this spec https://github.com/Ladas/manageiq-ui-classic/blob/09b1867835301a066fb884d09bcda3a9072110cb/spec/shared/controllers/shared_examples_for_network_router_controller.rb#L38

For having 100% coverage, we need to have the right attributes in the factories, which are in AWS repo.

martinpovolny · 2017-10-17T19:30:13Z

For having 100% coverage, we need to have the right attributes in the factories, which are in AWS repo.

What about adding a factory (possibly one that is based on the one from the AWS repo) in this repo? Does that make sense?

Ladas · 2017-10-18T07:09:55Z

@martinpovolny originally all factories were in https://github.com/Ladas/manageiq/blob/22284143d374a1818c6db232d6800df11db52b34/spec/factories/network_router.rb

We are slowly moving them to provider repos e.g. https://github.com/Ladas/manageiq-providers-amazon/blob/616ecc680196711018a3940381a1dd1b6ca30518/spec/factories/floating_ip.rb , network_router is not moved yet

Hm, not sure, I think the plan was to move all 'amazon related' objects under the amazon repo. But when the integration specs live in the UI, maybe we could touch the factories here.

martinpovolny · 2017-10-18T08:36:48Z

Hm, not sure, I think the plan was to move all 'amazon related' objects under the amazon repo. But when the integration specs live in the UI, maybe we could touch the factories here.

We need to test the UI somewhere right? So we need the factories. In the backend they don't have the same need so what happens from time to time, is that they actually remove the factories.

And you are right, the UI tests have the role of integration tests in this case.

Please, create a factory with the necessary values, extend the existing or add a new tests and let's merge this! That should be a 20 minutes' task for you.

Ladas · 2017-10-18T08:57:37Z

@martinpovolny hum, I think we use specific factories not, for the generic tests. Let me see what I can do. (need to finish other stuff first)

Cover update&delete buttons by support? check

Add amazon specific fields to a NetworkRouter detail Partially implements: https://bugzilla.redhat.com/show_bug.cgi?id=1488135

Remove list action because we can not evaluate support check

Fix codeclimate issue

Fix rubocop issues

Add missing translation

miq-bot · 2018-01-29T19:51:55Z

Checked commits Ladas/manageiq-ui-classic@399c044~...53712a4 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

martinpovolny · 2018-01-30T08:39:00Z

Need to add the list of BZs here.
Need to add test coverage for this.

Ladas · 2018-01-30T09:25:55Z

@martinpovolny added list of BZs this fixes

We need specs for buttons (that every button is covered by supports?)

We have specs for textual pages, but we need to enhance them to use more data combination, so starting with various factories in provider repos, that can be then used for testing that the textual pages don't break. Not sure how to generalize that yet.Maybe we could even test that in providers themselves (I think I started that with the shared specs) and somehow use the data from the VCRs (so we don't have to manually mock thousands of data combinations)

Displaying amazon network routers (cherry picked from commit 916d743) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540743 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540744 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540745 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540746

simaishi · 2018-01-31T22:43:42Z

Gaprindashvili backport details:

$ git log -1
commit 01db70a948226b211343712986e5113e0cb7e4d5
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Tue Jan 30 09:38:40 2018 +0100

    Merge pull request #2234 from Ladas/displaying_amazon_network_routers
    
    Displaying amazon network routers
    (cherry picked from commit 916d743d48d13cb8934532033cf7dc55d8b1b55d)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540743
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540744
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540745
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1540746

Ladas · 2018-04-19T12:14:05Z

@simaishi lets change this to Fine/no, this was a RFE

simaishi · 2018-04-19T12:30:23Z

@Ladas Thanks.

@h-kataria I think you added fine/yes for this 5.8.z BZ? Not sure if you can create a Fine PR, just taking what you need for the BZ?

h-kataria · 2018-04-19T13:13:19Z

@simaishi i added fine/yes based upon version on BZ, i don't mind changing it to fine/no

simaishi · 2018-04-19T13:23:55Z

@h-kataria meaning you'll create a separate PR for Fine branch, or we won't be fixing in Fine branch?

Fine specific PR for changes made in ManageIQ#2234 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1544488

Ladas · 2018-04-20T07:57:49Z

Ok, let's change this to Fine/no, it would be nontrivial amount of work to have the backend for Fine

@miq-bot add_label fine/no
@miq-bot remove_label fine/yes

durandom reviewed Sep 26, 2017

View reviewed changes

Ladas closed this Sep 26, 2017

Ladas reopened this Sep 26, 2017

miq-bot assigned martinpovolny Sep 27, 2017

miq-bot added the enhancement label Sep 27, 2017

ohadlevy suggested changes Sep 27, 2017

View reviewed changes

Ladas force-pushed the displaying_amazon_network_routers branch from 1ade6f7 to 721cde5 Compare October 3, 2017 08:09

miq-bot added the unmergeable label Oct 13, 2017

Ladas added 5 commits January 29, 2018 20:43

Cover update&delete buttons by support? check

399c044

Cover update&delete buttons by support? check

Add amazon specific fields to a NetworkRouter detail

c7d049e

Add amazon specific fields to a NetworkRouter detail Partially implements: https://bugzilla.redhat.com/show_bug.cgi?id=1488135

Remove list action because we can not evaluate support check

988d561

Remove list action because we can not evaluate support check

Fix codeclimate issue

d5aab9d

Fix codeclimate issue

Fix rubocop issues

aa51251

Fix rubocop issues

Add missing translation

53712a4

Add missing translation

Ladas force-pushed the displaying_amazon_network_routers branch from 721cde5 to 53712a4 Compare January 29, 2018 19:45

miq-bot removed the unmergeable label Jan 29, 2018

martinpovolny added gaprindashvili/yes networks labels Jan 30, 2018

martinpovolny merged commit 916d743 into ManageIQ:master Jan 30, 2018

simaishi added the blocker label Jan 30, 2018

martinpovolny added this to the Sprint 79 Ending Feb 12, 2018 milestone Jan 30, 2018

Ladas changed the title ~~Displaying amazon network routers~~ Fix network router buttons for AWS Jan 31, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 31, 2018

h-kataria added the fine/yes label Apr 10, 2018

simaishi mentioned this pull request Apr 10, 2018

Enhance NetworkRouter model for Amazon ManageIQ/manageiq#16030

Merged

h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Apr 19, 2018

Removed specific buttons for Network Routers

45510e4

Fine specific PR for changes made in ManageIQ#2234 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1544488

h-kataria mentioned this pull request Apr 19, 2018

[FINE] - Removed specific buttons for Network Routers #3807

Merged

h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Apr 19, 2018

Removed specific buttons for Network Routers

acb2574

Fine specific PR for changes made in ManageIQ#2234 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1544488

miq-bot added fine/no and removed fine/yes labels Apr 20, 2018

Fix network router buttons for AWS #2234

Fix network router buttons for AWS #2234

Conversation

Ladas commented Sep 26, 2017 • edited Loading

Ladas commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Ladas commented Sep 27, 2017

Ladas commented Sep 27, 2017

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas commented Sep 27, 2017 • edited Loading

ohadlevy commented Sep 28, 2017

ohadlevy commented Sep 28, 2017

Ladas commented Oct 3, 2017

martinpovolny commented Oct 3, 2017

martinpovolny commented Oct 3, 2017

Ladas commented Oct 3, 2017

miq-bot commented Oct 13, 2017

martinpovolny commented Oct 17, 2017

Ladas commented Oct 17, 2017

martinpovolny commented Oct 17, 2017

Ladas commented Oct 18, 2017

martinpovolny commented Oct 18, 2017 • edited Loading

Ladas commented Oct 18, 2017

miq-bot commented Jan 29, 2018

martinpovolny commented Jan 30, 2018

Ladas commented Jan 30, 2018

simaishi commented Jan 31, 2018

Ladas commented Apr 19, 2018

simaishi commented Apr 19, 2018

h-kataria commented Apr 19, 2018

simaishi commented Apr 19, 2018

Ladas commented Apr 20, 2018

Ladas commented Sep 26, 2017 •

edited

Loading

Ladas Sep 26, 2017 •

edited

Loading

Ladas Oct 3, 2017 •

edited

Loading

Ladas commented Sep 27, 2017 •

edited

Loading

martinpovolny commented Oct 18, 2017 •

edited

Loading