fix virtual systems listing filters #8265

rjpmestre · 2024-02-05T13:42:49Z

What does this PR change?

This PR fixes virtual system listing and enhances its filtering system. It adds a new dropdown input field. This will allow users to select either "Virtual Host" or "Virtual System" as the target for their query.

GUI diff

Before:

After:

DONE

Documentation

No documentation needed: There is a GUI difference but the granularity its not high enough to cover these filters.
DONE

Test coverage

Unit tests were added
DONE

Links

Issue(s): https://github.com/SUSE/spacewalk/issues/21192

DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests"

Before you merge

Check How to branch and merge properly!

github-actions · 2024-02-05T13:49:09Z

👋 Hello! Thanks for contributing to our project.
Acceptance tests will take some time (aprox. 1h), please be patient ☕
You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/8265/checks
Once tests finish, if they fail, you can check 👀 the cucumber report. See the link at the output of the action.
You can also check the artifacts section, which contains the logs at https://github.com/uyuni-project/uyuni/pull/8265/checks.

If you are unsure the failing tests are related to your code, you can check the "reference jobs". These are jobs that run on a scheduled time with code from master. If they fail for the same reason as your build, it means the tests or the infrastructure are broken. If they do not fail, but yours do, it means it is related to your code.

Reference tests:

KNOWN ISSUES

Sometimes the build can fail when pulling new jar files from download.opensuse.org . This is a known limitation. Given this happens rarely, when it does, all you need to do is rerun the test. Sorry for the inconvenience.

For more tips on troubleshooting, see the troubleshooting guide.

Happy hacking!
⚠️ You should not merge if acceptance tests fail to pass. ⚠️

github-actions · 2024-02-05T13:49:34Z

Suggested tests to cover this Pull Request

min_deblike_openscap_audit
sle_minion
proxy_branch_network
srv_push_package
srv_menu
srv_datepicker
buildhost_osimage_build_image
srv_restart
min_ssh_tunnel
min_rhlike_salt_install_package_and_patch
min_retracted_patches
min_bootstrap_api
srv_first_settings
srv_advanced_search
srv_patches_page
min_salt_formulas
min_salt_lock_packages
srv_users
srv_handle_software_channels_with_ISS_v2
srv_cobbler_sync
srv_reportdb
min_salt_minions_page
min_rhlike_openscap_audit
srv_salt
allcli_action_chain
srv_docker_cve_audit
srv_power_management
minssh_move_from_and_to_proxy
srv_organization_credentials
min_salt_mgrcompat_state
min_deblike_monitoring
proxy_cobbler_pxeboot
min_cve_id_new_syntax
srv_scc_user_credentials
min_bootstrap_script
srv_logfile
proxy_as_pod_basic_tests
allcli_system_group
buildhost_bootstrap
srv_monitoring
min_rhlike_salt
min_config_state_channel_subscriptions
srv_wait_for_reposync
min_custom_pkg_download_endpoint
srv_mainpage
srv_create_activationkey
min_ansible_control_node
min_virthost
srv_rename_hostname
srv_check_reposync
min_salt_formulas_advanced
srv_handle_config_channels_with_ISS_v2
min_project_lotus
srv_change_password
min_salt_pkgset_beacon
srv_check_channels_page
min_check_patches_install
srv_user_configuration_salt_states
min_salt_install_with_staging
allcli_reboot
buildhost_docker_auth_registry
allcli_sanity
min_deblike_salt_install_with_staging
min_rhlike_ssh
srv_clone_channel_npn
allcli_update_activationkeys
min_cve_audit
min_deblike_salt
srv_distro_cobbler
srv_docker_advanced_content_management
srv_add_rocky8_repositories
srv_custom_system_info
srv_sync_channels
min_move_from_and_to_proxy
srv_salt_download_endpoint
minssh_action_chain
minkvm_guests
min_activationkey
srv_maintenance_windows
min_bootstrap_negative
min_bootstrap_reactivation
min_timezone
min_salt_software_states
min_empty_system_profiles
srv_enable_sync_products
srv_create_repository
srv_virtual_host_manager
min_config_state_channel
min_deblike_salt_install_package
srv_manage_channels_page
minssh_ansible_control_node
proxy_register_as_minion_with_script
srv_power_management_api
proxy_retail_pxeboot_and_mass_import
min_salt_install_package
minssh_bootstrap_api
min_monitoring
min_rhlike_monitoring
min_docker_api
min_action_chain
min_salt_openscap_audit
min_salt_minion_details
srv_channel_api
srv_check_sync_source_packages
srv_payg_ssh_connection
srv_cobbler_distro
srv_activationkey_api
allcli_config_channel
minssh_salt_install_package
srv_cobbler_profile
min_recurring_action
srv_delete_channel_from_ui
srv_dist_channel_mapping
srv_user_preferences
srv_group_union_intersection
min_salt_user_states
srv_power_management_redfish
srv_task_status_engine
srv_delete_channel_with_tool
sle_ssh_minion
min_rhlike_remote_command
min_config_state_channel_api
allcli_overview_systems_details
srv_content_lifecycle
min_deblike_ssh
min_deblike_remote_command
srv_osimage
buildhost_docker_build_image
srv_channels_add
min_change_software_channel
srv_change_task_schedule
srv_cobbler_buildiso
allcli_software_channels_dependencies
srv_notifications
min_salt_migration
srv_disable_local_repos_off
min_bootstrap_ssh_key
allcli_software_channels

