Use xlsx instead of kotlin-csv #70

sohyun-ku · 2023-05-08T03:03:43Z

I used xlsx instead of vue-json-csv. Becuz I want to create a csv file after fetching the snapshot data using callback.

github-actions · 2023-05-08T03:07:56Z

Scavenger Test Results

155 files 155 suites 36s ⏱️
254 tests 242 ✔️ 12 💤 0 ❌
256 runs 244 ✔️ 12 💤 0 ❌

Results for commit 90c8f8a.

♻️ This comment has been updated with latest results.

scavenger-api/src/main/kotlin/com/navercorp/scavenger/controller/SnapshotController.kt

taeyeon-Kim · 2023-05-09T11:15:54Z

We don't use kotlin-csv anymore, so please delete it as well.
Is the reason we can't use vue-json-csv is because we can't handle the download event on click event after calling the api?

taeyeon-Kim · 2023-05-09T11:19:19Z

scavenger-frontend/src/components/snapshot/SnapshotTable.vue

@@ -85,6 +85,8 @@ import SnapshotForm from "./SnapshotForm.vue";
 import Momnet from "moment";
 import {useStore} from "../util/store";
 import {ElMessageBox, ElNotification} from "element-plus";
+import * as Xlsx from 'xlsx'


Previously, the delimiter was tab. Is this the same in xsx?

In xlsx, the default field separator delimiter is ,

https://www.npmjs.com/package/xlsx#delimiter-separated-output

sohyun-ku · 2023-05-10T02:38:57Z

@taeyeon-Kim

Is the reason we can't use vue-json-csv is because we can't handle the download event on click event after calling the api?

If we use vue-json-csv, we can use like below.

<download-csv
    :data   = "json_data">
    Download Data
    <img src="download_icon.png">
</download-csv>

But...There was a problem in the process of receiving data asynchronously when a click event occurred.
There may be several workarounds, but none of the features are explicitly provided by the package. (Belphemur/vue-json-csv#175)
The reason why I used xlsx is that I wanted to implement a function that asynchronously call data when a click event occurs and creates an excel file with a callback.

taeyeon-Kim · 2023-05-10T15:27:47Z

scavenger-api/src/main/kotlin/com/navercorp/scavenger/controller/SnapshotController.kt

@@ -105,31 +97,12 @@ class SnapshotController(
        )
    }

-    @GetMapping("/customers/{customerId}/snapshot/{snapshotId}/export", produces = ["text/csv"])
+    @GetMapping("/customers/{customerId}/snapshots/{snapshotId}/export")


Should we also rename the methods across the board?
It seems like it's now an API to get all snapshot nodes rather than export.

Fixed in d6d4a4e

sohyun-ku · 2023-05-16T10:45:15Z

There is no body size limit for HTTP responses, but added paging because it can take a long time to parse large size JSON data in the browser.
The execution time based on 10000 rows in my local env is within 1s.

taeyeon-Kim · 2023-05-17T03:13:43Z

scavenger-frontend/package.json

@@ -30,7 +30,9 @@
    "splitpanes": "^3.1.5",
    "vue": "^3.2.41",
    "vue-i18n": "^9.2.2",
-    "vue-router": "^4.1.6"
+    "vue-loading-overlay": "^6.0.3",


junoyoon · 2023-05-18T06:35:00Z

scavenger-api/src/main/kotlin/com/navercorp/scavenger/repository/SnapshotNodeDao.kt

    private val sql: SnapshotNodeSql = super.sqls(::SnapshotNodeSql)

