Remove compressed ids

[imtayadeway]

Removes:

• the miqUncompressedId js function
• the CompressedIds mixin and any references to it
• any references to ApplicationRecord-level compressed id methods

I can identify a few more cleanup tasks, but I think they would be best suited to a follow up PR, given the size and scope of this one.

/cc @chrisarcand @Fryguy
@miq-bot rm-label wip
@miq-bot add-label technical debt
@miq-bot assign @martinpovolny

[miq-bot]

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

[miq-bot]

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

[chrisarcand]

@miq-bot assign @martinpovolny

[imtayadeway]

@miq-bot rm-label wip
@miq-bot add-label technical debt

[martinpovolny]

This is really cool 🍻 !

I am however lacking the courage to merge this so close before the release. What do you think, guys and lads?

ping @AparnaKarve, @h-kataria, @mzazrivec, @himdel, @skateman, @dclarizio ?

[imtayadeway]

I am however lacking the courage to merge this so close before the release.

Totally understand this. Something to bear in mind, though, is that it will make backporting a lot easier if done before

[AparnaKarve]

This is really cool 🍻 !

Totally second that!

Totally understand this. Something to bear in mind, though, is that it will make backporting a lot easier if done before

And totally second that too! :-)

==> GTG from me

[martinpovolny]

==> GTG from me

@AparnaKarve : "GTG" -- does that mean reviewed and tested? Or do you volunteer to do that? I don't have the capacity right now.

[AparnaKarve]

@martinpovolny I can't test this at the moment.
Let us revisit this on Monday.
Thanks.

[dclarizio]

@imtayadeway while I'm not against this, is there some reason this is being done? The original reason we compressed the IDs is because they were taking up a lot of memory on the browser side for trees and lists that had thousands (or tens of thousands) of elements that were identified using the really long IDs. There may even be tree nodes that contain chains of IDs, so that would multiple how much memory is required.

I'm thinking we may want to wait to do this upstream and not in the G release, in case it might affect performance on the browser side.

Thoughts?

[skateman]

@dclarizio actually I was thinking about redesigning the trees to have complex keys. Instead of xy-1z4 we could do something like: {klass: 'Xy', id: 1000000...004} where the klass can remain the same 2 char string (but not necessarily) and the id would be an integer that requires significantly less memory than a string. So this PR is a good step in the direction I'm planning, but I'm also afraid of Gaprindashvili-ing this.

[dclarizio]

So if there's no objections and no real issue to "fix", I suggest we keep this upstream after the gaprindashvili release is cut so we can let it burn in and be tested for performance.

[miq-bot]

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

[martinpovolny]

@dclarizio : we had a discussion on this in Mahwah with a couple of guys. This will simplify things -- having just one form of the ID.
I can imagine there where performance issues with the long IDs on browser side in the past. But I can not imagine that if could be a problem for today's browsers.

I also think that a detailed testing is needed because there might be pieces of code that assume the compressed IDs that are hard to spot and problems can surface after some time.

[himdel]

Note:

after this gets merged, there will be no need for long_id in report_data and we may want to revert parts of #2791, to use id again

[martinpovolny]

@imtayadeway : Tim, the time has come for this to be merged. Will you, please, rebase (hopefully the last time)?

Thank you very much!

[miq-bot]

Some comments on commits imtayadeway/manageiq-ui-classic@1e16218~...93981cd

app/controllers/miq_policy_controller/conditions.rb

• ⚠️ - 78 - Detected pp. Remove all debugging statements.

app/controllers/miq_policy_controller/policy_profiles.rb

• ⚠️ - 60 - Detected pp. Remove all debugging statements.

app/views/miq_policy/_policy_details.html.haml

• ⚠️ - 266 - Detected pp. Remove all debugging statements.

app/views/miq_policy/_profile_list.html.haml

• ⚠️ - 15 - Detected pp. Remove all debugging statements.

app/views/report/_role_list.html.haml

• ⚠️ - 20 - Detected pp. Remove all debugging statements.

[miq-bot]

Some comments on commits imtayadeway/manageiq-ui-classic@1e16218~...93981cd

app/controllers/miq_policy_controller/conditions.rb

• ⚠️ - 78 - Detected pp. Remove all debugging statements.

app/controllers/miq_policy_controller/policy_profiles.rb

• ⚠️ - 60 - Detected pp. Remove all debugging statements.

app/views/miq_policy/_policy_details.html.haml

• ⚠️ - 266 - Detected pp. Remove all debugging statements.

app/views/miq_policy/_profile_list.html.haml

• ⚠️ - 15 - Detected pp. Remove all debugging statements.

app/views/report/_role_list.html.haml

• ⚠️ - 20 - Detected pp. Remove all debugging statements.

[miq-bot]

Checked commits imtayadeway/manageiq-ui-classic@1e16218~...93981cd with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
192 files checked, 77 offenses detected

**

• 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.5-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

app/controllers/ansible_credential_controller.rb

• ❗ - Line 32, Col 7 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

app/controllers/application_controller/buttons.rb

