Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/export xlsx #2658

Closed
wants to merge 4 commits into from
Closed

Conversation

LouisDelbosc
Copy link
Contributor

Change Summary

Add export xlsx button and on nc-gui.
Here the issue#2565

Change type

  • feat: (new feature for the user, not a new feature for build script)
  • fix: (bug fix for the user, not a fix to a build script)
  • docs: (changes to the documentation)
  • style: (formatting, missing semi colons, etc; no production code change)
  • refactor: (refactoring production code, eg. renaming a variable)
  • test: (adding missing tests, refactoring tests; no production code change)
  • chore: (updating grunt tasks etc; no production code change)

Test/ Verification

  • Add new routes to receives csv raw data (instead of files)
  • Add option on More button to have an xlsx export
  • Modify sdk for the client to handle the new export type

Additional information / screenshots (optional)

Screenshot 2022-07-12 at 17 23 53

Screenshot 2022-07-12 at 17 24 09

I decide to have the file construction done by frontend because it was simpler for the backend logic (not duplicate or refactor convoluted code which can be fragile).
If you would like another way of doing things, let me know and I'd gladly implement them.

@o1lab
Copy link
Member

o1lab commented Jul 12, 2022

Thanks for the PR - appreciate it.

However, expectation of export to excel here can mean export the entire database.
Since we are already supporting csv export - importing that csv to excel is a straightforward process.
Please get in touch with us before taking a feature implementation in discord.

@LouisDelbosc
Copy link
Contributor Author

However, expectation of export to excel here can mean export the entire database.
Since we are already supporting csv export - importing that csv to excel is a straightforward process.
Please get in touch with us before taking a feature implementation in discord.

Thanks for the answer. I have numerous users (and potential users) on my nocodb instance who don't know how to import a CSV on excel.
For you and me, it is a straightforward process, but since nocodb want to be a no code solution and since I have this issue with my users and the many more who will follow, I thought it would be a good idea to have it here.

I'm open for any critics and way to solves this problem. If you know a way to do it without having to contribute, I'd be very pleased.

@o1lab o1lab requested a review from wingkwong July 12, 2022 18:29
Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other comments.

  1. Extra empty cells
    image

  2. Discrepancy between exported data (See LastUpdated)
    image

@@ -26,6 +26,7 @@ export default {
dataGroupBy: true,
commentsCount: true,
exportCsv: true,
exportCsvData: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's difference with between exportCsv and exportCsvData? Why not using exportExcel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The xlsx file is created on the frontend. The backend send data as CSV (just data, not file) and the frontend use this data to build a XLSX file.
That's why the endpoint I created is not excel or xlsx because the backend doesn't know about all theses things, it only sends CSV file or CSV data.

@@ -13,6 +13,7 @@ export enum RelationTypes {

export enum ExportTypes {
CSV = 'csv',
CSV_DATA = 'csvData',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be EXCEL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, the backend doesn't build the excel file. But maybe it'd be better to do so ?

@@ -2554,7 +2554,7 @@ export class Api<
orgs: string,
projectName: string,
tableName: string,
type: 'csv' | 'excel',
type: 'csv' | 'excel' | 'csvData',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding a new one instead of using existing excel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I made the backend send data instead of file.

@@ -2917,7 +2917,7 @@ export class Api<
projectName: string,
tableName: string,
viewName: string,
type: 'csv' | 'excel',
type: 'csv' | 'excel' | 'xlsx',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When to use xlsx and when to use csvData?

@@ -18,6 +18,19 @@ async function exportCsv(req: Request, res: Response) {
res.send(data);
}

async function exportCsvData(req: Request, res: Response) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably make a generic function, i.e. combining into exportCsv

@@ -39,4 +56,10 @@ router.get(
ncMetaAclMw(csvDataExport, 'exportCsv')
);

router.get(
'/api/v1/db/data/:orgs/:projectName/:tableName/views/:viewName/export/csvData',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming - similar to other comments.

@wingkwong wingkwong requested a review from mertmit July 13, 2022 07:47
@LouisDelbosc
Copy link
Contributor Author

Other comments.

  1. Extra empty cells
    image
  2. Discrepancy between exported data (See LastUpdated)
    image
  1. It seems to be the case for csv spreadsheet, it is an issue with xlsx ?
  2. I did not see that beforehand thank you. After looking for information, it seems excel store datetime using numbers, I check on numbers or google spreadsheet and it doesn't seems to works as wanted. I need a real excel to check if we have a datetime reading this information. Otherwise, i'm quite disappointed :(

@yohanboniface
Copy link

I did not see that beforehand thank you. After looking for information, it seems excel store datetime using numbers, I check on numbers or google spreadsheet and it doesn't seems to works as wanted. I need a real excel to check if we have a datetime reading this information. Otherwise, i'm quite disappointed :(

Are you setting the right cell type ?

Copy link
Member

@mertmit mertmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, left some comments waiting for your update. You can ask for a review when you are done. Feel free to ask anything on process.

Comment on lines +247 to +267
res = await this.$api.public.csvExport(this.publicViewId, ExportTypes.CSV_DATA, {
query: {
fields:
this.queryParams &&
this.queryParams.fieldsOrder &&
this.queryParams.fieldsOrder.filter(c => this.queryParams.showFields[c]),
offset,
sortArrJson: JSON.stringify(
this.reqPayload &&
this.reqPayload.sorts &&
this.reqPayload.sorts.map(({ fk_column_id, direction }) => ({
direction,
fk_column_id,
}))
),
filterArrJson: JSON.stringify(this.reqPayload && this.reqPayload.filters),
},
headers: {
'xc-password': this.reqPayload && this.reqPayload.password,
},
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should handle shared view case as well. Nothing exported + no corresponding endpoint on backend.

Comment on lines +276 to +285
const { data } = res
const xlsx = XLSX.read(data.csvData, { type: 'string' })
offset = +res.headers['nc-export-offset']
XLSX.writeFile(xlsx, `${this.meta.title}_exported_${c++}.xlsx`)
if (offset > -1) {
this.$toast.info('Downloading more files').goAway(3000)
} else {
this.$toast.success('Successfully exported all table data').goAway(3000)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should be out of if/else block, we should return file for both cases.

ExportTypes.CSV_DATA
)
const { data } = res
const xlsx = XLSX.read(data.csvData, { type: 'string' })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you can use raw: true to avoid unwanted processing of dates, also looks like it fixes extra empty cells.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants