Fixed the VmDisks sort so bootable disks sort first on the VmDetail page #547

sjd78 · 2018-03-29T21:41:30Z

Resolves issue #511

This change is

Resolves issue oVirt#511

jniederm

otherwise LGTM

jniederm · 2018-03-29T23:30:05Z

src/components/VmDisks/index.js

+
+      return aBoot && !bBoot ? -1
+        : !aBoot && bBoot ? 1
+        : a.get('name').localeCompare(b.get('name'))


Can we please use number aware sorting:

const b = ['r', 'ř10', 'ř9', 'ř'] b.sort((a, b) => a.localeCompare(b, undefined, { numeric: true })) // b is Array [ "r", "ř", "ř9", "ř10" ]

Excellent point. I've added that and some tests so it's a bit more obvious what is intended.

…ocales.

sjd78 · 2018-04-05T03:57:45Z

I moved the disk sort out to its own file to keep the component simple. Adding in explicit locales to the compare is easier to see in the utils.js. The test make it explicit what we are expecting the sort to look like on screen.

package.json got a tweak to fix a jest complaint.

jniederm · 2018-04-05T10:50:37Z

src/components/VmDisks/utils.js

+import { locale as appLocale, DEFAULT_LOCALE } from '../../intl'
+
+function localeCompare (a, b, locale = appLocale) {
+  return a.localeCompare(b, [ locale, DEFAULT_LOCALE ], { numeric: true })


[ locale, DEFAULT_LOCALE ]

IIUIC this is already done at

ovirt-web-ui/src/intl/index.js

Line 29 in e6f8c1f

export const locale: string = getLocaleFromUrl() || getBrowserLocale() || DEFAULT_LOCALE

- locale from intl should always provide valid locale string. So using DEFAULT_LOCALE and exporting it seems redundant.

I did notice that locale coming from intl.js will fallback to DEFAULT_LOCALE. However, the sort function accepts a locale as a parameter, primarily for testing purposes. When the locale passed in isn't correct, and there isn't a fallback given the behavior is inconsistent across browsers.

In short, it is there to protect against edge cases if the function is ever used elsewhere.

There is no test for invalid locale code so reproducibility of test isn't in risk.

Current production code won't provide wrong locale code.

Inconsistent sorting in case of potential future usage and wrong language code isn't much of an issue.

... which makes me find the second item of locales array redundant.

I did a bit more reading and it is redundant and doesn't protect against bad locales passed in via function argument. I'm ok with letting the locale functions throw the errors if the locale isn't correct. I'll adjust accordingly.

Sorry for my confusion, it's been awhile since I dealt with the ECMA Intl APIs.

jniederm · 2018-04-05T11:20:22Z

package.json

@@ -117,7 +117,9 @@
      "^.+\\.css$": "<rootDir>/config/jest/CSSStub.js",
      "ovirtapi": "<rootDir>/src/mock/ovirtapi.mock.js"
    },
-    "scriptPreprocessor": "<rootDir>/config/jest/transform.js",


I'm not able to find doc for scriptPreprocessor property. What is the motivation for this change?

That is exactly why I changed it. scriptPreprocessor was migrated to transform. When I ran the tests, jest reported the change. It is talked about a bit here [1].

[1] - jestjs/jest#1468 (comment)

jniederm

LGTM, optional minor comments

jniederm · 2018-04-05T18:23:00Z

src/components/VmDisks/utils.test.js

+
+  {
+    testTitle: 'name with number',
+    locale: 'cs',


Note, action not required

['r', 'ř10', 'ř9', 'ř'].sort((a, b) => a.localeCompare(b, 'cs', { numeric: true })) ['r', 'ř10', 'ř9', 'ř'].sort((a, b) => a.localeCompare(b, 'en', { numeric: true }))

both returns the same result Array [ "r", "ř", "ř9", "ř10" ]. So this isn't actually testing that other sorting algorithm is used. The distiguishing difference might be the fact that Czech language consider sequence 'ch' as one char that is sorted between 'h' and 'i':

['cecil', 'chris', 'david', 'harry'].sort((a, b) => a.localeCompare(b, 'cs', { numeric: true })) // Array [ "cecil", "david", "harry", "chris" ] ['cecil', 'chris', 'david', 'harry'].sort((a, b) => a.localeCompare(b, 'en', { numeric: true })) // Array [ "cecil", "chris", "david", "harry" ]

Thanks for the language lesson! I'll adjust the tests.

Oh fun. The normal build of nodejs, at least for node 8 and newer, appears to enable the Intl API but only includes English (en). There is a way to include the full locale support. I'll update package.json to do that, however that will also require updating ovirt-engine-nodejs-modules for full CI and build support.

jniederm · 2018-04-05T18:37:45Z

src/components/VmDisks/utils.js

+import { locale as appLocale, DEFAULT_LOCALE } from '../../intl'
+
+function localeCompare (a, b, locale = appLocale) {
+  return a.localeCompare(b, [ locale, DEFAULT_LOCALE ], { numeric: true })


There is no test for invalid locale code so reproducibility of test isn't in risk.

Current production code won't provide wrong locale code.

Inconsistent sorting in case of potential future usage and wrong language code isn't much of an issue.

... which makes me find the second item of locales array redundant.

Add full-icu to devDependencies to enable testing in multiple locales.

sjd78 · 2018-04-05T21:52:41Z

How are new libraries/version handled in ovirt-web-ui? I added full-icu to enable jest to use locales other then 'en' in tests. I see CI passed, but will rpm packaging fail since ovirt-engine-nodejs-modules won't have full-icu available yet?

mareklibra · 2018-04-06T11:10:01Z

@sjd78 , ovirt-engine-nodejs-modules needs to be rebuilt to take effect.
Don't worry about it now, it's one-time action before ovirt-web-ui release.

Introduced by: #547 Breaks recent offline-build by using runtime-determined npm dependencies. The npm package "full-icu" does post-installation setup consisting of downloading "icu4c-data" _in a version corresponding to running nodejs_ which is determined at runtime. Can be fixed by either including all potential icu4c-data versions in ovirt-engine-nodejs-modules or using clean full-icu npm dependency mechanism.

Fixed the VmDisks sort so bootable disks sort first on the VmDetail page

704d255

Resolves issue oVirt#511

jniederm suggested changes Mar 29, 2018

View reviewed changes

Move VM disks sorting to a utility file and add tests in at least 2 l…

c1382de

…ocales.

jniederm suggested changes Apr 5, 2018

View reviewed changes

jniederm approved these changes Apr 5, 2018

View reviewed changes

Adjust locale handling and tests for disk sorting.

9c06ae7

Add full-icu to devDependencies to enable testing in multiple locales.

mareklibra approved these changes Apr 6, 2018

View reviewed changes

mareklibra merged commit 59d9f49 into oVirt:master Apr 6, 2018

mareklibra mentioned this pull request Apr 6, 2018

bootable disk is not first in disks list #511

Closed

sjd78 deleted the issue511 branch April 6, 2018 14:10

mareklibra mentioned this pull request Apr 9, 2018

Revert adding of full-icu package #556

Merged

mareklibra mentioned this pull request Apr 9, 2018

unit test for Czech locale needs ICU data #557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the VmDisks sort so bootable disks sort first on the VmDetail page #547

Fixed the VmDisks sort so bootable disks sort first on the VmDetail page #547

sjd78 commented Mar 29, 2018 •

edited by mareklibra

jniederm left a comment

jniederm Mar 29, 2018

sjd78 Apr 5, 2018

sjd78 commented Apr 5, 2018

jniederm Apr 5, 2018

sjd78 Apr 5, 2018 •

edited

jniederm Apr 5, 2018

sjd78 Apr 5, 2018

jniederm Apr 5, 2018

sjd78 Apr 5, 2018

jniederm left a comment

jniederm Apr 5, 2018

sjd78 Apr 5, 2018

sjd78 Apr 5, 2018

jniederm Apr 5, 2018

sjd78 commented Apr 5, 2018

mareklibra commented Apr 6, 2018

Fixed the VmDisks sort so bootable disks sort first on the VmDetail page #547

Fixed the VmDisks sort so bootable disks sort first on the VmDetail page #547

Conversation

sjd78 commented Mar 29, 2018 • edited by mareklibra

jniederm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjd78 commented Apr 5, 2018

Choose a reason for hiding this comment

sjd78 Apr 5, 2018 • edited

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

jniederm left a 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

sjd78 commented Apr 5, 2018

mareklibra commented Apr 6, 2018

sjd78 commented Mar 29, 2018 •

edited by mareklibra

sjd78 Apr 5, 2018 •

edited