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

[WIP] miqToolbarOnClick - use explicit param send_checked #642

Closed

Conversation

vecerek
Copy link

@vecerek vecerek commented Mar 9, 2017

send_checked toolbar param added to all toolbar buttons having url_parms =~ /_div$/.
Can't yet make rubocop to align the hashes only with its autocorrect feature. Any ideas?

/cc @himdel

Links

Parent issue: #588

@@ -1522,7 +1522,7 @@ function miqToolbarOnClick(_e) {
// put url_parms into params var, if defined
var params;
if (button.data("url_parms")) {
if (button.data('url_parms').match("_div$")) {
if (button.data("send_checked")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any chance you can pull the send_checked condition out of the url_parms condition, while preserving the current behaviour?

Copy link
Author

Choose a reason for hiding this comment

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

maybe?

if (button.data("send_checked")) {
  if (ManageIQ.gridChecks.length) {
    params = "miq_grid_checks=" + ManageIQ.gridChecks.join(',');
  } else {
    params = miqSerializeForm(button.data('url_parms'));
  }
} else if (button.data("url_parms")) {
  params = button.data('url_parms').split("?")[1];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

..except that params = miqSerializeForm(button.data('url_parms')) should happen when url_parms but not send_checked.

@himdel
Copy link
Contributor

himdel commented Mar 9, 2017

As for the rockets, well, one idea is # rubocop:disable AlignHash :). (And I would consider it valid enough here.)

But I think I may have another solution.. will let you know :)

@himdel
Copy link
Contributor

himdel commented Mar 9, 2017

@vecerek ..this seems to work :) https://github.com/himdel/dotfiles/blob/master/bin/rockets.pl

A bit cumbersome in that it's a perl script that outputs a perl script, but .. it seems to align the rockets quite nicely :).

rockets.pl file.rb > x.pl
perl -i x.pl file.rb

@himdel himdel self-assigned this Mar 10, 2017
} else {
params = button.data('url_parms').split("?")[1];
params = (params ? '&' : '') + button.data('url_parms').split("?")[1];
Copy link
Contributor

@himdel himdel Mar 10, 2017

Choose a reason for hiding this comment

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

(params ? params + '&' : '') + ... I guess? ;)

Copy link
Author

Choose a reason for hiding this comment

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

params += (params ? '&' : '') seems simpler

@himdel
Copy link
Contributor

himdel commented Mar 10, 2017

OK, LGTM 👍 .. (have not tested in the UI yet)

Do you want to do the rockets thing first, or should I just test and merge?

} else {
params = button.data('url_parms').split("?")[1];
params += (params ? '&' : '') + button.data('url_parms').split("?")[1];
Copy link
Contributor

@himdel himdel Mar 10, 2017

Choose a reason for hiding this comment

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

Aaah, doesn't work unless you initialize params to "".

(Because undefined + "" == "undefined", of course ;) )

Copy link
Author

Choose a reason for hiding this comment

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

@himdel rebased, corrected, pushed
Thanks for pointing that one out 😅

@miq-bot
Copy link
Member

miq-bot commented Mar 15, 2017

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

@himdel
Copy link
Contributor

himdel commented Mar 27, 2017

@vecerek ping ;)

@vecerek vecerek force-pushed the add_send_checked_button_param branch from 6cc798a to ad572c2 Compare April 6, 2017 11:05
@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

Checked commits vecerek/manageiq-ui-classic@9019b8a~...ad572c2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
154 files checked, 453 offenses detected

app/helpers/application_helper/toolbar/arbitration_profiles_center.rb

app/helpers/application_helper/toolbar/auth_key_pair_clouds_center.rb

app/helpers/application_helper/toolbar/availability_zones_center.rb

app/helpers/application_helper/toolbar/catalogitem_button_center.rb

app/helpers/application_helper/toolbar/catalogitem_buttons_center.rb

app/helpers/application_helper/toolbar/chargeback_center.rb

app/helpers/application_helper/toolbar/chargebacks_center.rb

app/helpers/application_helper/toolbar/cloud_networks_center.rb

app/helpers/application_helper/toolbar/cloud_object_store_container_center.rb

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

app/helpers/application_helper/toolbar/cloud_object_store_containers_center.rb

app/helpers/application_helper/toolbar/cloud_object_store_objects_center.rb

app/helpers/application_helper/toolbar/cloud_tenants_center.rb

app/helpers/application_helper/toolbar/cloud_volume_backups_center.rb

app/helpers/application_helper/toolbar/cloud_volume_snapshots_center.rb

app/helpers/application_helper/toolbar/cloud_volumes_center.rb

app/helpers/application_helper/toolbar/compare_center.rb

app/helpers/application_helper/toolbar/compare_view.rb

app/helpers/application_helper/toolbar/condition_center.rb

