feat(import): Add several features to productType json generator #47

junajan · 2016-11-10T11:53:31Z

This PR adds these features:

possibility to import xlsx file
possibility to specify an input encoding
possibility to specify a csv delimiter

…ncoding and csv delimiter

coveralls · 2016-11-10T12:06:34Z

Coverage increased (+3.5%) to 90.038% when pulling 68d9b49 on import-xlsx-encoding into 1499c1a on master.

coveralls · 2016-11-10T13:00:44Z

Coverage increased (+3.5%) to 90.038% when pulling dc4a160 on import-xlsx-encoding into 1499c1a on master.

coveralls · 2016-11-10T13:05:37Z

Coverage increased (+3.5%) to 90.038% when pulling 7569c08 on import-xlsx-encoding into e4bcd75 on master.

hisabimbola · 2016-11-11T12:09:46Z

Instead of having all three in one PR, good if each were separate PRs, especially when they are independent.

junajan · 2016-11-11T12:33:50Z

@hisabimbola - yes I agree but in this case, I just took IO layer from another project and use it here with minor changes.

hisabimbola

Dropped some comments about the PR

hisabimbola · 2016-11-11T12:11:47Z

package.json

    "jszip": "2.5.x",
+    "lodash": "^4.16.6",


why are we using lodash and underscore?

Thank you for a notice - I will unify this.

updated here 9262a82

hisabimbola · 2016-11-11T12:17:39Z

src/coffee/run.coffee

@@ -1,11 +1,15 @@
 _ = require 'underscore'
 Promise = require 'bluebird'
 fs = Promise.promisifyAll require('fs')
+Path = require 'path'


any reason for capitalising Path?

Class are the only Object in javascript that is capitalise.

all requires (except fs) are capitalized there so I was following convention

updated here 9262a82

hisabimbola · 2016-11-11T12:19:46Z

src/coffee/run.coffee

+  fileType = getFileType(path)
+
+  if not isSupportedFileType(fileType)
+    return Promise.reject(Error("File type #{fileType} is not supported. Use one of #{supportedFileTypes}"))


To use the Error object, I think you need to instantiate it with the new keyword

new Error()

updated here 9262a82

hisabimbola · 2016-11-11T12:24:29Z

src/spec/unit/reader.spec.coffee

+
+    stream.end()
+
+createReader = (fileType, delimiter = ',', encoding = 'utf-8' ) ->


Can we factor out all this helper methods for test to a folder helper.

updated here 9262a82

hisabimbola · 2016-11-11T12:28:09Z

src/spec/unit/reader.spec.coffee

+      expect(data[0].numb).to.equal('123')
+
+
+  it 'should not read csv file with unsupported encoding', ->


There are no assertions in this test. How do we validate this test and what it's trying to test.

You can return a promise as a result of the function and mocha will check if the promise ends with an error. This test tries to create an instance of Reader with unsupported encoding which should throw an error.

and if it throw an error, the test should fail right?

I think this test can be rewritten.

Handle all promises and assert that it's fails as we expect.

Please check this update 25bdb72 if it is ok this way.

hisabimbola · 2016-11-11T13:03:36Z

src/coffee/io/reader.coffee

+        if not header
+          header = row
+        else
+          rows.push Reader.mapRow(header, row)


any reason for not using this to access mapRow?

updated here 9262a82

hisabimbola · 2016-11-11T13:06:09Z

src/coffee/io/reader.coffee

+        buffers.push buffer
+      stream.on 'error', (err) -> reject(err)
+      stream.on 'end', =>
+        buffer = Buffer.concat(buffers)


Reason why I said I do not see the importance in streaming with this module. We saving all in memory here. Good as reading the file sync...

There are loaded two files at the same time (attributes and product-type-attributes) so at least we save some time by reading them in parallel instead of synchronous version.

hisabimbola · 2016-11-11T13:08:46Z

src/coffee/io/reader.coffee

+    .then (workbook) ->
+      header = null
+      rows = []
+      worksheet = workbook.getWorksheet(1)


why are we hardcoding 1 here?

I was assuming that the worksheet in the xlsx file will be always the first one (as it is when exporting productTypes using product-type-export tool)

hisabimbola · 2016-11-11T13:10:28Z

src/coffee/product-type-generator.coffee

@@ -196,8 +196,22 @@ class ProductTypeGenerator
    i18n = {}
    languages = @_languages header, _.keys row
    for language in languages
-      if row["#{header}.#{language}"].trim() isnt ''
-        i18n[language] = row["#{header}.#{language}"].trim()
+      # condition was commented out due to an error:


I do not get this comment?

which error?

Meanwhile, we should not commit commented out code.

There was one issue that when you have a productType with LENUM attribute it exported first empty value like this:

"values": [ { "key": "", "label": {} }

Which is wrong because you are missing a value in label and Sphere thus you will get an error: attributes -> type -> elementType -> values -> label: Values of LocalizedString must not be empty.
So I removed that if and now creates first empty value with language code:

"label": { "de": "" }

Anyway - I removed that comment.

updated here 9262a82

hisabimbola · 2016-11-11T13:21:40Z

src/spec/unit/reader.spec.coffee

+      iconv.encode(row, 'win1250')
+    ]
+
+    writeCsv(tempFile, dataInput)


There is a race condition with this test. I think we want to use mocha async method(done) to write this.

Mocha can work with promises too - so you don't have to use a done callback, it will try to use a promise returned from the function instead

Ach so.

Did not know, thanks.

coveralls · 2016-11-14T11:30:41Z

Coverage increased (+3.6%) to 90.189% when pulling 9262a82 on import-xlsx-encoding into e4bcd75 on master.

hisabimbola · 2016-11-14T13:31:37Z

Gruntfile.coffee

          rename: (dest, matchedSrcPath) ->
-            dest + matchedSrcPath
+            dest + matchedSrcPath.replace('coffee', 'js')


Instead of doing this replace and removing ext parameter totally, what if we add the ext with .js only

ext: '.js'

@hisabimbola I tried that but then it was renaming:

abcd.spec.coffee -> abcd.js

helper.coffee -> helper.js

Which did not start any tests because they did not have .spec.js suffix anymore.

Okay. Nice to hear...

hisabimbola · 2016-11-14T13:33:23Z

src/coffee/io/reader.coffee

+
+  read: (file) =>
+    debugLog "READER::stream file %s", file
+    @inputStream = fs.createReadStream file


Yeah, no too much pain point... just we not utilizing the importance of the stream. That is too much for this PR.

Something we can do later, probably in a rewrite.

hisabimbola · 2016-11-14T13:33:32Z

src/coffee/io/reader.coffee

+
+  read: (file) =>
+    debugLog "READER::stream file %s", file
+    @inputStream = fs.createReadStream file


But this is fine for now.

hisabimbola · 2016-11-14T13:35:57Z

src/spec/unit/reader.spec.coffee

+      iconv.encode(row, 'win1250')
+    ]
+
+    writeCsv(tempFile, dataInput)


Ach so.

Did not know, thanks.

hisabimbola · 2016-11-14T13:42:36Z

src/spec/unit/reader.spec.coffee

+      expect(data[0].numb).to.equal('123')
+
+
+  it 'should not read csv file with unsupported encoding', ->


and if it throw an error, the test should fail right?

I think this test can be rewritten.

Handle all promises and assert that it's fails as we expect.

coveralls · 2016-11-15T10:19:35Z

Coverage increased (+3.6%) to 90.189% when pulling 25bdb72 on import-xlsx-encoding into e4bcd75 on master.

junajan · 2016-11-21T15:49:44Z

@hisabimbola - unsupported encoding test updated here 63d8cda

coveralls · 2016-11-21T15:51:50Z

Coverage increased (+3.6%) to 90.189% when pulling 63d8cda on import-xlsx-encoding into e4bcd75 on master.

hisabimbola

LGTM now.

feat(import): Add possibility to import xlsx file, specify an input e…

92bbb1d

…ncoding and csv delimiter

junajan assigned emmenko, PhilippSpo and hisabimbola Nov 10, 2016

PhilippSpo added the Status: In Progress label Nov 10, 2016

junajan added 2 commits November 10, 2016 12:55

fix(import): Add missing dependency

72df7be

fix(import): Add missing dependency

68d9b49

fix(test): Fix cucumber test

dc4a160

Merge branch 'master' into import-xlsx-encoding

7569c08

hisabimbola suggested changes Nov 11, 2016

View reviewed changes

refactor(import): Refactor code due to PR review

9262a82

hisabimbola suggested changes Nov 14, 2016

View reviewed changes

refactor(tests): Refactor test

25bdb72

chore(test): Refactor test of unsupported encoding

63d8cda

hisabimbola approved these changes Nov 22, 2016

View reviewed changes

junajan merged commit c4597aa into master Nov 22, 2016

junajan deleted the import-xlsx-encoding branch November 22, 2016 15:42

PhilippSpo removed the Status: In Progress label Nov 22, 2016


		stream.end()

		createReader = (fileType, delimiter = ',', encoding = 'utf-8' ) ->

		expect(data[0].numb).to.equal('123')


		it 'should not read csv file with unsupported encoding', ->

feat(import): Add several features to productType json generator #47

feat(import): Add several features to productType json generator #47

Conversation

junajan commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

hisabimbola commented Nov 11, 2016

junajan commented Nov 11, 2016

hisabimbola 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junajan Nov 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 14, 2016

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

coveralls commented Nov 15, 2016

junajan commented Nov 21, 2016

coveralls commented Nov 21, 2016

hisabimbola left a comment

Choose a reason for hiding this comment

junajan Nov 11, 2016 •

edited