-    fun findAllExportSnapshotNode(
+    fun selectAllExportSnapshotNode(


we use find here.

fixed in 90c8f8a

junoyoon · 2023-05-18T06:38:46Z

scavenger-frontend/src/components/snapshot/SnapshotTable.vue

+        }
+
+        utils.book_append_sheet(workBook, workSheet);
+        writeFile(workBook, `snapshot_${id}.csv`);


@sohyun-ku

Could u explain why xlsx should be created in fe layer? what was your intention here?

I opened this issue #69.
In my opinion... it has the advantage of not handling IO on the server and doesn't require direct control of FE's DOM.

I don't think this is really necessary.
This fix blocks the way directly download excel using curl. Moreover, it always need to use the browser.
Instead of this... what about adding excel download beside of csv?

@junoyoon
I hadn't considered downloading excel via curl. 😭
I think that's a good way to go,
@sohyun-ku please comment as well.

I hadn't considered using curl too..haha..

what about adding excel download beside of csv?

Before I did this PR, there was already excel download feature, what does this mean?

@junoyoon Please check out sohyun`s comment.

@sohyun-ku @taeyeon-Kim
Sorry for late response. I was too busy.. :-(

It was csv export feature not excel.
And csv is created in server side.
So it's callable using cli

I think we should make it server side job (not fe).

@junoyoon @taeyeon-Kim

thanks for explaining :)
I didn't think someone can use cli when using csv export feature...
We have already a csv export feature on the server side before this PR.
So I'll close this PR.

Co-authored-by: taeyeonKim <[email protected]>

sohyun-ku requested review from a team and junoyoon as code owners May 8, 2023 03:03

taeyeon-Kim reviewed May 9, 2023

View reviewed changes

scavenger-api/src/main/kotlin/com/navercorp/scavenger/controller/SnapshotController.kt Outdated Show resolved Hide resolved

taeyeon-Kim reviewed May 9, 2023

View reviewed changes

sohyun-ku force-pushed the feature/apply-xlsx-instead-of-kotlin-csv branch from 588c26e to 20e912c Compare May 10, 2023 02:54

taeyeon-Kim reviewed May 10, 2023

View reviewed changes

sohyun-ku force-pushed the feature/apply-xlsx-instead-of-kotlin-csv branch from d6d4a4e to de84c20 Compare May 16, 2023 10:41

taeyeon-Kim approved these changes May 17, 2023

View reviewed changes

sohyun-ku force-pushed the feature/apply-xlsx-instead-of-kotlin-csv branch from 146dd31 to f2b4e9f Compare May 17, 2023 02:56

taeyeon-Kim reviewed May 17, 2023

View reviewed changes

taeyeon-Kim approved these changes May 17, 2023

View reviewed changes

sohyun-ku self-assigned this May 17, 2023

junoyoon reviewed May 18, 2023

View reviewed changes

sohyun-ku and others added 9 commits May 24, 2023 10:27

Use xlsx instead of kotlin-csv

04963eb

Polish

79a9c2b

Co-authored-by: taeyeonKim <[email protected]>

Remove kotlin-csv

33b537d

Polish

2ad04c7

Add Paging to exportSnapshotNode API

4357d80

Add Error handling when export snapshotNode

63f20ee

Add loading plugin

88a2a20

Clean up code

f5ed8cd

Polish

90c8f8a

sohyun-ku force-pushed the feature/apply-xlsx-instead-of-kotlin-csv branch from efe32b6 to 90c8f8a Compare May 24, 2023 01:41

sohyun-ku requested a review from junoyoon June 7, 2023 01:26

sohyun-ku closed this Jun 9, 2023

sohyun-ku mentioned this pull request Jun 9, 2023

Use vue-json-csv instead of kotlin-csv #69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use xlsx instead of kotlin-csv #70

Use xlsx instead of kotlin-csv #70

sohyun-ku commented May 8, 2023

github-actions bot commented May 8, 2023 •

edited

Loading

taeyeon-Kim commented May 9, 2023 •

edited

Loading

taeyeon-Kim May 9, 2023

sohyun-ku May 10, 2023 •

edited

Loading

sohyun-ku commented May 10, 2023

taeyeon-Kim May 10, 2023

sohyun-ku May 11, 2023

sohyun-ku commented May 16, 2023 •

edited

Loading

taeyeon-Kim May 17, 2023

junoyoon May 18, 2023

sohyun-ku May 24, 2023

junoyoon May 18, 2023

taeyeon-Kim May 18, 2023

junoyoon May 25, 2023

taeyeon-Kim May 25, 2023

sohyun-ku May 26, 2023

taeyeon-Kim Jun 1, 2023

junoyoon Jun 9, 2023

sohyun-ku Jun 9, 2023

Use xlsx instead of kotlin-csv #70

Use xlsx instead of kotlin-csv #70

Conversation

sohyun-ku commented May 8, 2023

github-actions bot commented May 8, 2023 • edited Loading

Scavenger Test Results

taeyeon-Kim commented May 9, 2023 • edited Loading

Choose a reason for hiding this comment

sohyun-ku May 10, 2023 • edited Loading

Choose a reason for hiding this comment

sohyun-ku commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohyun-ku commented May 16, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 8, 2023 •

edited

Loading

taeyeon-Kim commented May 9, 2023 •

edited

Loading

sohyun-ku May 10, 2023 •

edited

Loading

sohyun-ku commented May 16, 2023 •

edited

Loading