app/helpers/application_helper/toolbar/configuration_jobs_center.rb

app/helpers/application_helper/toolbar/configuration_manager_providers_center.rb

app/helpers/application_helper/toolbar/configuration_script_center.rb

app/helpers/application_helper/toolbar/configuration_scripts/policy_mixin.rb

app/helpers/application_helper/toolbar/configured_system/lifecycle_mixin.rb

app/helpers/application_helper/toolbar/configured_system/policy_mixin.rb

app/helpers/application_helper/toolbar/container_builds_center.rb

app/helpers/application_helper/toolbar/container_center.rb

app/helpers/application_helper/toolbar/container_groups_center.rb

app/helpers/application_helper/toolbar/container_image_registries_center.rb

app/helpers/application_helper/toolbar/container_images_center.rb

app/helpers/application_helper/toolbar/container_nodes_center.rb

app/helpers/application_helper/toolbar/container_projects_center.rb

app/helpers/application_helper/toolbar/container_replicators_center.rb

app/helpers/application_helper/toolbar/container_routes_center.rb

app/helpers/application_helper/toolbar/container_services_center.rb

app/helpers/application_helper/toolbar/container_templates_center.rb

app/helpers/application_helper/toolbar/containers_center.rb

app/helpers/application_helper/toolbar/custom_button_center.rb

app/helpers/application_helper/toolbar/custom_buttons_center.rb

app/helpers/application_helper/toolbar/customization_templates_center.rb

app/helpers/application_helper/toolbar/diagnostics_region_center.rb

app/helpers/application_helper/toolbar/dialog_center.rb

app/helpers/application_helper/toolbar/dialogs_center.rb

app/helpers/application_helper/toolbar/drift_center.rb

app/helpers/application_helper/toolbar/drifts_center.rb

app/helpers/application_helper/toolbar/ems_block_storages_center.rb

app/helpers/application_helper/toolbar/ems_clouds_center.rb

app/helpers/application_helper/toolbar/ems_clusters_center.rb

app/helpers/application_helper/toolbar/ems_containers_center.rb

app/helpers/application_helper/toolbar/ems_datawarehouses_center.rb

app/helpers/application_helper/toolbar/ems_infras_center.rb

app/helpers/application_helper/toolbar/ems_middlewares_center.rb

app/helpers/application_helper/toolbar/ems_networks_center.rb

@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

...continued

app/helpers/application_helper/toolbar/ems_object_storages_center.rb

app/helpers/application_helper/toolbar/ems_physical_infras_center.rb

app/helpers/application_helper/toolbar/ems_storages_center.rb

app/helpers/application_helper/toolbar/flavors_center.rb

app/helpers/application_helper/toolbar/floating_ips_center.rb

app/helpers/application_helper/toolbar/generic_object_definition.rb

  • ❗ - Line 14, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 24, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 25, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 26, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 36, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 37, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 38, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 39, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/helpers/application_helper/toolbar/host_aggregates_center.rb

app/helpers/application_helper/toolbar/host_center.rb

app/helpers/application_helper/toolbar/host_cloud_service_center.rb

app/helpers/application_helper/toolbar/host_cloud_services_center.rb

app/helpers/application_helper/toolbar/hosts_center.rb

app/helpers/application_helper/toolbar/infra_networkings_center.rb

app/helpers/application_helper/toolbar/iso_datastores_center.rb

app/helpers/application_helper/toolbar/ldap_regions_center.rb

app/helpers/application_helper/toolbar/load_balancers_center.rb

app/helpers/application_helper/toolbar/middleware_datasources_center.rb

app/helpers/application_helper/toolbar/middleware_deployments_center.rb

app/helpers/application_helper/toolbar/middleware_messagings_center.rb

app/helpers/application_helper/toolbar/middleware_server_center.rb

app/helpers/application_helper/toolbar/middleware_server_group_center.rb

  • ❗ - Line 40, Col 24 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 41, Col 24 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 51, Col 24 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 52, Col 24 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 96, Col 24 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/helpers/application_helper/toolbar/middleware_servers_center.rb

app/helpers/application_helper/toolbar/miq_action_center.rb

app/helpers/application_helper/toolbar/miq_ae_domain_center.rb

app/helpers/application_helper/toolbar/miq_ae_domains_center.rb

app/helpers/application_helper/toolbar/miq_ae_instances_center.rb

app/helpers/application_helper/toolbar/miq_ae_methods_center.rb

app/helpers/application_helper/toolbar/miq_ae_namespace_center.rb

app/helpers/application_helper/toolbar/miq_alert_center.rb

app/helpers/application_helper/toolbar/miq_alert_profile_center.rb

app/helpers/application_helper/toolbar/miq_dialog_center.rb

app/helpers/application_helper/toolbar/miq_dialogs_center.rb

app/helpers/application_helper/toolbar/miq_event_center.rb

app/helpers/application_helper/toolbar/miq_groups_center.rb

app/helpers/application_helper/toolbar/miq_policy_center.rb

app/helpers/application_helper/toolbar/miq_policy_profile_center.rb

app/helpers/application_helper/toolbar/miq_report_center.rb

app/helpers/application_helper/toolbar/miq_report_schedules_center.rb

app/helpers/application_helper/toolbar/miq_requests_center.rb

app/helpers/application_helper/toolbar/miq_schedules_center.rb

app/helpers/application_helper/toolbar/miq_templates_center.rb

@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

...continued

app/helpers/application_helper/toolbar/network_ports_center.rb

app/helpers/application_helper/toolbar/network_routers_center.rb

app/helpers/application_helper/toolbar/openstack_vm_cloud_center.rb

app/helpers/application_helper/toolbar/orchestration_stacks_center.rb

app/helpers/application_helper/toolbar/orchestration_templates_center.rb

app/helpers/application_helper/toolbar/persistent_volumes_center.rb

app/helpers/application_helper/toolbar/pxe_image_types_center.rb

app/helpers/application_helper/toolbar/pxe_servers_center.rb

app/helpers/application_helper/toolbar/resource_pools_center.rb

app/helpers/application_helper/toolbar/saved_reports_center.rb

app/helpers/application_helper/toolbar/scan_profiles_center.rb

app/helpers/application_helper/toolbar/security_groups_center.rb

app/helpers/application_helper/toolbar/service/lifecycle_mixin.rb

app/helpers/application_helper/toolbar/service/policy_mixin.rb

app/helpers/application_helper/toolbar/service/vmdb_mixin.rb

app/helpers/application_helper/toolbar/service_center.rb

app/helpers/application_helper/toolbar/servicetemplate_center.rb

app/helpers/application_helper/toolbar/servicetemplatecatalog_center.rb

app/helpers/application_helper/toolbar/servicetemplatecatalogs_center.rb

app/helpers/application_helper/toolbar/servicetemplates_center.rb

app/helpers/application_helper/toolbar/storage_managers_center.rb

app/helpers/application_helper/toolbar/storages_center.rb

app/helpers/application_helper/toolbar/tasks_center.rb

app/helpers/application_helper/toolbar/template_clouds_center.rb

app/helpers/application_helper/toolbar/template_infras_center.rb

app/helpers/application_helper/toolbar/tenants_center.rb

app/helpers/application_helper/toolbar/time_profiles_center.rb

app/helpers/application_helper/toolbar/user_roles_center.rb

app/helpers/application_helper/toolbar/users_center.rb

app/helpers/application_helper/toolbar/vm_clouds_center.rb

app/helpers/application_helper/toolbar/vm_infras_center.rb

@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

...continued

app/helpers/application_helper/toolbar/vm_performance.rb

app/helpers/application_helper/toolbar/vms_center.rb

app/helpers/application_helper/toolbar/x_configuration_script_center.rb

app/helpers/application_helper/toolbar/x_foreman_configured_system_center.rb

app/helpers/application_helper/toolbar/x_miq_template_center.rb

app/helpers/application_helper/toolbar/x_provider_foreman_foreman_configured_system_center.rb

app/helpers/application_helper/toolbar/x_template_cloud_center.rb

app/helpers/application_helper/toolbar/x_vm_center.rb

app/helpers/application_helper/toolbar/x_vm_performance.rb

@himdel
Copy link
Contributor

himdel commented Apr 7, 2017

@vecerek looks good, but 2 more things..

In the mean time, a few more toolbars appeared, can you change those too please? (Probably mostly ansible* and physical infra, but not sure..)

And it seems send_checked never gets to JS, probably because the toolbar code ignores it. May need a few changes in ui-components.. src/toolbar/interfaces/toolbar.ts and src/toolbar/components/toolbar-menu/toolbar-button.html most likely.. (not sure if whitelisting stuff there even makes sense..)

@miq-bot
Copy link
Member

miq-bot commented Apr 7, 2017

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

@himdel
Copy link
Contributor

himdel commented Apr 10, 2017

(Added a few comments related to send_checked in custom buttons to #796)

@martinpovolny
Copy link
Member

@himdel: will you take over this one (or assign someone) or will you close it?

Thx!

@martinpovolny martinpovolny changed the title miqToolbarOnClick - use explicit param send_checked [WIP] miqToolbarOnClick - use explicit param send_checked Oct 10, 2017
@himdel
Copy link
Contributor

himdel commented Oct 12, 2017

Already on my list :) this needs to be re-done from scratch now, but I do have all the scripts :)

@himdel
Copy link
Contributor

himdel commented Oct 13, 2017

Closing in favor of multiple PRs - documented in #588

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.

4 participants