Drop Select2 #7701

Etheryte · 2023-10-17T22:16:40Z

What does this PR change?

Select2 is a dropdown plugin left over from the jQuery days, right now it's only used in a few views which haven't been updated to get rid of it. We got some Dependabot alerts for it and the upgrade path was painful so I went ahead and refactored it out to use our more modern form components instead.

GUI diff

Cosmetic differences in how the dropdown looks, now those dropdowns look the same everywhere.

DONE

Documentation

No documentation needed: Dropping a library so we use the same one everywhere.
DONE

Test coverage

No tests: already covered
DONE

Links

A part of https://github.com/SUSE/spacewalk/issues/22581

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"

github-actions · 2023-10-20T15:40:49Z

👋 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/7701/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/7701/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:

For more tips on troubleshooting, see the troubleshooting guide.

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

Etheryte · 2023-10-23T22:27:37Z

web/html/src/manager/visualization/hierarchy.tsx

          <i className={"fa fa-caret-" + (this.state.showFilters ? "up" : "down")} aria-hidden="true"></i>
        </button>
-        <div id="visualization-filter-wrapper"></div>
+        <div id="visualization-filter-wrapper">


This is not actually new DOM, this is the DOM the old jQuery code created, lifted out and bound to React so we can use our own form components.

Etheryte · 2023-10-23T22:28:32Z

web/html/src/manager/visualization/hierarchy.tsx

-    .on("click", (d) => {
-      showFilterTab("partitioning-tab");
-    });
+// NB! This is a magic constant


Here and below, this is not actually new code, almost all of it has been lifted out of the old jQuery etc code and modified to work with React.

Etheryte · 2023-10-24T11:23:48Z

Fyi for QE: I also went ahead and updated/removed the relevant code from step definitions as well, so far PR acceptance tests fail at an earlier stage so I'm still waiting for the results on those.

srbarrios · 2023-11-28T08:36:28Z

testsuite/features/reposync/reference_srv_sync_products_extra.feature

@@ -37,7 +37,7 @@ Feature: Synchronize extra products in the products page of the Setup Wizard
    When I follow the left menu "Admin > Setup Wizard > Products"
    And I wait until I do not see "Loading" text
    And I enter "Ubuntu" as the filtered product description
-    And I select "amd64-deb" in the dropdown list of the architecture filter
+    And I select "amd64-deb" from "product-arch-filter"


Ideally, we better hide this technical detail in the step definition.
That way, if we change this id in the future, we only need to change it in the step definition and not in all the scenarios using it.

Hi @Etheryte the Gherkin syntax and Cucumber scenarios have been designed to use natural language to describe the steps a user must follow to achieve something. Ideally, we should write the scenarios as if were on the official documentation. So, even if other steps don't follow this good practice, let's try to do it properly wherever we can.

The good news is that's easy to fix, we only need to create a new step definition, where inside it we use this id.

Yes, we should avoid using internals as much as we could. The "feature" part should remain "as the end user sees it", and end users don't see ids when they are in front of the UI, they see texts...

That being said, instead of adding new specialized steps, we might try to write a generic step that uses texts instead of ids and labels.

srbarrios · 2023-11-28T08:38:03Z

testsuite/features/secondary/srv_dist_channel_mapping.feature

+    And I select "x86_64" from "architecture"
+    And I select "SLE-Product-SLES15-SP4-Pool for x86_64" from "channel_label"


It would be better to hide these technical details inside new step definitions too.
Personally, I think that from the user's point of view, knowing that we are acting on a dropdown web element, if that is still the case, can help.

In that case, do you want to add this to all other tests that use dropdowns? We do the same for dropdowns everywhere else: And I select "equals" from "matcher", And I select "1-centos7_ssh_minion_key" from "activationKeys" etc. Currently only the steps with Select2 were different, this change makes it be the same way everywhere. Unless I misunderstand what you mean?

@srbarrios ping.

OK, I think I did not get it the first time. I see, so we already have a step definition doing exactly the same, but without dropdown word appended. That's it?
In such case, in my opinion, I would better keep the step that has the word dropdown and replace the scenarios where we use the other step. The reasoning still the same, try to describe with natural language in a visual manner the actions that a user must follow.

Still, we should improve it later, and don't use element ids as step definition parameters, exposing them in the Gherkin text, instead, we should be hiding this inside the step.

Example:

When(/^I select "([^"]*)" from (activation keys|filter match|channel label) dropdown$/) do |option, dropdown| field = case dropdown when 'activation keys' 'activationKeys' when 'filter match' 'matcher' when 'channel_label' 'channel_label' else raise ScriptError, "Unknown dropdown '#{dropdown}'" end if has_select?(field, with_options: [option], wait: 1) select(option, from: field) else # Custom React selector xpath_field = "//*[contains(@class, 'data-testid-#{field}-child__control')]" xpath_option = ".//*[contains(@class, 'data-testid-#{field}-child__option') and contains(text(), '#{option}')]" find(:xpath, xpath_field).click find(:xpath, xpath_option, match: :first).click end end

Or see my proposal above.

Etheryte · 2023-12-04T16:42:17Z

@cbbayburt Would you have the time to look at this at some point please?

cbbayburt

Hard to coprehend everything, but LGTM 👍 Thanks for all the cleanup :)

Bischoff

see comments

Bischoff · 2023-12-14T09:24:22Z

testsuite/features/reposync/reference_srv_sync_products_extra.feature

@@ -37,7 +37,7 @@ Feature: Synchronize extra products in the products page of the Setup Wizard
    When I follow the left menu "Admin > Setup Wizard > Products"
    And I wait until I do not see "Loading" text
    And I enter "Ubuntu" as the filtered product description
-    And I select "amd64-deb" in the dropdown list of the architecture filter
+    And I select "amd64-deb" from "product-arch-filter"


Yes, we should avoid using internals as much as we could. The "feature" part should remain "as the end user sees it", and end users don't see ids when they are in front of the UI, they see texts...

That being said, instead of adding new specialized steps, we might try to write a generic step that uses texts instead of ids and labels.

Bischoff · 2023-12-14T09:25:38Z

testsuite/features/secondary/srv_dist_channel_mapping.feature

+    And I select "x86_64" from "architecture"
+    And I select "SLE-Product-SLES15-SP4-Pool for x86_64" from "channel_label"


Or see my proposal above.

Bischoff · 2023-12-14T09:26:26Z

testsuite/features/step_definitions/navigation_steps.rb

@@ -202,15 +202,6 @@
  end
 end

-# select an item from any dropdown


I checked, it's not used anywhere else, good.

Bischoff · 2023-12-14T09:40:00Z

testsuite/features/step_definitions/setup_steps.rb

@@ -73,14 +73,6 @@
  end
 end

-When(/^I select "(.*?)" in the dropdown list of the architecture filter$/) do |architecture|


same, i checked it is not used anywhere else, good

Bischoff

We should not use ids nor labels, but we can do that in a separate PR. LGTM.

github-actions bot added java java_lint_checkstyle java_pgsql_tests ui javascript_lint webapp needs-translation labels Oct 17, 2023

uyuni-project deleted a comment from github-actions bot Oct 23, 2023

Etheryte commented Oct 23, 2023

View reviewed changes

github-actions bot added secondary-features testing ruby_rubocop test-framework labels Oct 24, 2023

Etheryte force-pushed the select2 branch from 10a86a3 to 8f4e0b0 Compare October 24, 2023 10:00

Etheryte changed the title ~~WIP: Drop Select2~~ Drop Select2 Oct 24, 2023

Etheryte marked this pull request as ready for review October 24, 2023 11:24

Etheryte requested review from a team as code owners October 24, 2023 11:24

Etheryte requested review from lucidd and cbbayburt and removed request for a team October 24, 2023 11:24

Etheryte force-pushed the select2 branch 3 times, most recently from c8849b6 to cb21670 Compare October 26, 2023 09:40

lucidd approved these changes Nov 27, 2023

View reviewed changes

srbarrios reviewed Nov 28, 2023

View reviewed changes

cbbayburt approved these changes Dec 5, 2023

View reviewed changes

Drop Select2

dfe01e3

Etheryte force-pushed the select2 branch from cb21670 to dfe01e3 Compare December 13, 2023 18:47

Etheryte added 2 commits December 13, 2023 19:49

Fix rebase

d6824e1

Remove unnecessary escape

ad083a6

srbarrios requested a review from a team December 14, 2023 08:24

Bischoff requested changes Dec 14, 2023

View reviewed changes

Bischoff reviewed Dec 14, 2023

View reviewed changes

Bischoff approved these changes Dec 14, 2023

View reviewed changes

Etheryte merged commit 4f70588 into uyuni-project:master Dec 15, 2023
21 checks passed

Etheryte mentioned this pull request Dec 20, 2023

Fix missing action chain schedule initial value #8068

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Select2 #7701

Drop Select2 #7701

Etheryte commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 20, 2023 •

edited

Loading

Etheryte Oct 23, 2023

Etheryte Oct 23, 2023

Etheryte commented Oct 24, 2023

srbarrios Nov 28, 2023

srbarrios Dec 14, 2023

Bischoff Dec 14, 2023 •

edited

Loading

srbarrios Nov 28, 2023 •

edited

Loading

Etheryte Nov 28, 2023

Etheryte Dec 4, 2023

srbarrios Dec 14, 2023 •

edited

Loading

Bischoff Dec 14, 2023

Etheryte commented Dec 4, 2023

cbbayburt left a comment

Bischoff left a comment

Bischoff Dec 14, 2023 •

edited

Loading

Bischoff Dec 14, 2023

Bischoff Dec 14, 2023 •

edited

Loading

Bischoff Dec 14, 2023

Bischoff left a comment

		And I select "x86_64" from "architecture"
		And I select "SLE-Product-SLES15-SP4-Pool for x86_64" from "channel_label"

Drop Select2 #7701

Drop Select2 #7701

Conversation

Etheryte commented Oct 17, 2023 • edited Loading

What does this PR change?

GUI diff

Documentation

Test coverage

Links

Changelogs

Re-run a test

github-actions bot commented Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Etheryte commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bischoff Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

srbarrios Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srbarrios Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Etheryte commented Dec 4, 2023

cbbayburt left a comment

Choose a reason for hiding this comment

Bischoff left a comment

Choose a reason for hiding this comment

Bischoff Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bischoff Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bischoff left a comment

Choose a reason for hiding this comment

Etheryte commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 20, 2023 •

edited

Loading

Bischoff Dec 14, 2023 •

edited

Loading

srbarrios Nov 28, 2023 •

edited

Loading

srbarrios Dec 14, 2023 •

edited

Loading

Bischoff Dec 14, 2023 •

edited

Loading

Bischoff Dec 14, 2023 •

edited

Loading