• ❗ - Line 1081, Col 93 - Rails/Present - Use if @edit[:new][:name].present? instead of unless @edit[:new][:name].blank?.
• ❗ - Line 878, Col 16 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/application_controller/miq_request_methods.rb

• ❗ - Line 184, Col 13 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/catalog_controller.rb

• ❗ - Line 183, Col 14 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 211, Col 12 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 251, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 253, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 256, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 259, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/chargeback_controller.rb

• ❗ - Line 339, Col 10 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 428, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/miq_ae_customization_controller/dialogs.rb

• ❗ - Line 1434, Col 17 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/miq_ae_customization_controller/old_dialogs.rb

• ❗ - Line 107, Col 29 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 185, Col 16 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 83, Col 18 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 83, Col 51 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 83, Col 75 - Layout/ExtraSpacing - Unnecessary spacing detected.

app/controllers/miq_policy_controller/events.rb

• ❗ - Line 67, Col 64 - Layout/ExtraSpacing - Unnecessary spacing detected.

app/controllers/mixins/vm_show_mixin.rb

• ❗ - Line 60, Col 14 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/ops_controller/db.rb

• ❗ - Line 119, Col 16 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 124, Col 25 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 129, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 137, Col 18 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 76, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/ops_controller/diagnostics.rb

• ❗ - Line 244, Col 23 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 408, Col 14 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 660, Col 16 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 695, Col 16 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 708, Col 53 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/ops_controller/ops_rbac.rb

• ❗ - Line 893, Col 40 - Style/FormatStringToken - Prefer annotated tokens (like %<foo>s) over template tokens (like %{foo}).
• ❗ - Line 896, Col 41 - Style/FormatStringToken - Prefer annotated tokens (like %<foo>s) over template tokens (like %{foo}).
• ❗ - Line 900, Col 36 - Style/FormatStringToken - Prefer annotated tokens (like %<foo>s) over template tokens (like %{foo}).

app/controllers/ops_controller/settings/common.rb

• ❗ - Line 677, Col 25 - Style/RescueModifier - Avoid using rescue in its modifier form.

app/controllers/ops_controller/settings/ldap.rb

• ❗ - Line 131, Col 68 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 177, Col 22 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 177, Col 24 - Layout/SpaceAroundOperators - Operator = should be surrounded by a single space.
• ❗ - Line 177, Col 61 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 377, Col 24 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 453, Col 31 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/picture_controller.rb

• ❗ - Line 7, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/pxe_controller/iso_datastores.rb

• ❗ - Line 177, Col 14 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 280, Col 26 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 283, Col 26 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/pxe_controller/pxe_customization_templates.rb

• ❗ - Line 220, Col 14 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 220, Col 59 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 220, Col 83 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 243, Col 25 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 249, Col 24 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/pxe_controller/pxe_servers.rb

• ❗ - Line 193, Col 14 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 243, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 446, Col 25 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 449, Col 26 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 452, Col 27 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/report_controller/menus.rb

• ❗ - Line 6, Col 94 - Rails/Present - Use if x_node(:roles_tree).split('-').last.present? instead of unless x_node(:roles_tree).split('-').last.blank?.

app/controllers/report_controller/saved_reports.rb

• ❗ - Line 18, Col 10 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/report_controller/widgets.rb

• ❗ - Line 288, Col 27 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 369, Col 7 - Layout/MultilineMethodCallIndentation - Align .collect with .filtered on line 368.
• ❗ - Line 373, Col 7 - Layout/MultilineMethodCallIndentation - Align .collect with .filtered on line 372.
• ❗ - Line 616, Col 17 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 625, Col 18 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/service_controller.rb

• ❗ - Line 156, Col 9 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/storage_controller/storage_d.rb

• ❗ - Line 47, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/controllers/storage_controller/storage_pod.rb

• ❗ - Line 38, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.
• ❗ - Line 43, Col 34 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/presenters/tree_builder_chargeback_reports.rb

• ❗ - Line 32, Col 11 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_iso_datastores.rb

• ❗ - Line 37, Col 11 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 49, Col 11 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/presenters/tree_builder_pxe_servers.rb

• ❗ - Line 39, Col 22 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 46, Col 22 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 57, Col 10 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

app/presenters/tree_builder_utilization.rb

• ❗ - Line 51, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

spec/services/cloud_topology_service_spec.rb

• ❗ - Line 31, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

spec/services/infra_topology_service_spec.rb

• ❗ - Line 35, Col 88 - Layout/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
• ❗ - Line 71, Col 89 - Layout/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

spec/services/network_topology_service_spec.rb

• ❗ - Line 54, Col 55 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

[imtayadeway]

@martinpovolny this should be GTG now!

[martinpovolny]

Thanks, @imtayadeway. I was testing this in the UI and found no issues.

I know that @h-kataria and @AparnaKarve and @bmclaughlin also will test this a bit today. After that I believe I can merge this.

[imtayadeway]

@martinpovolny thanks for your additional testing! LMK if I can do anything else! ❤️

[h-kataria]

@martinpovolny i have tested in UI for a while, so far all good. I say let's merge this.