-
Notifications
You must be signed in to change notification settings - Fork 62
fix xls sheet name length #7901
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { toWorkbook } from '@/components/material/useMaterialViewHelper.js' | ||
|
|
||
| describe('toWorkBook', () => { | ||
| it('replaces invalid characters', () => { | ||
| const sheets = createSheets('s?h*e[]e/t:1:') | ||
|
|
||
| const workbook = toWorkbook(sheets) | ||
|
|
||
| expect(workbook.SheetNames).toEqual(['sheet1']) | ||
| }) | ||
|
|
||
| it('shortens sheets names with more than 31 characters', () => { | ||
| const sheetName = 'a'.repeat(32) | ||
| const sheets = createSheets(sheetName) | ||
|
|
||
| const workbook = toWorkbook(sheets) | ||
|
|
||
| expect(workbook.SheetNames).toEqual([sheetName.slice(0, 31)]) | ||
| }) | ||
|
|
||
| it('does not overwrite sheets when shortening them makes them not unique anymore', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tripped over this test name because I assumed overwrite means changing the sheet name, but you mean it doesn't replace the first one with the same (resulting) name. Maybe "does not drop sheets ..."?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not dropping, its replacing the pointer in the map. I would call that overwriting, but i am open for better formulations. |
||
| const sheets = [ | ||
| ...createSheets('a'.repeat(32)), | ||
| ...createSheets('a'.repeat(33)), | ||
| ...createSheets('a'.repeat(34)), | ||
| ] | ||
| const workbook = toWorkbook(sheets) | ||
|
|
||
| expect(Object.entries(workbook.Sheets)).toHaveLength(sheets.length) | ||
| expect(workbook.SheetNames.filter((name) => name.length > 31)).toEqual([]) | ||
| }) | ||
|
|
||
| it('does not overwrite sheets when they have the same name', () => { | ||
| const sheets = [...createSheets('sheet1'), ...createSheets('sheet1')] | ||
| const workbook = toWorkbook(sheets) | ||
|
|
||
| expect(Object.entries(workbook.Sheets)).toHaveLength(sheets.length) | ||
| }) | ||
| }) | ||
|
|
||
| const data = [['row1']] | ||
|
|
||
| function createSheets(sheetName) { | ||
| return [ | ||
| { | ||
| sheetName: sheetName, | ||
| data, | ||
| }, | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,6 +67,30 @@ async function getSheets(camp, collection, materialList) { | |
| ) | ||
| } | ||
|
|
||
| function randomString(len) { | ||
| const p = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789' | ||
| return [...Array(len)].reduce((a) => a + p[~~(Math.random() * p.length)], '') | ||
| } | ||
|
|
||
| /** | ||
| * @param sheets {array} array of {sheetName: string, data: array} | ||
| * @returns {object} an XLSX workbook | ||
| */ | ||
| export function toWorkbook(sheets) { | ||
| const workbook = XLSX.utils.book_new() | ||
| sheets.forEach(({ sheetName, data }) => { | ||
| const worksheet = XLSX.utils.aoa_to_sheet(data) | ||
| const validSheetName = sheetName.replaceAll(/[?*[\]/\\:]/g, '') | ||
| let slicedSheetName = validSheetName.slice(0, 31) | ||
| if (workbook.SheetNames.includes(slicedSheetName)) { | ||
| slicedSheetName = validSheetName.slice(0, 28) + randomString(3) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you using a random string here? Like: |
||
| } | ||
| workbook.SheetNames.push(slicedSheetName) | ||
| workbook.Sheets[slicedSheetName] = worksheet | ||
| }) | ||
| return workbook | ||
| } | ||
|
|
||
| /** | ||
| * @param {object} camp | ||
| * @param {{value: {period: object, materialItems: {items: array}}[]}} collection | ||
|
|
@@ -76,14 +100,8 @@ function downloadMaterialList(camp, collection, materialList) { | |
| return async () => { | ||
| await camp.activities().$loadItems() | ||
|
|
||
| const workbook = XLSX.utils.book_new() | ||
| const sheets = await getSheets(camp, collection.value, materialList) | ||
| sheets.forEach(({ sheetName, data }) => { | ||
| const worksheet = XLSX.utils.aoa_to_sheet(data) | ||
| const validSheetName = sheetName.replaceAll(/[?*[\]/\\:]/g, '') | ||
| workbook.SheetNames.push(validSheetName) | ||
| workbook.Sheets[validSheetName] = worksheet | ||
| }) | ||
| const workbook = toWorkbook(sheets) | ||
| XLSX.writeFile(workbook, generateFilename(camp, materialList)) | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are plus ou moin testing that slice does what it does, so maybe use 'a'.repeat(31)?