Fixes #24679 - Sorting repos Red Hat repos page #7690

johnsonm325 · 2018-09-10T19:23:18Z

Description of problem: Sorting of available minor version repos is not consistent. To test this bug, upload a manifest and expand the the products on the Red Hat Repositories page.

Example: When we look for "Red Hat Enterprise Linux 7 Server (RPMs)" repos we see "x86_64 7Server" as a first entry. Whereas when we look for "Red Hat Enterprise Linux 6 Server (RPMs)" repos then we see "x86_64 6Server" as the last entry and the sorting is like:

i386 6.1
x86_64 6.1
x86_64 6.10 <<<< After 6.1 we should see 6.2 instead of 6.10
i386 6.10
i386 6.2
x86_64 6.2
i386 6.3
...
...
...
x86_64 6Server
i386 6Server

6Server or 7Server are base repositories and these are most commonly used repos so those should be listed as a first entry.

Solution:

Repos are now grouped by architecture (i386, ia64, x86_64), and then ordered by base repositories first (Server, Workstation, etc.) and then in descending order (7.11, 7.10, 7.9, etc.).

theforeman-bot · 2018-09-10T19:23:25Z

Issues: #24679

jturel · 2018-09-11T14:24:51Z

app/controllers/katello/api/v2/repository_sets_controller.rb

+      architecture_groups.each do |group, repo_sets|
+        sorted_architecture_groups[group] = repo_sets.sort_by do |repo|
+          release_ver = repo[:substitutions][:releasever]
+          release_ver.split('.').map(&:to_i)


I was looking at this line and thinking about what it does - which is give us something like [5, 11] if we are looking at a 5.11 release version. Ruby sort_by is able to understand sorting across multiple elements in an array such as this.

I also observed that there is quite a bit of code here to perform this search including a regular expression which I personally like to avoid unless necessary. With that in mind and also thinking about sorting across multiple values with sort_by I wrote this:

results = repos.sort_by do |repo| major_minor = repo[:substitutions][:releasever].split('.').map(&:to_i) minor = major_minor[1].nil? ? 1 : major_minor[1] [repo[:substitutions][:basearch], major_minor[0], minor] end

I also gave results.reverse to collection

It seems to work but you might want to check the page for examples which are missing data such as the basearch.

jturel · 2018-09-11T14:25:22Z

test/controllers/api/v2/repository_sets_controller_test.rb

+
+      get :available_repositories, params: { product_id: @product.id, id: @content_id }
+
+      correct_version_sort = [{"substitutions" => {"releasever" => "5.11", "basearch" => "i386"}, "path" => ""},


How is the result affected if you add '7Workstation' in addition to existing '7Server' to x86_64 arch?

jturel · 2018-09-11T14:27:30Z

app/controllers/katello/api/v2/repository_sets_controller.rb

@@ -41,7 +41,6 @@ def show
    def available_repositories
      scan_cdn = sync_task(::Actions::Katello::RepositorySet::ScanCdn, @product, @product_content.content.cp_content_id)
      repos = scan_cdn.output[:results]
-


Let's put the empty line back. I think judicious use of newlines makes code more readable within our rubocop guidelines of course!

johnsonm325 · 2018-09-12T18:10:26Z

New commit pushed.

jturel · 2018-09-12T19:09:49Z

app/controllers/katello/api/v2/repository_sets_controller.rb

+        major, minor = repo[:substitutions][:releasever].nil? ? 1000.1000 : repo[:substitutions][:releasever].split('.').map(&:to_i)
+        major = major == 0 ? 1000 : major
+        minor = minor.nil? ? 1000 : minor
+        repo[:substitutions][:basearch] = repo[:substitutions][:basearch].nil? ? "" : repo[:substitutions][:basearch]


In this line you're setting basearch to an empty string where it would have been nil in the response. That's probably not what you wanted to do! Let's change this to preserve (not modify) the basearch on the repo itself.

jturel · 2018-09-12T19:11:36Z

app/controllers/katello/api/v2/repository_sets_controller.rb

@@ -52,11 +52,21 @@ def available_repositories
        end
      end

+      sorted_groups = repos.sort_by do |repo|


Naming nitpick! This variable has the word 'groups' in it. Personally, I would expect it to be a Hash structure of some kind with the keys representing the groups for example. There is no actual "grouping" here in terms of the data structure returned. What about sorted_repos ?

jturel · 2018-09-12T19:15:32Z

test/controllers/api/v2/repository_sets_controller_test.rb

+        content_id.must_equal @content_id
+      end
+
+      task.expects(:output).at_least_once.returns(results: [{:substitutions => {:releasever => "5.10", :basearch => "i386"}, :path => ""},


I see that your mocked data and your expected data are identical - just in a different order. Can you come up with a way to eliminate the duplication and achieve the same result? I think the shuffle method will be handy here: https://ruby-doc.org/core-2.2.0/Array.html#method-i-shuffle

jturel · 2018-09-12T19:17:09Z

test/controllers/api/v2/repository_sets_controller_test.rb

@@ -103,6 +103,51 @@ def test_available_repositories
      assert_response :success
    end

+    def test_available_repo_sort
+      task = assert_sync_task ::Actions::Katello::RepositorySet::ScanCdn do |product, content_id|


The primary concern of this test is sorting, so I think we are OK to remove any assertions against this task especially since they are identical to a pre-existing test.

It can be rewritten:

task = assert_sync_task ::Actions::Katello::RepositorySet::ScanCdn

jturel · 2018-09-12T19:17:40Z

app/controllers/katello/api/v2/repository_sets_controller.rb

@@ -52,11 +52,21 @@ def available_repositories
        end
      end

+      sorted_groups = repos.sort_by do |repo|
+        major, minor = repo[:substitutions][:releasever].nil? ? 1000.1000 : repo[:substitutions][:releasever].split('.').map(&:to_i)
+        major = major == 0 ? 1000 : major


What's the significance of checking for 0 here instead of nil? check like minor version?

johnsonm325 · 2018-09-14T16:15:54Z

Newest commits pushed.

jturel · 2018-09-14T16:48:32Z

test/controllers/api/v2/repository_sets_controller_test.rb

+        { :substitutions => { :releasever => version, :basearch => arch }, :path => path }.with_indifferent_access
+      }
+
+      expected_sort = [repo_set.call(version: nil, arch: "x86_64", path: ""),


since your lambda function provides a default empty path you don't need to set it on your expected sort list

I'm not sure what you are saying here. :)

Maybe that I can just drop the expected_sort altogether? What would hold my final sorted list?

I mean you can change your expected values to ex: repo_set.call(version: nil, arch: "x86_64") and omit the path since your lambda function is giving "" by default. I think that'd be preferred here since your test case isn't concerned with the value in path at all

I see, when you said path, I wasn't thinking of the actual value, "path", but of something else. :)

test/controllers/api/v2/repository_sets_controller_test.rb

johnsonm325 · 2018-09-17T13:18:39Z

New (and final?) commit pushed. :)

johnsonm325 · 2018-09-17T14:28:09Z

[test katello]

jturel · 2018-09-18T16:57:13Z

@johnsonm325 this looks great to me, and I verified that it works. Since I had a bit of a hand in this I think we need an ACK from another developer as well.

For any other devs looking: the API changes were the first attempt and we found that we needed to fix the UI as well, so the API changes are not really relevant. However, since it improves the default sort of the API I am OK with leaving those changes in.

jturel

ACK

johnsonm325 · 2018-09-18T17:57:43Z

Before final ACK, I wanted to discuss best way to test here. This appears to be the best place to test here: https://github.com/akofink/katello/blob/1b2585759b5ed27063f353db038bd06f65944899/webpack/scenes/RedHatRepositories/components/__tests__/RepositorySetRepository.test.js

Would we be building upon this test, or adding to it to check for proper order?

jturel · 2018-09-18T18:04:23Z

webpack/scenes/RedHatRepositories/components/RepositorySetRepositories.js

+        }
+
+        if (repo1.arch === repo2.arch) { // arch is the same
+          // look at major, minor


oh, one ask. these comments are pretty self-explanatory & think they can be removed

jturel · 2018-09-18T22:56:27Z

app/controllers/katello/api/v2/repository_sets_controller.rb

@@ -52,11 +52,21 @@ def available_repositories
        end
      end

+      sorted_repos = repos.sort_by do |repo|
+        major, minor = repo[:substitutions][:releasever].nil? ? 1000.1000 : repo[:substitutions][:releasever].split('.').map(&:to_i)


You might have meant [1000, 1000] here

jturel · 2018-09-18T23:01:31Z

Testing wise it looks like new test file is needed for the JS component. There is one for RepositorySetRepository, but we are here in RepositorySetRepositories

jturel · 2018-09-19T14:28:44Z

webpack/scenes/RedHatRepositories/components/RepositorySetRepositories.js

@@ -28,7 +28,36 @@ class RepositorySetRepositories extends Component {
    }

    const availableRepos = [...data.repositories.filter(({ enabled }) => !enabled)]


@johnpmitsch any advice on how to test availableRepos? Since it's effectively private my first thought would be to test the output of render itself - but want to make sure that is correct since this test relies on external data that we would mock out.

Ultimately we are wanting to test that the RepositorySetRepository components are in the correct order once rendered .. something like:

<RepositorySetRepositories> <RepositorySetRepository ...> <RepositorySetRepository ...> <RepositorySetRepository ...> </RepositorySetRepository>

Does shallow make this an impossiblity?

Would it be easier to move availableRepos out of render and test it as an ordinary method?

A few thoughts:

This feels like something that should be outside of render.

I'm thinking something like this method declared in the class (all untested):

sortedRepos = () => { const { data } = this.props; return [...data.repositories.filter(({ enabled }) => !enabled)] .sort((repo1, repo2) => { //sort logic } }

Then in render() do this:

const availableRepos = this.sortedRepos().map(repo => ( <RepositorySetRepository key={repo.arch + repo.releasever} type={type} {...repo} /> )); const repoMessage = (data.repositories.length > 0 && availableRepos.length === 0 ? __('All available architectures for this repo are enabled.') : __('No repositories available.')); return ( <Spinner loading={data.loading}> {availableRepos.length ? availableRepos : <div>{repoMessage}</div>} </Spinner> );

Then you can test the sortedRepos() function by using the snapshot instance. You may have to modify it to take arguments rather than read from props. There is an example here of testing this method

johnpmitsch · 2018-09-19T14:34:26Z

app/controllers/katello/api/v2/repository_sets_controller.rb

@@ -52,11 +52,21 @@ def available_repositories
        end
      end

+      sorted_repos = repos.sort_by do |repo|
+        major, minor = repo[:substitutions][:releasever].nil? ? 1000.1000 : repo[:substitutions][:releasever].split('.').map(&:to_i)


Sorry to add this late in the review, but is Gem::Version of any use here? We use this in other places when comparing versions

No problem! I don't see any current examples in our code, but looking at what the class does I don't think it'd be a great fit since we are looking at things like architectures and versions that are not just things like '7.3' but also things like '7Server'. Not to say that it couldn't be made to work, but feels like we wouldn't get much value. Open to be shown otherwise though :)

Sounds fair to me. I guess we don't use it in Katello, but I know its used in foreman-maintain since it deals with package versions, maybe that is where I was thinking of. Carry on :)

johnsonm325 · 2018-09-19T20:16:56Z

I know exactly what went wrong with my rubocop offense, but I could not figure out the npm lint error: http://ci.theforeman.org/blue/organizations/jenkins/katello-pr-test/detail/katello-pr-test/3183/pipeline/62

Any clues?

jturel · 2018-09-19T20:42:53Z

webpack/scenes/RedHatRepositories/components/RepositorySetRepositories.js

@@ -16,6 +16,39 @@ class RepositorySetRepositories extends Component {
    }
  }

+  sortedRepos = (repos) => {


I fixed the lint this way:

sortedRepos = repos => [...repos.filter(({ enabled }) => !enabled)] .sort((repo1, repo2) => { const repo1YStream = yStream(repo1.releasever || ''); const repo2YStream = yStream(repo2.releasever || '');

I ran eslint locally with --fix

./node_modules/.bin/eslint webpack/scenes/RedHatRepositories/components/RepositorySetRepositories.js --fix

johnsonm325 · 2018-09-20T03:32:44Z

[test katello]

johnsonm325 · 2018-09-20T13:00:47Z

I'm getting a lint error here that I'm not getting locally. Not sure what I'm missing, but I'm not seeing the issue in the file it's referencing.

@johnpmitsch Do you mind giving the final ack here since you have had eyes on the fix? This is a GA bug, so needs a merge this morning. Thanks!

johnpmitsch · 2018-09-20T13:04:45Z

webpack/scenes/RedHatRepositories/components/__tests__/RepositorySetRepositories.test.js

+
+    expectedIndices.forEach((expected, i) => {
+      expect(result[i]).toEqual(repos[expected]);
+    });


I know there were no tests before, but we really should have a snapshot comparison for any component. here is an example

Does that need to be in the form of toJson(shallowWrapper)? Or can we put the correct order of repos in the snapshot and then expect(result).toMatchSnapshot(); ?

this is a separate test outside of the repo sorting testing, snapshots are a rendered component, which is then used as comparison on any future tests so you can see what has changed in the actual render when you refactor/change anything

Something like what I've added in my new commit?

johnpmitsch · 2018-09-20T13:22:00Z

Linting issue is unrelated to your PR and is being fixed here

johnpmitsch · 2018-09-20T14:06:11Z

webpack/scenes/RedHatRepositories/components/__tests__/RepositorySetRepositories.test.js

+      expect(result[i]).toEqual(repos[expected]);
+    });
+
+    expect(toJson(shallowWrapper)).toMatchSnapshot();


it really should be in its own its block, which means you would have to split out shallowRender too. If you want to keep it in this its block, just adjust the description to sorts repos correctly and renders or something like that.

johnsonm325 · 2018-09-20T14:18:23Z

Ok, this should hopefully be the final run. Thanks for all of the feedback.

johnpmitsch · 2018-09-20T15:14:42Z

[test katello]

johnpmitsch

ACK from a code perspective

johnsonm325 · 2018-09-20T15:59:18Z

@johnpmitsch @jturel Sweet! I think this is ready to be merged then?

theforeman-bot added Needs testing Not yet reviewed labels Sep 10, 2018

jturel reviewed Sep 11, 2018

View reviewed changes

jturel self-assigned this Sep 11, 2018

johnsonm325 force-pushed the MinorVersionSortRepos branch from 5dc9c26 to 46de3f7 Compare September 12, 2018 18:08

jturel suggested changes Sep 12, 2018

View reviewed changes

johnsonm325 force-pushed the MinorVersionSortRepos branch 2 times, most recently from 4fb69b4 to adff7ba Compare September 14, 2018 16:15

jturel reviewed Sep 14, 2018

View reviewed changes

test/controllers/api/v2/repository_sets_controller_test.rb Show resolved Hide resolved

johnsonm325 force-pushed the MinorVersionSortRepos branch from adff7ba to 9e520dc Compare September 17, 2018 13:17

johnsonm325 force-pushed the MinorVersionSortRepos branch 2 times, most recently from 9b507b8 to f6850be Compare September 18, 2018 15:49

jturel approved these changes Sep 18, 2018

View reviewed changes

jturel reviewed Sep 18, 2018

View reviewed changes

jturel reviewed Sep 19, 2018

View reviewed changes

johnpmitsch reviewed Sep 19, 2018

View reviewed changes

jturel added the Demo worthy label Sep 19, 2018

johnsonm325 force-pushed the MinorVersionSortRepos branch from f6850be to 18ad81b Compare September 19, 2018 20:05

jturel reviewed Sep 19, 2018

View reviewed changes

johnsonm325 force-pushed the MinorVersionSortRepos branch from 18ad81b to bb56eca Compare September 19, 2018 23:59

johnpmitsch reviewed Sep 20, 2018

View reviewed changes

johnsonm325 force-pushed the MinorVersionSortRepos branch from bb56eca to 4863ad3 Compare September 20, 2018 13:48

johnpmitsch reviewed Sep 20, 2018

View reviewed changes

Fixes #24679 - Sorting repos Red Hat repos page

2f4be21

johnsonm325 force-pushed the MinorVersionSortRepos branch from 4863ad3 to 2f4be21 Compare September 20, 2018 14:17

johnpmitsch approved these changes Sep 20, 2018

View reviewed changes

jturel merged commit 4252623 into Katello:master Sep 20, 2018


		get :available_repositories, params: { product_id: @product.id, id: @content_id }

		correct_version_sort = [{"substitutions" => {"releasever" => "5.11", "basearch" => "i386"}, "path" => ""},

		@@ -28,7 +28,36 @@ class RepositorySetRepositories extends Component {
		}

		const availableRepos = [...data.repositories.filter(({ enabled }) => !enabled)]

Fixes #24679 - Sorting repos Red Hat repos page #7690

Fixes #24679 - Sorting repos Red Hat repos page #7690

Conversation

johnsonm325 commented Sep 10, 2018

theforeman-bot commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsonm325 commented Sep 12, 2018

Choose a reason for hiding this comment

jturel Sep 12, 2018 • 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

johnsonm325 commented Sep 14, 2018

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

johnsonm325 commented Sep 17, 2018

johnsonm325 commented Sep 17, 2018

jturel commented Sep 18, 2018

jturel left a comment

Choose a reason for hiding this comment

johnsonm325 commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturel commented Sep 18, 2018 • edited Loading

jturel Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

johnpmitsch Sep 19, 2018 • 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

johnsonm325 commented Sep 19, 2018

Choose a reason for hiding this comment

johnsonm325 commented Sep 20, 2018

johnsonm325 commented Sep 20, 2018 • 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

johnpmitsch commented Sep 20, 2018

Choose a reason for hiding this comment

johnsonm325 commented Sep 20, 2018

johnpmitsch commented Sep 20, 2018

johnpmitsch left a comment

Choose a reason for hiding this comment

johnsonm325 commented Sep 20, 2018

jturel Sep 12, 2018 •

edited

Loading

jturel commented Sep 18, 2018 •

edited

Loading

jturel Sep 19, 2018 •

edited

Loading

johnpmitsch Sep 19, 2018 •

edited

Loading

johnsonm325 commented Sep 20, 2018 •

edited

Loading