Deduplicate tree building. #2059

martinpovolny · 2017-08-31T08:44:26Z

deduplicate tree building, unifying some differences

next step should be removing all the build_*_tree methods and calling the TreeBuilder subclass directly

miq-bot · 2017-09-08T09:33:42Z

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

martinpovolny · 2017-09-18T15:34:03Z

ping @skateman: just rebased with your changes

skateman · 2017-09-20T05:54:24Z

app/controllers/application_controller/explorer.rb

+
+  def try_build_tree(tree_symbol)
+    method_name = "build_#{tree_symbol}_tree"
+    return unless respond_to?(method_name, true)


I'm a little bit afraid that someone might try to remove the build_*_tree methods as they might seem unused after a grepping. Maybe it's over-engineering, but I'd rather keep a list of these methods here and check against. What do you think?

Well I prefer not. I'd like to unify it a bit further and then remove the build_*tree methods calling directly the tree builder.

There's no value and little information in the build_*tree methods

Good enough for me 👍

skateman · 2017-09-20T06:17:05Z

@martinpovolny try to add a new catalog item, it will fail with:

F, [2017-09-20T08:14:21.326513 #1897] FATAL -- : Error caught: [ArgumentError] wrong number of arguments (given 1, expected 0)
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/catalog_controller.rb:2108:in `build_svccat_tree'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/application_controller/explorer.rb:15:in `call'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/application_controller/explorer.rb:15:in `try_build_tree'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/application_controller/explorer.rb:8:in `block in build_replaced_trees'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/application_controller/explorer.rb:8:in `collect'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/application_controller/explorer.rb:8:in `build_replaced_trees'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/catalog_controller.rb:1929:in `replace_right_cell'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/catalog_controller.rb:955:in `atomic_req_submit'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/catalog_controller.rb:95:in `atomic_st_edit'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/catalog_controller.rb:74:in `servicetemplate_edit'

I think you forgot to add the optional argument for some of these build_XY_tree methods.

martinpovolny · 2017-09-20T08:45:53Z

Going to add a spec for that and then fix it... Thx, @skateman !

skateman · 2017-09-27T11:13:24Z

@martinpovolny it seems like the tree isn't reloading at all. I opened a catalog item and tried to remove using the toolbar menu, but the tree has not been reloaded and it still displays the removed node.

martinpovolny · 2017-10-03T16:18:18Z

I am not going to fix the CC issue.

It's not a new one
The code is question is going away with the new dialog editor

skateman · 2017-10-05T07:24:30Z

If you try to add/copy a chargeback rate the action fails
If you add/delete something from the RBAC tree, it doesn't reload

martinpovolny · 2017-10-08T17:24:44Z

@skateman : I have fixed the fist problem + added specs.

martinpovolny · 2017-10-08T17:25:57Z

@skateman : can you be more specific about the RBAC tree problem? I have created an user w/o any issues. What are you doing? Do you really mean the tree of users, groups and roles, when you say "RBAC tree"?

martinpovolny · 2017-10-09T08:41:18Z

@skateman: on RBAC tree: The tree is reloading on add/remove user. After excensive testing my working hypotesis is that the initial state of the tree is our of sync between client and server.

The server thinks that Users are not expanded and Groups + Roles are. The client has Users and Groups expanded and Roles collapsed.

So the update does not bring the newly created data to the client. Thus it seems the tree did not rebuild but it did.

If you click around expand/collapse, things get in sync and everything works. So the problem you are seeing can be seen the first time you get there. Login and go directly to adding or removing an user.

Seems unrelated. I'll retest on master. I have found more bugs in that area :-(

martinpovolny · 2017-10-09T08:55:39Z

The tree unsync issue is a heisenbug. Now I have everything just fine on master or my branch. Don't know what are the conditions for getting the tree out of sync. Might have to do with some reloading and restarting servers while switching branches. Cannot see how it could be related to my changes. I am only changing how the tree building is called and that works.

miq-bot · 2017-10-09T09:04:13Z

Checked commits martinpovolny/manageiq-ui-classic@bb585ab~...7e9e929 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
33 files checked, 0 offenses detected
Everything looks fine. 👍

martinpovolny · 2017-10-09T10:11:06Z

I just recreated the issue on master when adding a Role. Thus confirming it's not related to this PR.

skateman

martinpovolny added fine/no technical debt trees labels Aug 31, 2017

martinpovolny force-pushed the tree_building_dedup branch 3 times, most recently from 4999cdd to 7f5b293 Compare August 31, 2017 14:52

martinpovolny added the wip label Aug 31, 2017

martinpovolny changed the title ~~Deduplicate tree building.~~ [WIPDeduplicate tree building. Aug 31, 2017

martinpovolny changed the title ~~[WIPDeduplicate tree building.~~ [WIP] Deduplicate tree building. Aug 31, 2017

martinpovolny force-pushed the tree_building_dedup branch 4 times, most recently from a65f256 to b601e16 Compare September 5, 2017 10:49

miq-bot added the unmergeable label Sep 8, 2017

martinpovolny force-pushed the tree_building_dedup branch from b601e16 to 8cc7351 Compare September 18, 2017 14:44

miq-bot removed the unmergeable label Sep 18, 2017

martinpovolny changed the title ~~[WIP] Deduplicate tree building.~~ Deduplicate tree building. Sep 19, 2017

martinpovolny removed the wip label Sep 19, 2017

dclarizio requested a review from skateman September 19, 2017 17:21

dclarizio assigned h-kataria Sep 19, 2017

skateman reviewed Sep 20, 2017

View reviewed changes

martinpovolny force-pushed the tree_building_dedup branch 4 times, most recently from 416b048 to 9f9c89b Compare September 27, 2017 10:47

martinpovolny force-pushed the tree_building_dedup branch 2 times, most recently from 5ab1f1f to bff0d2f Compare October 3, 2017 14:39

martinpovolny changed the title ~~[WIP] Deduplicate tree building.~~ Deduplicate tree building. Oct 3, 2017

martinpovolny removed the wip label Oct 3, 2017

martinpovolny force-pushed the tree_building_dedup branch from bff0d2f to 991e878 Compare October 8, 2017 17:23

martinpovolny added 12 commits October 9, 2017 10:51

Deduplicate tree building.

bb585ab

Catalog explorer trees: add a spec for tree replacement.

bd14223

Deduplicate tree building: call build_*_tree with 1 or 0 args.

399f326

Tree building: unify tree build methods naming.

83a95f9

Tree building: unify tree build methods naming.

f980853

Catalog controller: basic tree building spec.

80c6cb1

PXE controller: basic tree building spec.

ba9ccd4

Automate customization controller: basic tree building spec.

acd0940

OpsController: basic tree building spec.

d630c9c

ReportController: basic tree building spec.

c72a8db

Chargeback rates tree: fix args for reload_trees_by_presenter + spec.

7ac8794

Ops Controller spec: trivial style fixes.

7e9e929

martinpovolny force-pushed the tree_building_dedup branch from 991e878 to 7e9e929 Compare October 9, 2017 08:59

skateman approved these changes Oct 9, 2017

View reviewed changes

mzazrivec added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 10, 2017

mzazrivec merged commit 6140a02 into ManageIQ:master Oct 10, 2017

martinpovolny mentioned this pull request Oct 16, 2017

Fix tree building in miq_ae_class controller #2415

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate tree building. #2059

Deduplicate tree building. #2059

martinpovolny commented Aug 31, 2017

miq-bot commented Sep 8, 2017

martinpovolny commented Sep 18, 2017

skateman Sep 20, 2017

martinpovolny Sep 20, 2017

skateman Sep 20, 2017

skateman commented Sep 20, 2017

martinpovolny commented Sep 20, 2017 •

edited

skateman commented Sep 27, 2017

martinpovolny commented Oct 3, 2017

skateman commented Oct 5, 2017

martinpovolny commented Oct 8, 2017

martinpovolny commented Oct 8, 2017

martinpovolny commented Oct 9, 2017

martinpovolny commented Oct 9, 2017 •

edited

miq-bot commented Oct 9, 2017

martinpovolny commented Oct 9, 2017

skateman left a comment

Deduplicate tree building. #2059

Deduplicate tree building. #2059

Conversation

martinpovolny commented Aug 31, 2017

miq-bot commented Sep 8, 2017

martinpovolny commented Sep 18, 2017

skateman Sep 20, 2017

Choose a reason for hiding this comment

martinpovolny Sep 20, 2017

Choose a reason for hiding this comment

skateman Sep 20, 2017

Choose a reason for hiding this comment

skateman commented Sep 20, 2017

martinpovolny commented Sep 20, 2017 • edited

skateman commented Sep 27, 2017

martinpovolny commented Oct 3, 2017

skateman commented Oct 5, 2017

martinpovolny commented Oct 8, 2017

martinpovolny commented Oct 8, 2017

martinpovolny commented Oct 9, 2017

martinpovolny commented Oct 9, 2017 • edited

miq-bot commented Oct 9, 2017

martinpovolny commented Oct 9, 2017

skateman left a comment

Choose a reason for hiding this comment

martinpovolny commented Sep 20, 2017 •

edited

martinpovolny commented Oct 9, 2017 •

edited