java/code/src/com/redhat/rhn/manager/system/SystemManager.java

cbosdo · 2024-02-08T07:59:21Z

java/code/src/com/redhat/rhn/manager/system/SystemManager.java

+                            "COALESCE(RS.name, '(none)') AS virtual_host_name, " +
+                            "COALESCE(VII.name, '(none)') AS virtual_vm_name, " +


There is no need to change the names here as this will imply additional fields and getters/setters in the DTO.

cbosdo · 2024-02-08T08:00:18Z

java/code/src/com/redhat/rhn/frontend/dto/VirtualSystemOverview.java

@@ -67,7 +68,8 @@ public VirtualSystemOverview(Tuple tuple) {
            stateLabel = getTupleValue(tuple, "state_label", String.class).orElse(null);
            vcpus = getTupleValue(tuple, "vcpus", Number.class).map(Number::longValue).orElse(0L);
            memory = getTupleValue(tuple, "memory", Number.class).map(Number::longValue).orElse(0L);
-            hostServerName = getTupleValue(tuple, "host_server_name", String.class).orElse(null);
+            virtualHostName = getTupleValue(tuple, "virtual_host_name", String.class).orElse(null);
+            virtualVmName = getTupleValue(tuple, "virtual_vm_name", String.class).orElse(null);


server_name is setup the parent class IIRC. There is no need to add another field for it

cbosdo · 2024-02-08T08:00:31Z

java/code/src/com/redhat/rhn/frontend/dto/VirtualSystemOverview.java

-            hostServerName = getTupleValue(tuple, "host_server_name", String.class).orElse(null);
+            virtualHostName = getTupleValue(tuple, "virtual_host_name", String.class).orElse(null);


Why changing the field name?

yeah, you're right, it goes in line with the original names. already rolled it back

cbosdo · 2024-02-08T08:01:33Z

java/code/src/com/suse/manager/webui/controllers/SystemsController.java

-                "hostServerName", "host_server_name",
-                "name", "VII.name",
+                defaultFilterColumn, "RS.name",
+                "virtual_vm_name", "VII.name",


I'm not sure this rename is really needed

cbosdo · 2024-02-08T08:02:34Z

java/code/src/com/suse/utils/Predicates.java

+import java.util.Optional;
+import java.util.function.Supplier;
+
+public final class Predicates {


You are probably missing a Javadoc comment for this class

cbosdo · 2024-02-08T08:04:06Z

web/html/src/manager/systems/list-filter.tsx

@@ -52,6 +52,15 @@ const allListOptions = [
  { value: "group_count", label: t("Groups") },
 ];

+const virtualSystemsListOptions = [
+  { value: "virtual_host_name", label: t("Virtual Host Name") },
+  { value: "virtual_vm_name", label: t("Virtual VM Name") },


virtual VM is a pleonasm, either use virtual guest, virtual machine or just vm

cbosdo · 2024-02-09T07:50:24Z

web/html/src/manager/systems/virtual-list.tsx

-          header={t("Virtual System")}
+          header={t("Virtual Machine")}


Better not change this name: the word System is used all over the place in the user interface and docs to refer to a machine. But this is nitpicking

Got you! Updated the label on the dropdown as well.

cbbayburt

Thanks, @rjpmestre!

I left 2 minor comments below.

Also, thanks for extensive testing, and introducing parametrized tests ❤️

cbbayburt · 2024-02-13T11:03:33Z

java/code/src/com/suse/utils/Predicates.java

+    /**
+     * Determines whether a CharSequence is null or blank, ie, containing only whitespace characters.
+     *
+     * @param value the CharSequence to be evaluated
+     * @return true if the CharSequence is null or blank
+     */
+    public static boolean isBlank(final CharSequence value) {
+        if (isNull(value)) {
+            return true;
+        }
+
+        int length = value.length();
+        if (length == 0) {
+            return true;
+        }
+
+        for (int i = 0; i < length; i++) {
+            if (!Character.isWhitespace(value.charAt(i))) {
+                return false;
+            }
+        }
+
+        return true;
+    }


This method is already available in StringUtils

Yeah, you're right. I had this sitting for a while now and i cannot recall exactly the small detail this was supposed to address/cover. Well, i should've commented back then. Lets stick to apache's StringUtils.

cbbayburt · 2024-02-13T11:13:29Z

java/code/src/com/suse/manager/webui/controllers/SystemsController.java

+        String filterColumn = requestIn.queryParams("qc");
+        String filterQuery = requestIn.queryParams("q");


You can also use the PageControlHelper to get these parameters. It encapsulates the logic and does some minimal validation.

Suggested change

String filterColumn = requestIn.queryParams("qc");

String filterQuery = requestIn.queryParams("q");

PageControlHelper pageHelper = new PageControlHelper(requestIn);

// pageHelper.getQuery()

// pageHelper.getQueryColumn()

rjpmestre requested review from a team as code owners February 5, 2024 13:42

rjpmestre requested review from nadvornik and parlt91 February 5, 2024 13:42

rjpmestre self-assigned this Feb 5, 2024

github-actions bot added java java_lint_checkstyle java_pgsql_tests API ui javascript_lint old-ui webui-templates labels Feb 5, 2024

github-actions bot added the needs-translation label Feb 5, 2024

rjpmestre marked this pull request as draft February 5, 2024 14:06

rjpmestre force-pushed the um_fix-virtual-systems-filters branch 3 times, most recently from 169a0f4 to aff153f Compare February 7, 2024 17:46

rjpmestre marked this pull request as ready for review February 7, 2024 19:20

rjpmestre requested a review from cbosdo February 7, 2024 19:20

cbosdo requested changes Feb 8, 2024

View reviewed changes

rjpmestre force-pushed the um_fix-virtual-systems-filters branch from aff153f to 6498937 Compare February 8, 2024 13:09

github-actions bot removed API old-ui labels Feb 8, 2024

rjpmestre force-pushed the um_fix-virtual-systems-filters branch from 6498937 to e1fbd32 Compare February 8, 2024 13:12

rjpmestre requested a review from cbosdo February 8, 2024 21:23

cbosdo approved these changes Feb 9, 2024

View reviewed changes

rjpmestre force-pushed the um_fix-virtual-systems-filters branch from e1fbd32 to d59fbe9 Compare February 9, 2024 10:48

rjpmestre force-pushed the um_fix-virtual-systems-filters branch from d59fbe9 to 236d6d7 Compare February 9, 2024 10:57

cbbayburt requested changes Feb 13, 2024

View reviewed changes

fix virtual systems filters

82696d5

rjpmestre force-pushed the um_fix-virtual-systems-filters branch from 236d6d7 to 82696d5 Compare February 14, 2024 15:04

rjpmestre requested a review from cbbayburt February 14, 2024 15:15

cbbayburt approved these changes Feb 14, 2024

View reviewed changes

rjpmestre merged commit 9dabeef into uyuni-project:master Feb 20, 2024
26 checks passed

rjpmestre deleted the um_fix-virtual-systems-filters branch February 21, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix virtual systems listing filters #8265

fix virtual systems listing filters #8265

rjpmestre commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024 •

edited

Loading

cbosdo Feb 8, 2024

cbosdo Feb 8, 2024

cbosdo Feb 8, 2024

rjpmestre Feb 8, 2024

cbosdo Feb 8, 2024

cbosdo Feb 8, 2024

cbosdo Feb 8, 2024

cbosdo Feb 9, 2024

rjpmestre Feb 9, 2024

cbbayburt left a comment

cbbayburt Feb 13, 2024

rjpmestre Feb 14, 2024 •

edited

Loading

cbbayburt Feb 13, 2024

		"COALESCE(RS.name, '(none)') AS virtual_host_name, " +
		"COALESCE(VII.name, '(none)') AS virtual_vm_name, " +

		hostServerName = getTupleValue(tuple, "host_server_name", String.class).orElse(null);
		virtualHostName = getTupleValue(tuple, "virtual_host_name", String.class).orElse(null);

		String filterColumn = requestIn.queryParams("qc");
		String filterQuery = requestIn.queryParams("q");

-        String filterColumn = requestIn.queryParams("qc");
-        String filterQuery = requestIn.queryParams("q");
+        PageControlHelper pageHelper = new PageControlHelper(requestIn);
+        // pageHelper.getQuery()
+        // pageHelper.getQueryColumn()

fix virtual systems listing filters #8265

fix virtual systems listing filters #8265

Conversation

rjpmestre commented Feb 5, 2024 • edited Loading

What does this PR change?

GUI diff

Documentation

Test coverage

Links

Changelogs

Re-run a test

Before you merge

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbbayburt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjpmestre Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjpmestre commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

rjpmestre Feb 14, 2024 •

edited

Loading